Skip to content

CSV-292: Add Automatic-Module-Name to JAR file#191

Closed
rvesse wants to merge 1 commit into
apache:masterfrom
rvesse:CSV-292
Closed

CSV-292: Add Automatic-Module-Name to JAR file#191
rvesse wants to merge 1 commit into
apache:masterfrom
rvesse:CSV-292

Conversation

@rvesse

@rvesse rvesse commented Oct 25, 2021

Copy link
Copy Markdown
Member

Ensure that the resulting Commons CSV JAR file can be used in JPMS based
projects

Prior to this change the JAR is missing the Automatic-Module-Name entry in its
manifest meaning it cannot be used in JPMS based projects (e.g. rvesse/airline#106) yielding errors like the following:

Error occurred during initialization of boot layer
java.lang.module.FindException: Module org.apache.commons.csv not found, required by com.github.rvesse.airline.examples

With this change JPMS based projects can successfully use Commons CSV without errors.

@kinow kinow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts in the pom.xml file. But looks good to me other than that.

@rvesse

rvesse commented Oct 25, 2021

Copy link
Copy Markdown
Member Author

@kinow I'm a little confused as to why there's a conflict. Files Changed view shows a clean additive diff so not sure why GitHub seems to think the entire file is conflicted

I will try a rebase tomorrow and see if that cleans things up

@kinow

kinow commented Oct 25, 2021

Copy link
Copy Markdown
Member

@rvesse I took the liberty to rebase and push to your branch, hope that's OK. Take a look to confirm your change is still OK, please.

The conflicts were created in the post-release commits to master. I think GitHub UI only shows that there are conflicts, but the diff view only shows the difference between your change and the old commit in master (where your change could be applied cleanly with no conflicts). A bit confusing I think.

@kinow kinow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed mvn package on this branch produces the module name (which I assume is the desired name for commons csv):

image

Thanks!

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 98.323% when pulling 616c7fa on rvesse:CSV-292 into a4d15bc on apache:master.

This allows for using Commons CSV in JPMS based projects that want to
declare a requirement on the org.apache.commons.csv module
@rvesse

rvesse commented Dec 15, 2021

Copy link
Copy Markdown
Member Author

@kinow What needs to happen for this to get merged?

@kinow

kinow commented Dec 15, 2021

Copy link
Copy Markdown
Member

@kinow What needs to happen for this to get merged?

Ops, missed this PR, sorry @rvesse ! LGTM

@kinow kinow closed this in dbecd03 Dec 15, 2021
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.

3 participants