Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing module-info.java #40

Closed
lscoughlin opened this issue Feb 11, 2025 · 14 comments
Closed

missing module-info.java #40

lscoughlin opened this issue Feb 11, 2025 · 14 comments

Comments

@lscoughlin
Copy link

You still need a module-info.java for java platform modules ( not OSGI ).

@ethlo
Copy link
Owner

ethlo commented Feb 11, 2025

Need seems to be a strong word. What is that good for? I have never needed this in any project.

@lscoughlin
Copy link
Author

Any libraries ( like networknt's json schema validator ) that use JPMS need this, otherwise you enter into a mess of --add-opens that makes larger builds really painful. I'm not sure how it is that you haven't been exposed to JPMS before but it's been pretty much universally adopted. If you're still largely working in java8 environments check the multi-release specifications int the META-INF - ( like the java8 PR i opened for you yesterday ).

What this means in practice:

  • None of your packages are exposed, so your library may as well not exist for folks running on the module path.
  • For folks running on the classpath it's completely transparent.

@ethlo
Copy link
Owner

ethlo commented Feb 11, 2025

I largely use the later Java releases like Java 17 and 21 in my personal and professional projects. I have thus obviously been exposed to it, but I have yet to find it useful (more of a pain in the neck, tbh).

It obviously doesn't hurt to add the module-info.java.

(PS: Please see the code review comment)

@merks
Copy link

merks commented Feb 11, 2025

Is it intended that this new release's MANIFEST.MF is like this

Manifest-Version: 1.0
Archiver-Version: Plexus Archiver
Created-By: Apache Maven 3.8.7
Built-By: morten
Build-Jdk: 21.0.6

but the previous version was like this:

Manifest-Version: 1.0
Bnd-LastModified: 1736157981273
Build-Jdk-Spec: 21
Bundle-Description: Extremely fast date-time parser and formatter - RFC 
 3339 (ISO 8601 profile) and W3C format
Bundle-License: https://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-ManifestVersion: 2
Bundle-Name: Internet Time Utility
Bundle-SymbolicName: com.ethlo.time.itu
Bundle-Version: 1.10.3
Created-By: Apache Maven Bundle Plugin 5.1.8
Export-Package: com.ethlo.time;version="1.10.3"
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
Tool: Bnd-6.3.1.202206071316
Multi-Release: true

@ethlo
Copy link
Owner

ethlo commented Feb 11, 2025

It seems like it's hard to keep everyone happy. 😅

@merks, I’d recommend checking the release notes first, as they explain the intended rationale behind the changes in this release.

I genuinely appreciate all the feedback and contributions, but I find myself in a situation where bug reports and PRs often conflict with each other, making it difficult to ensure compatibility for everyone.

To clarify my position:

I personally have no need for OSGi and, as a result, no way to properly test it.
I also have no need for multi-module support or Java modules, though I understand others might rely on them.
That said, I'm open to practical, minimal-impact solutions that help users without significantly increasing the maintenance burden. If there's a lightweight way to address some of these concerns without reintroducing complexity, I'm happy to consider it. If we are to maintain multi-modules we may as well keep Java 8 compatibility as well.

Let’s find a path forward that works for most users!

@lscoughlin
Copy link
Author

If you're on java 11, having the module-info.java has no impact on folks that are running on the classpath. For folks that are building libraries or running on the modulepath, not having it makes the library unusable.

I can't stress enough - there is no impact on folks not using the modulepath.

The JPMS requirements and the OSGI requirements are completely orthogonal, and do not conflict in anyway.

The original PR I opened against the JDK8 version would of maintained compatibility for everyone.

I'm happy with the java11 requirement, but folks on the module path need a module descriptor.

I have no opinion about the OSGi stuff, but i'm also not interfering with it in anyway.

@ethlo
Copy link
Owner

ethlo commented Feb 11, 2025

Thanks for your feedback! @merks What is you interest and perspective on this? Is OSGi the concern or the lack of a module descriptor? Or something entirely different. Please advise.

I will likely reverse it to include the module descriptor and maintain the Java 1.8 compatibility and OSGi for the 1.12.x release, but I will give it some time to gather all requirements.

@merks
Copy link

merks commented Feb 11, 2025

Firstly compliments to your responsiveness! 🥇

Sorry for not reading notes. I just saw the one open issue and dived right in.

For your background, I maintain Eclipse's Orbit project which produces a p2 update site with OSGi bundles for reuse across a bunch of Eclipse projects:

https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/table.html

We build this site from a number of parts. One of those parts is for artifacts available at Maven Central as OSGi bundles:

https://github.com/eclipse-orbit/orbit-simrel/blob/cbfd9b6ecb12dbe2262d48531f1e2ed0ef024bd5/maven-osgi/tp/Maven.target#L82-L87

The Orbit tools automatically find new versions to update that file. Before I commit the updates I generally try to build it locally, which fails like this now:

[ERROR] Failed to resolve target definition file:/D:/Users/merks/orbit-simrel/git/orbit-simrel/maven-osgi/tp/Maven.target: Artifact com.ethlo.time:itu:1.11.0 is not a bundle and MissingManifestStrategy is set to error for this location

For artifacts that are not available as OSGi bundles, I need to use BND to wrap them, e.g.,

https://github.com/eclipse-orbit/orbit-simrel/blob/cbfd9b6ecb12dbe2262d48531f1e2ed0ef024bd5/maven-bnd/tp/MavenBND.target#L230-L269

So I just wanted to know if I should "move" ethlo from Maven.target to MavenBND.target and then define the needed BND instructions for it. What I hope to avoid is to do that work, and then later it has an OSGi MANIFEST.MF again, maybe even with a different bundle symbolic name. I might not notice that the BND instructions I've defined are not actually being used later...

So if you plan to restore the OSGi details, which has no potential adverse impact and merely enables consuming the jar as an OSGi bundle in an OSGi runtime, I'll wait for the next release. If your don't plan to do that, then I can move it, and generate the needed OSGi details.

So I don't really have any hard requirements and I appreciate your quick answers.

merks added a commit to eclipse-orbit/orbit-simrel that referenced this issue Feb 11, 2025
- Revisit the issue next time there is a new version.

ethlo/itu#40
@ethlo
Copy link
Owner

ethlo commented Feb 11, 2025

@merks Excellent, and thank you! I could perhaps ask you to code review the PR that will originate from this, considering that you seem to be an expert on the matter.

To anyone reading this, I'm a bit torn on the Java 1.8 compatibility. I see that the https://github.com/networknt/json-schema-validator/blob/master/pom.xml#L72 which is probably the biggest project depending on this, maintain 1.8 compatibility.

@stevehu could perhaps chime in on the matter?

@lscoughlin
Copy link
Author

The networknt folks specifically declare dependencies on com.ethlo.time in their module info, which is the root of all this activity from me :)

@merks
Copy link

merks commented Feb 12, 2025

I'm happy to help if I can be of some assistance. Just @ mention me...

@ethlo
Copy link
Owner

ethlo commented Feb 12, 2025

I have added a new PR, #42, largely reverting all changes in 1.11.0. It would be great if you could test if it is ok or if other changes are needed @merks @lscoughlin 👍🏻

There is also a 1.12.0-SNAPSHOT available that you can test, built on this branch.

@merks
Copy link

merks commented Feb 12, 2025

It looks like there is an additional package and the uses declarations are nice:

Image

I tested building an Orbit repository locally with this version and that worked fine.

So from my point of view, this looks good.

@ethlo
Copy link
Owner

ethlo commented Feb 13, 2025

Thank you guys for the valuable feedback and learning experience this has been. Version 1.12.0 is now released: https://github.com/ethlo/itu/releases/tag/v1.12.0

@ethlo ethlo closed this as completed Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants