jakartaee / jakartaee-api

jakartaee-api
Other
39 stars 42 forks source link

Regression: EE 10 Platform API JARs include all classes, and transitive dependencies to Spec API JARs leading to duplicate classes and JPMS split package failures and OSGi issues #133

Closed lprimak closed 1 year ago

lprimak commented 1 year ago

Describe the bug This is a regression from Jakarta EE 8 (possibly 9)

Platform API JARs now include both all the API .class files as well as all Spec JARs as transitive dependencies This is seen below. The leads to duplicate classes on the classpath and problems if an application uses has JPMS module-info anywhere (split package issues) This also leads to OSGi deployment issues.

% mvnd dependency:tree
[INFO] --- maven-dependency-plugin:3.4.0:tree (default-cli) @ hope-website ---
[INFO] +- jakarta.platform:jakarta.jakartaee-api:jar:10.0.0:provided
[INFO] |  +- jakarta.platform:jakarta.jakartaee-web-api:jar:10.0.0:provided
[INFO] |  |  +- jakarta.servlet:jakarta.servlet-api:jar:6.0.0:provided
[INFO] |  |  +- jakarta.servlet.jsp:jakarta.servlet.jsp-api:jar:3.1.0:provided
[INFO] |  |  +- jakarta.el:jakarta.el-api:jar:5.0.1:provided
[INFO] |  |  +- jakarta.servlet.jsp.jstl:jakarta.servlet.jsp.jstl-api:jar:3.0.0:provided
[INFO] |  |  +- jakarta.faces:jakarta.faces-api:jar:4.0.1:provided
[INFO] |  |  +- jakarta.websocket:jakarta.websocket-api:jar:2.1.0:provided
[INFO] |  |  +- jakarta.websocket:jakarta.websocket-client-api:jar:2.1.0:provided
[INFO] |  |  +- jakarta.ejb:jakarta.ejb-api:jar:4.0.1:provided
[INFO] |  |  +- jakarta.transaction:jakarta.transaction-api:jar:2.0.1:provided
[INFO] |  |  +- jakarta.persistence:jakarta.persistence-api:jar:3.1.0:provided
[INFO] |  |  +- jakarta.validation:jakarta.validation-api:jar:3.0.2:provided
[INFO] |  |  +- jakarta.interceptor:jakarta.interceptor-api:jar:2.1.0:provided
[INFO] |  |  +- jakarta.enterprise:jakarta.enterprise.lang-model:jar:4.0.1:provided
[INFO] |  |  +- jakarta.inject:jakarta.inject-api:jar:2.0.1:provided
[INFO] |  |  +- jakarta.authentication:jakarta.authentication-api:jar:3.0.0:provided
[INFO] |  |  \- jakarta.security.enterprise:jakarta.security.enterprise-api:jar:3.0.0:provided
[INFO] |  +- jakarta.jms:jakarta.jms-api:jar:3.1.0:provided
[INFO] |  +- jakarta.activation:jakarta.activation-api:jar:1.2.2:provided
[INFO] |  +- jakarta.mail:jakarta.mail-api:jar:2.1.0:provided
[INFO] |  +- jakarta.resource:jakarta.resource-api:jar:2.1.0:provided
[INFO] |  +- jakarta.authorization:jakarta.authorization-api:jar:2.1.0:provided
[INFO] |  \- jakarta.batch:jakarta.batch-api:jar:2.1.1:provided

To Reproduce Steps to reproduce the behavior:

        <dependency>
            <groupId>jakarta.platform</groupId>
            <artifactId>jakarta.jakartaee-api</artifactId>
            <version>10.0.0</version>
            <scope>provided</scope>
        </dependency>

Workaround The issue can be worked around by using it as pom dependency to exclude the platform JAR file from the classpath:

        <dependency>
            <groupId>jakarta.platform</groupId>
            <artifactId>jakarta.jakartaee-api</artifactId>
            <version>10.0.0</version>
            <type>pom</type>
            <scope>provided</scope>
        </dependency>

This is the offending commit: 8ff41dcbe544ae74c286c3342ed46e7e3024b3a9 / #127 Related to #125

Possible Solution Provide an empty platform JAR file instead of including all Spec .class files in it. Spec JARs as transitive dependencies will make the classpath correct and will work perfectly.

lprimak commented 1 year ago

@starksm64 @ivargrimstad Do you remember what was the reason for this change? It seems to be somewhat-unrelated to the PR it's in...thanks!

ivargrimstad commented 1 year ago

The reason is to explicitly state the top-level dependencies, and also make sure that the JavaDoc for these is generated for the Platform JavaDoc. See the mail thread here: https://www.eclipse.org/lists/jakartaee-platform-dev/msg03436.html

There may be other ways of achieving this, so feel free to suggest a PR. But also keep in mind that these API artifacts are not normative deliverables of the specifications, so it may not have the highest priority.

lprimak commented 1 year ago

Thanks Ivar,

perhaps the way to go is to have "empty" JARs in jakarta.platform (full/web), that way there are no duplicates and the "new" way forward with transitive dependencies would work

OndroMih commented 1 year ago

