jakartaee / inject

Apache License 2.0
17 stars 17 forks source link

add jpms module info #19

Closed h908714124 closed 3 years ago

h908714124 commented 3 years ago

Suggestion: Bump java version to 11, and add module-info to sources root. This is one possible way to resolve #17.

h908714124 commented 3 years ago

Wow legal checks. FOSS sure is fun these days.

h908714124 commented 3 years ago

@manovotn jpms is needed to use jpackage, to create custom runtime images. What's the alternative? Fat jars? Shade plugin?

A248 commented 3 years ago

I don't see any good reason to delay adding a module descriptor. Whether or not you like JPMS, it's here to stay, and it's best that projects provide compatibility with it, especially as adoption increases.

I am not sure we want to add these. There needs to be an agreement on jakarta level about it before proceeding (i.e. all specs should have it or none for certain ee release).

I don't quite follow your reasoning here. From a specification standpoint and for the purposes of declaring dependencies, what matters is the declared module name.

Adding a module descriptor is more akin to a feature enhancement with no effect on the API specification. The descriptor states qualities of an API which were already the case pre-modularization. For example, "internal" packages were never part of the API, and thus when modularization occurs, they're simply not exported. This may change runtime behavior, as JPMS enforces this encapsulation, but no changes to the specification took place, strictly speaking.

manovotn commented 3 years ago

My personal reason is irrelevant, like I said.

What I'd like to see prior to accepting this is an agreement that next EE version will have this for all artifacts/specs. Besides, I am not the only one who can review and merge PRs. So an opinion of some other commiter would be welcome. You could also start a discussion on the mailing list which might draw attention of more people.

overheadhunter commented 3 years ago

GitHub drew my attention as well 😉

While I personally (again - irrelevant) would love to see this PR, we should keep in mind that Java 8 compatibility is requested elsewhere (see discussion in #18). It is my understanding that the debate covers both 1.x as well as 2.x jars. If this is correct, the only option to add a module-info.java would be a multi-release-jar.


There needs to be an agreement on jakarta level about it before proceeding (i.e. all specs should have it or none for certain ee release).

jakarta.annotation and jakarta.ws.rs have a module-info btw

Emily-Jiang commented 3 years ago

I think this change will need to be added to the project release plan if it has not already. At the moment, on the platform calls, we are discussing the strategies towards bumping compile java version (would this require a major or minor release?). As for jpms, it is not required as yet but component specs can add it if they know what they are doing. I think we should wait to merge this PR till platform spec team agreed on a policy towards the java compile version. Is there a compiling reason to support jpms right now?

h908714124 commented 3 years ago

@Emily-Jiang I think jpackage requiring it is a compelling reason to use jpms right now. Many things depend on @Inject, including dagger which is foundational itself. Well, actually dagger depends on javax.annotation, but that project is inactive. There are open dagger tickets to add jpms support, and to support jakarta annotations. As it stands, one cannot be achieved without the other. But even then, the former ticket will only be resolved when jakarta also adds a module descriptor.

starksm64 commented 3 years ago

It is the next topic that the Jakarta platform group needs to address, so we will update the api jar once that is concluded.

msgilligan commented 3 years ago

@h908714124 thanks for making the PR. My recommendation would be to set source/target Java version to 9. Although most people will probably want to use 11/LTS, there's really no advantage to making this JAR require Java 11.

h908714124 commented 3 years ago

Hey @msgilligan, thanks for the suggestion. Good point. However this PR can't be merged due to legal constraints, so I guess it's not even worth the effort.

dwhitla commented 3 years ago

I am not sure we want to add these. There needs to be an agreement on jakarta level about it before proceeding (i.e. all specs should have it or none for certain ee release).

Personally I am -1 because I simply consider JPMS a grand leap in wrong direction but that's mostly irrelevant personal opinion here :-)

Honestly - what people may want based on their opinion of the direction taken by Oracle when releasing jigsaw is irrelevant. Right now the whole JavaEE (JakartaEE) ecosystem is broken for people forced to use Java11+. This costs almost nothing to implement but spares application developers budget-breaking amounts of time working around its absence using hacks like moditect. I just can't express how damned frustrated I am as a 22 year Java veteran with how JPMS is being handled by people who are supposed to be leading the community. It just breaks my brain how people are happy to let the language die because Oracle had the hide to try and fix a very real problem with the JVM but not use the existing poorly designed module systems. Java9 shipped almost 5 years ago - it happened - can we just move forward please.

dwhitla commented 3 years ago

While I personally (again - irrelevant) would love to see this PR, we should keep in mind that Java 8 compatibility is requested elsewhere (see discussion in #18). It is my understanding that the debate covers both 1.x as well as 2.x jars. If this is correct, the only option to add a module-info.java would be a multi-release-jar.

This is not correct. Java8 and earlier ignore the module-info.class. The only requirement for Java8 compatibility is that the class version be no later than Java8. Shipping a multi-release jar would certainly allow it but is not a prerequisite unless you want multiple versions of a class which utilise language features not present in all JVM releases.

dwhitla commented 3 years ago

I think jpackage requiring it is a compelling reason to use jpms right now.

Not to mention jlink, jdeps and GraalVM native image creation. And lets not leave out JavaFX which requires Java11+ and strongly pushes developers to Java16 in order to address bugs in earlier releases. The newer Java based technologies that keep Java competitive as a development platform and relevant as a language in many businesses are being hamstrung by what used to be Java's greatest strength - the OSS libraries. When Spring, JBoss and even the bloody EE reference implementation refuse to adopt a language specification it pretty much kills Java as a language for many of my clients. And I'm too old, too invested in Java and too angry to learn .Net just to stay employed.

dwhitla commented 3 years ago

Can someone please elaborate on the nature of the legal hold up here?

rbygrave commented 3 years ago

@dwhitla I believe the hold up is that @h908714124 has not signed the Eclipse ECA agreement. Is that correct @h908714124 ?

@h908714124 are you ok with me re-submitting your PR or would you be up to signing the eclipse eca agreement?

I would love to see this PR merged so I have signed the eca (at least I believe I have) and I am happy to re-submit this PR and see if we can get that merged. Any objections?

If we desire I am also happy to do an initial PR that stays with Java8 and probably uses moditech plugin to add the module-info (as that's what I've been doing myself elsewhere). We would then get the maintainers to release that before we then merge this second PR which bumps to Java 9 and get the maintainers to release again a second time.

Are people on board with this? Do we want the initial Java 8 release as well as the Java 9 one?

rbygrave commented 3 years ago

FYI:

h908714124 commented 3 years ago

Hey @rbygrave, how are you doing buddy? If you have already signed the ECA, then please go ahead and resubmit this PR. Thanks

h908714124 commented 3 years ago

This PR is replaced by 21