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

PROTON-2158 Adding automatic module name entry. #37

Closed
wants to merge 2 commits into from

Conversation

conniey
Copy link
Contributor

@conniey conniey commented Mar 19, 2020

  • Adds an Automatic-Module-Name to the MANIFEST.MF so it can be consumed by JDK 9+ libraries without warnings.
    • Item added in manifest: Automatic-Module-Name: org.apache.qpid.proton-j
    • The module name was chosen by looking at the value of "Bundle-SymbolicName".
  • Fixes issue: https://issues.apache.org/jira/browse/PROTON-2158

@JonathanGiles
Copy link

@gemmellr Who is best placed to review this and consider merging it? Thanks.

@gemmellr
Copy link
Member

For future, please add the JIRA key reference to the PR title and commit log as that way the JIRA gets updated when PRs are opened, commented on, commits are pushed, etc. See prior PRs or commits for examples.

@gemmellr Who is best placed to review this and consider merging it? Thanks.

Someone not off on holiday or sick, or more annoyingly both together as happened :) I'll try to look soon.

@conniey conniey changed the title Adding automatic module name entry. PROTON-2158 Adding automatic module name entry. Mar 26, 2020
@conniey
Copy link
Contributor Author

conniey commented Mar 26, 2020

For future, please add the JIRA key reference to the PR title and commit log as that way the JIRA

Thanks for letting me know. :) I'll do that in the future.

@gemmellr
Copy link
Member

I had a look, and the module name addition itself seems fine, but I want to look into some odd behaviour around other manifest entries that get added after the change (yet on other pre-existing config seem like they should have been added previously) as well before putting the change in.

Can I also ask why you did the override on the parent managed jar plugin version? Its obviously a little newer, but was there a specific reason to need to use it? If not I'd perhaps suggest removing that and just sticking to the managed version for simplicity and consistency.

@conniey
Copy link
Contributor Author

conniey commented Mar 30, 2020

Can I also ask why you did the override on the parent managed jar plugin version?

No reason to use this version. I removed it.

I want to look into some odd behaviour around other manifest entries that get added after the change

I'm not sure why they didn't show up when built from master, but the entries appear in previous versions of proton-j. :( I only know enough about maven-bundle-plugin to execute bundle:bundle. Looking at the manifest instructions, it looks like we did expect all those Export-Package entries.
https://github.com/apache/qpid-proton-j/blob/master/pom.xml#L100

@gemmellr
Copy link
Member

It isnt the bundle plugin package additions, those are indeed expected, but rather it was some other additions about implementation and specification that dont seem to have been there before but are afterwards, and yet given existing parent config seem like perhaps they should have been before also. I was off yesterday and so still need to look into this.

@asfgit asfgit closed this in 20c9943 Mar 31, 2020
@gemmellr
Copy link
Member

I've pushed your changes in 20c9943 and added a further change in 02998b3 to address the other entries being added.

(The Apache parent pom has dependencyManagement with configuration for the maven jar plugin, which isnt picked up by the bundle plugin when used due to 'bundle' packaging type... unless also adding an explicit jar plugin config as your change did, even though the bundle plugin is still used instead of the jar plugin. Odd.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants