Skip to content

[MJAR-260] Fail on invalid automatic module name #3

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 1, 2018
Merged

[MJAR-260] Fail on invalid automatic module name #3

merged 1 commit into from
Dec 1, 2018

Conversation

sormuras
Copy link
Contributor

@sormuras sormuras self-assigned this Nov 16, 2018
eolivelli
eolivelli requested changes Nov 16, 2018
View reviewed changes
Copy link

@eolivelli eolivelli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like this 'feature'

@sormuras sormuras force-pushed the MJAR-260-Automatic-Module-Name branch from 6cd74eb to 736e16c Compare November 16, 2018 15:23
eolivelli
eolivelli approved these changes Nov 16, 2018
View reviewed changes
Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

Awesome!

@eolivelli
Copy link

Seems there is a failure
https://builds.apache.org/job/maven-box/job/maven-jar-plugin/job/MJAR-260-Automatic-Module-Name/2/

@eolivelli
Copy link

I wonder if we should fix other plugins, namely the shade plugin

@sormuras
Copy link
Contributor Author

sormuras commented Nov 16, 2018 •
edited
Loading

Seems there is a failure
https://builds.apache.org/job/maven-box/job/maven-jar-plugin/job/MJAR-260-Automatic-Module-Name/2/

Wrong source file encoding used for javac? Locally, all tests and ITs are green.

I wonder if we should fix other plugins, namely the shade plugin

Sure. Here it is more relevant to exclude a /module-info.class from shading.

@sormuras sormuras force-pushed the MJAR-260-Automatic-Module-Name branch 2 times, most recently from 51d4303 to 16c0d21 Compare November 16, 2018 17:43
@sormuras
Copy link
Contributor Author

Seems there is a failure
https://builds.apache.org/job/maven-box/job/maven-jar-plugin/job/MJAR-260-Automatic-Module-Name/2/

Wrong source file encoding used for javac? Locally, all tests and ITs are green.

No, the real reason was the _ (underscore) which is not invalid on Java 7. Removed the check as it is not relevant in this context. Force-pushed changes to this branch.

@sormuras
Copy link
Contributor Author

Build No 4 is green (read: blue). Waiting for @khmarbaise or @rfscholte to chime in.

https://builds.apache.org/job/maven-box/job/maven-jar-plugin/job/MJAR-260-Automatic-Module-Name/

@sormuras sormuras removed the request for review from rfscholte November 16, 2018 19:18
@sormuras sormuras force-pushed the MJAR-260-Automatic-Module-Name branch from 16c0d21 to 617f54c Compare November 19, 2018 17:21
@sormuras
[MJAR-260] Upgrade to Archiver 3.3.0 and add ITs
aff0671 
This PR updates the dependency to the version `3.3.0` of the Maven
Archiver and introduces two integration tests that a) verify that
JAR containing a valid name is created and b) that no JAR file is
created when it would contain an invalid automatic module name.

Addresses https://issues.apache.org/jira/browse/MJAR-260
@sormuras sormuras force-pushed the MJAR-260-Automatic-Module-Name branch from 617f54c to aff0671 Compare November 24, 2018 22:42
@sormuras sormuras merged commit b59d531 into master Dec 1, 2018
@sormuras sormuras deleted the MJAR-260-Automatic-Module-Name branch December 1, 2018 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khmarbaise khmarbaise khmarbaise left review comments

@eolivelli eolivelli eolivelli approved these changes

Assignees

@sormuras sormuras

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sormuras @eolivelli @khmarbaise