-
Notifications
You must be signed in to change notification settings - Fork 43
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
Normalize core profile dependencies in web profile #139
Normalize core profile dependencies in web profile #139
Conversation
f624654
to
10c5894
Compare
@Emily-Jiang This is the PR that you may have concerns with. |
I did a quick troll through the core profile pom.xml and you were right that the CDI dependency did not have exclusion. However, I have concerns with the core profile without any exclusion for CDI as in MicroProfile, we specified exclusion deliberately. See here. @starksm64 I thought the EL and EJB dependencies were excluded from Core Profile. Why was it not the case? |
Emily, please see jakartaee-api/jakartaee-core-api/pom.xml Line 111 in 10c5894
In core profile, cdi-api excludes all dependencies, so I think what you are saying is correct. Both in current core API and after this PR is merged. |
Thanks for looking into this Emily! |
The exclusion was at the spec level and could not be done at the api jar level due to the fact that we were rebuilding the jar from source. With the move to using empty jars these can be excluded. |
a6bc809
to
a8e27ce
Compare
See also jakartaee/cdi#640 |
I want to make sure that jakartaee/cdi#640 is only tangentially related to this one, there are no dependencies between them, and certainly, this one is (or should) not be controversial. This simply fixes a maven hierarchy error that doesn't make Jakarta EE currently play well with MicroProfile. |
jakartaee-bom/pom.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two findings here:
The BOM misses a separate Core Profile list of dependencies, that should be defined first before the Web Profile ones, so they can be managed easier.
We should get rid of this plugin here, may be in a separate PR:
<plugins>
<plugin>
<groupId>org.glassfish.build</groupId>
<artifactId>glassfishbuild-maven-plugin</artifactId>
<executions>
<execution>
<id>generate-pom</id>
<goals>
<goal>generate-pom</goal>
</goals>
</execution>
<execution>
<id>attach-all-artifacts</id>
<goals>
<goal>attach-all-artifacts</goal>
</goals>
</execution>
</executions>
</plugin>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, great idea. Also probably not part of this particular PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JanWesterkamp-iJUG If you want to create a separate issue for that, I can volunteer to create a PR there too
a8e27ce
to
572a4f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be we can change the comment reflecting this behaviour as explanation.
572a4f6
to
8f3db64
Compare
@starksm64 Would love your feedback here... thank you! |
Thank you @Emily-Jiang ! |
Thank you Scott! |
@JanWesterkamp-iJUG ping please :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lprimak I added some comments - sorry for the delay.
jakartaee-web-api/pom.xml
Outdated
<artifactId>*</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this should become a required part in the Core (or Web Profile) in a future release (Jakarta EE 11) because XML is still in heavy use and is also required for some configurations (like Persistence and optionally in Servlet) - but that's a separate issue.
Good to fix it here now! But is the exclusion required - I don't think so. But I can verify this with a jQA run, to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jakarta XML Binding got dependency to Jakarta Activation 2.1.0 - this need to be maintained. So it looks like we need this exclusion until we have that solved first unfortunately.
@JanWesterkamp-iJUG I granted you write permissions for this PR so you can try some language or you can comment here |
Also, this PR has been open for 3 months with minimal changes and no issues. Let's merge this in and then we can work on the other, very valid, issues. Thank you @edburns and @JanWesterkamp-iJUG ! |
adbf10f
to
8444abb
Compare
@edburns done thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As dicussed in the Jakarta Platform call on 2023-08-22 the remaining chages will be handled in separate PRs - so this on could be merged.
Meanwhile #140 was merged before, but this one is rebased and remaining issues in #140 can be done after merging this on first.
@edburns when you agree on this, I will do this merge then.
Summary
Conclusion of #133 Bug fix train
Makes Core profile a true dependency of Web profile and plays nice with MicroProfile POM
fixes #138
Goals
jakartaee-api
andjakartaee-web-api
depend onjakartaee-core-api
to be consistent with maven resolution rules, and thus make it intuitive to application both app devs and MicroProfile.*-api.cp.version
properties, as they are now overridable with 'regular' maven pom directives, and thus no longer necessary.jakartaee-web-api
that are already injakartaee-core-api
(this can now be done safely because empty JARs are now shipped.<optional>
tags from POMsNon-Goals