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

Normalize core profile dependencies in web profile #139

Merged

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented May 17, 2023

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

  • Make jakartaee-api and jakartaee-web-api depend on jakartaee-core-api to be consistent with maven resolution rules, and thus make it intuitive to application both app devs and MicroProfile.
  • Remove *-api.cp.version properties, as they are now overridable with 'regular' maven pom directives, and thus no longer necessary.
  • Remove duplicate dependencies from jakartaee-web-api that are already in jakartaee-core-api (this can now be done safely because empty JARs are now shipped.
  • Make the minimum changes necessary to accomplish the goal.
  • Make this suitable as a patch release for Jakarta EE 10 (ex: 10.0.1)
  • Remove unnecessary <optional> tags from POMs
  • Preserve generation of javadoc as it is now (jakarta-api includes platrorm, web and core javadocs, jakartaee-web-api includes web and core javadocs, etc)
  • Make javadoc generation errors fail the build. Otherwise, they fail silently generating invalid javadocs (possibly leading to invalid releases)

Non-Goals

  • Not intended as a complete refactoring of the POM files

@lprimak lprimak force-pushed the normalize-core-profile branch 2 times, most recently from f624654 to 10c5894 Compare May 17, 2023 02:46
@lprimak lprimak changed the title Normalize core profile Normalize core profile dependencies May 17, 2023
@lprimak
Copy link
Contributor Author

lprimak commented May 23, 2023

@Emily-Jiang This is the PR that you may have concerns with.
Thank you!

@lprimak lprimak changed the title Normalize core profile dependencies Normalize core profile dependencies in web profile May 23, 2023
@Emily-Jiang
Copy link

@Emily-Jiang This is the PR that you have concerns with. Thank you!

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?

@lprimak
Copy link
Contributor Author

lprimak commented May 23, 2023

Emily, please see

<groupId>jakarta.enterprise</groupId>

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.

@lprimak
Copy link
Contributor Author

lprimak commented May 23, 2023

Thanks for looking into this Emily!
Also, just FYI the reason I started looking into this is that Jakarta EE APIs as they currently are not playing nice with MicoProfile maven dependencies. After these PRs are merged, both Jakarta EE and MP will play much nicer with each other!

@starksm64
Copy link
Contributor

@Emily-Jiang This is the PR that you have concerns with. Thank you!

@starksm64 I thought the EL and EJB dependencies were excluded from Core Profile. Why was it not the case?

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.

jakartaee-api/pom.xml Outdated Show resolved Hide resolved
@arjantijms
Copy link
Contributor

See also jakartaee/cdi#640

@lprimak lprimak requested a review from edburns June 12, 2023 17:30
@lprimak
Copy link
Contributor Author

lprimak commented Jun 12, 2023

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.
I view this issue as a simple bug fix, that doesn't break anything, nor introduce any design or architecture changes.

This simply fixes a maven hierarchy error that doesn't make Jakarta EE currently play well with MicroProfile.
After this is merged, MP and Jakarta EE can be mixed better and will play nice together without overriding each other's dependencies or requiring exclusions or inclusions of multiple already-included Jakarta EE APIs

Copy link
Contributor

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>

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor

@JanWesterkamp-iJUG JanWesterkamp-iJUG left a 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.

jakartaee-web-api/pom.xml Outdated Show resolved Hide resolved
@lprimak
Copy link
Contributor Author

lprimak commented Jun 20, 2023

@starksm64 Would love your feedback here... thank you!

pom.xml Show resolved Hide resolved
@lprimak lprimak requested a review from Emily-Jiang July 8, 2023 07:15
@lprimak
Copy link
Contributor Author

lprimak commented Jul 11, 2023

Thank you @Emily-Jiang !
Still waiting for feedback from @JanWesterkamp-iJUG and @starksm64

@lprimak
Copy link
Contributor Author

lprimak commented Jul 19, 2023

Thank you Scott!
@JanWesterkamp-iJUG you are the last one :)

@lprimak
Copy link
Contributor Author

lprimak commented Aug 11, 2023

@JanWesterkamp-iJUG ping please :)

Copy link
Contributor

@JanWesterkamp-iJUG JanWesterkamp-iJUG left a 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-api/pom.xml Show resolved Hide resolved
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>

Copy link
Contributor

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.

Copy link
Contributor

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.

@lprimak
Copy link
Contributor Author

lprimak commented Aug 17, 2023

@JanWesterkamp-iJUG I granted you write permissions for this PR so you can try some language or you can comment here

@lprimak
Copy link
Contributor Author

lprimak commented Aug 17, 2023

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 !

@edburns
Copy link
Contributor

edburns commented Aug 22, 2023

Hello @lprimak , I have merged #140 from @lukasj . Can you please revise this PR to take this fact into account?

@lprimak
Copy link
Contributor Author

lprimak commented Aug 23, 2023

@edburns done thank you

Copy link
Contributor

@JanWesterkamp-iJUG JanWesterkamp-iJUG left a 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.

@JanWesterkamp-iJUG
Copy link
Contributor

@edburns thanks for your approval!
I will merge this now.
Further (required) updates will be done is a separate PR, as updating comments or potential updates to optional dependency handling (some are updated in #140).

@JanWesterkamp-iJUG JanWesterkamp-iJUG merged commit 8ec1c8b into jakartaee:master Aug 29, 2023
@lprimak lprimak deleted the normalize-core-profile branch August 29, 2023 15:00
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

Successfully merging this pull request may close these issues.

Make Full / Web profile play nice with MicroProfile maven dependencies
6 participants