I agree that platform API JARs shouldn't contain anything that is already in spec API JARs brought as dependencies. So, in the end, they should be more or less empty and just an aggregator for all related spec API JARs. That's how usually Maven dependencies work and it works well.

lprimak commented 1 year ago

More thoughts: Platform APIs should be of type pom instead of jar so they just bring in the appropriate spec JARs as transitive dependencies. I believe this is how MicroProfile does it as well. This would also fix https://github.com/eclipse/microprofile/issues/297 This would remove the shade legacy altogether and would be a better overall

OndroMih commented 1 year ago

How would POM type help? I really don't like that MP platform API is a POM type, the type always has to be specified in the dependency in pom.xml. The JAR type is implicit, so it doesn't have to be explicitly specified. If the umbrella artifact is an empty JAR, it should be enough, am I wrong?

lprimak commented 1 year ago

How would POM type help?

Just for it not to have a JAR at all, as an empty JAR may (or may not!) cause confusion. However, I do see your point about the JAR being default and either way works for me just the same.

lprimak commented 1 year ago

@edburns What do you think? Should I submit a PR? Should be a simple matter

lprimak commented 1 year ago

@edburns @OndroMih @ivargrimstad I have changed the description of the ticket to simplify and align with the current problem status and ways to fix it.

Thank you

starksm64 commented 1 year ago

It seems to be an unexpected side effect of trying to clean up the dependency graph of the profile/platform jars. As @ivargrimstad says we are not using these jars as normative artifacts that are validated in any TCK, but it might as well be fixed.

lprimak commented 1 year ago

@starksm64 So do you agree with the approach to ship empty JARs for umbrella (platform and web profile) JARs? Or do you have something else in mind?

JanWesterkamp-iJUG commented 1 year ago

@lprimak, I agree that the umbralla specs should not add APIs, they should consist of the components specs API only, as provided specs.

I am unsure I understand what is your exact solution for the problem - mark the umbrella spec as provided or component specs as optional (with the last, I would disagree)? Or package the umbrella spec dependency as type POM? This may indeed bring Maven to not adding/creating a JAR file consting the compoent spec API artefacts JARs...

Your linked commit is refering to the Jakarta Core Profile API - which is not a real subset of Jakarta Web Profile API (only an equal subset) at the moment - personally, I would like to change that, but this is under discussion... A change in the Core Profile API can not have a direct impact on the Web or Full Profile aka Platform - but of course when adding both it can lead to dublicates too, if the implementations dependency management does not remove doublets.

I think it is very important to understand the system environment behaviour with modularisation technics like JPMS, Maven, OSGi here to solve this.

May be you can give some details about the environment you used to reproduce and test potential solutions with it?

lprimak commented 1 year ago

@JanWesterkamp-iJUG My solution is simple: Ship empty JARs for all three umbrella JARs: jakarta.jakartaee-api-10.0.0.jar , jakarta.jakartaee-web-api-10.0.0.jar , and jakarta.jakartaee-core-api-10.0.0.jar

By 'empty' I mean a JAR that contains no files at all.

This way, all classes will be loaded from the transitive dependencies and no duplicates will be loaded.

OndroMih commented 1 year ago

@JanWesterkamp-iJUG My solution is simple: Ship empty JARs for all three umbrella JARs: jakarta.jakartaee-api-10.0.0.jar , jakarta.jakartaee-web-api-10.0.0.jar , and jakarta.jakartaee-core-api-10.0.0.jar

By 'empty' I mean a JAR that contains no files at all.

This way, all classes will be loaded from the transitive dependencies and no duplicates will be loaded.

I fully support this.

This will also solve the problem when adding multiple profiles as dependencies. If they have the same version, they would pull the same dependencies without duplicates.

JanWesterkamp-iJUG commented 1 year ago

@OndroMih and I think this should work even when there are different versions in the profiles (in Maven, then the order and depth defines which version is choosen), but having the Core Profile to be a true subset of the Web Profile (as this is a true subset of the Platform/Full Profile) can solve this too - my favorite solution ;-)

@lprimak so which options we have to achieve this (in Maven, so it is fixed in JPMS and OSGi too)? Make the component specs optional in umbrella specs might solve this, but creates other issues... Can you list the potential options please, as you did some experiments already?

I would like to add this to the CN4J call preparation slides then. A new version is available here: https://github.com/eclipse-ee4j/jakartaee-platform/issues/607).

lprimak commented 1 year ago

I think there is only one viable option, which is the empty JAR option outlines above.

The more I think about the other possibilities the more problems I see with them

lprimak commented 1 year ago

@starksm64 ping :) Any feedback is appreciate, thank you!

starksm64 commented 1 year ago

Yes, let's move ahead with the empty jar approach.

lprimak commented 1 year ago

I think this is important enough to release 10.0.1 IMHO @ivargrimstad

lprimak commented 1 year ago

@OndroMih what do you think of the fix?

lprimak commented 1 year ago

As a "side-effect" of the fix of this issue, it'll be much easier to update individual Specs on ad-hoc basis, as noted in the Jakarta EE Contribution Guide @m-reza-rahman