-
Notifications
You must be signed in to change notification settings - Fork 88
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
[MSHADE-400] Self-minimisation with custom entry points #110
Conversation
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. |
Do it. |
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. |
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
Outdated
Show resolved
Hide resolved
Just those two and I will merge. |
@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. |
Also add self-minimisation + SPI services IT. Remove MinijarFilterTest.removeServicesShouldIgnoreDirectories, because now removing services precisely does **not** ignore directories anymore. Co-authored-by: Romain Manni-Bucau <[email protected]> Co-authored-by: Jan Mosig <[email protected]> This closes apache#110
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. |
To do: