maven-shade-plugin
maven-shade-plugin copied to clipboard
apache
[MSHADE-400] Self-minimisation with custom entry points
- See https://issues.apache.org/jira/browse/MSHADE-400.
- Solves https://issues.apache.org/jira/browse/MSHADE-366, too.
- Contains commits from and therefore supersedes
- #83,
- #104,
- #108 (not all commits, but alternative implementation I already had before the latest commit was pushed).
To do:
- [ ] Add unit test (please help, @rmannibucau, I don't know the API for mocking well enough)
- [x] Add integration test (I have a simple one in a separate project, which I am going to push)
- [x] Rebase on master after MSHADE-391 was merged recently (merge conflicts in default shader + test).
- [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
The merge conflicts during the rebase were pretty ugly, because both in MSHADE-391 as well as here there was refactoring in the default shader. I hope I did not mess up anything. At least, locally the project compiles and some selected key tests for both issues are passing. Let us see what the CI builds say, before I add a specific integration test for self-minimisation.
Update: CI build looks good, both before and after adding my integration test.
@JanMosigItemis, would you mind if I squashed your commits (and my early one too)? The original workaround incl. unit test is no longer necessary, nor is my refactoring of it. Only a little bit of structural test refactoring is left over.
@rmannibucau, I can squash your "basic dir handling in default shader" commit too, but also try to retain it, even though Tibor is not going to like it and probably going to squash before the merge anyway. Please also be so kind as to review this PR. I think it is as far as we should go before the next major release, for which you are planning more general refactoring. Thank you.
OK, I squashed into one commit, using Co-authored-by: to set both of you as co-authors. Looks like GitHub understands it, I did it for the first time:
Rebased on master, as requested by @michael-o. Actually, after 1.5 years of code rot, this was a pain in the neck to rebase with lots of nasty conflicts. I am not even remembering exactly what I did back then and why exactly I did it the way I you see here. It was hard enough to understand the inner workings of this plugin back then, you cannot expect me to remember everything now after such a long time.
As a contributor who does not know the code base well, I would appreciate more timely responses and reviews, because repeatedly rebasing stuff is difficult and error-prone. The tests are still green, but I am not 100% sure if that means the patch is as perfect as it used to be.
@michael-o, I pushed my educated guess about what you asked me to do in a separate commit to be more conveniently reviewable. If it looks OK for you and you want me to squash and force-push once more, just let me know.
Um, sorry @michael-o, what was the rationale behind first asking me for improvements, me consequently doing them and you then not merging them but deciding for the code without the cove review improvements instead? I mean these:
Subject: [PATCH] [MSHADE-400] Code review findings
---
--- a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java (revision 1cee035ebcce0376fc1c4e726b18f7a15edb8d76)
+++ b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java (revision 131d4ae37eeb25895bdbf54a2b9acb5df2b918c4)
@@ -35,6 +35,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
+import java.nio.file.Files;
import java.util.Collections;
import java.util.Enumeration;
import java.util.HashSet;
@@ -228,14 +229,15 @@
}
try ( final BufferedReader configFileReader = new BufferedReader(
- new InputStreamReader( new FileInputStream( serviceProviderConfigFile ), UTF_8 ) ) )
+ new InputStreamReader( Files.newInputStream( serviceProviderConfigFile.toPath() ), UTF_8 ) ) )
{
// check whether the found classes use services in turn
repeatScan |= scanServiceProviderConfigFile( cp, configFileReader );
}
catch ( final IOException e )
{
- log.warn( e.getMessage() );
+ log.warn( "Failed to scan service provider config file " + serviceProviderConfigFile );
+ log.debug( e );
}
}
return repeatScan;
@@ -269,7 +271,8 @@
}
catch ( final IOException e )
{
- log.warn( e.getMessage() );
+ log.warn( "Failed to scan service provider config file " + jarEntry + " in jar " + jar.getName() );
+ log.debug( e );
}
}
}
--- a/src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java (revision 1cee035ebcce0376fc1c4e726b18f7a15edb8d76)
+++ b/src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java (revision 131d4ae37eeb25895bdbf54a2b9acb5df2b918c4)
@@ -996,8 +996,7 @@
{
entryPoints = new HashSet<>();
}
- getLog().info( "Minimizing jar " + project.getArtifact()
- + ( entryPoints.isEmpty() ? "" : ", entry points = " + entryPoints ) );
+ getLog().info( "Minimizing jar " + project.getArtifact() );
try
{
Um, sorry @michael-o, what was the rationale behind first asking me for improvements, me consequently doing them and you then not merging them but deciding for the code without the cove review improvements instead? I mean these:
Subject: [PATCH] [MSHADE-400] Code review findings --- --- a/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java (revision 1cee035ebcce0376fc1c4e726b18f7a15edb8d76) +++ b/src/main/java/org/apache/maven/plugins/shade/filter/MinijarFilter.java (revision 131d4ae37eeb25895bdbf54a2b9acb5df2b918c4) @@ -35,6 +35,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.nio.file.Files; import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; @@ -228,14 +229,15 @@ } try ( final BufferedReader configFileReader = new BufferedReader( - new InputStreamReader( new FileInputStream( serviceProviderConfigFile ), UTF_8 ) ) ) + new InputStreamReader( Files.newInputStream( serviceProviderConfigFile.toPath() ), UTF_8 ) ) ) { // check whether the found classes use services in turn repeatScan |= scanServiceProviderConfigFile( cp, configFileReader ); } catch ( final IOException e ) { - log.warn( e.getMessage() ); + log.warn( "Failed to scan service provider config file " + serviceProviderConfigFile ); + log.debug( e ); } } return repeatScan; @@ -269,7 +271,8 @@ } catch ( final IOException e ) { - log.warn( e.getMessage() ); + log.warn( "Failed to scan service provider config file " + jarEntry + " in jar " + jar.getName() ); + log.debug( e ); } } } --- a/src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java (revision 1cee035ebcce0376fc1c4e726b18f7a15edb8d76) +++ b/src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java (revision 131d4ae37eeb25895bdbf54a2b9acb5df2b918c4) @@ -996,8 +996,7 @@ { entryPoints = new HashSet<>(); } - getLog().info( "Minimizing jar " + project.getArtifact() - + ( entryPoints.isEmpty() ? "" : ", entry points = " + entryPoints ) ); + getLog().info( "Minimizing jar " + project.getArtifact() ); try {
It was faster to do it myself at the end than explaning how I expect it to be. For improvement of consistency followup PRs can be created any time.
I won't create a new PR after having the work you explicitly asked me to do here already. Too much micro-management. Besides, I did the work before you committed. Why didn't you just commit on top of my PR? Please take my changes and commit them yourself. My branch still exists, you can cherry-pick the commit. Thank you.