You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:06:07 UTC
[GitHub] [maven-shade-plugin] Reamer opened a new pull request #100: [MSHADE-396] Improve SourceContent Shading
Reamer opened a new pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100
This PR tries to improve the shading of the source content. It adds some more exclusions which are explained in the source code.
I use the generated source code in [apache/zeppelin](https://github.com/apache/zeppelin/) as a workaround for a missing intellij IDE feature [IDEA-93855 ](https://youtrack.jetbrains.com/issue/).
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] kriegaex edited a comment on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-876058443
Sorry for the late feedback. I just checked out this PR and ran it against my shading configuration in AspectJ, then diffed the shaded source archives before and after the patch. I found a regression, the functionality I implemented before is broken: Shading `org.objectweb.asm` to `aj.org.objectweb.asm` is not working anymore in package declarations. I added a simple test case in a quick & dirty way to `SimpleRelocatorTest.testRelocateSourceWithMultipleRelocators`. Just apply the patch and run that test. Of course, the change breaks `testRelocateSourceWithExcludes` as well for now, because I simply hacked another package declaration into the test strings, but it should be enough for you to inspect the problem and fix your PR. We can improve the tests later.
```patch
b/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java
--- a/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java (revision Staged)
+++ b/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java (date 1625708927654)
@@ -191,6 +191,7 @@
private static final String sourceFile =
"package org.apache.maven.hello;\n" +
+ "package org.objectweb.asm;\n" +
"\n" +
"import foo.bar.Bar;\n" +
"import zot.baz.Baz;\n" +
@@ -218,6 +219,7 @@
private static final String relocatedFile =
"package com.acme.maven.hello;\n" +
+ "package aj.org.objectweb.asm;\n" +
"\n" +
"import foo.bar.Bar;\n" +
"import zot.baz.Baz;\n" +
@@ -270,6 +272,14 @@
Arrays.asList( "irrelevant.exclude", "org.apache.maven.exclude1", "org.apache.maven.sub.exclude2" ) );
SimpleRelocator relocatorPackage = new SimpleRelocator( "io", "shaded.io", null, null);
- assertEquals( relocatedFile, relocatorPackage.applyToSourceContent( relocator.applyToSourceContent( sourceFile ) ) );
+ SimpleRelocator asmPackage = new SimpleRelocator( "org.objectweb.asm", "aj.org.objectweb.asm", null, null);
+ assertEquals(
+ relocatedFile,
+ asmPackage.applyToSourceContent(
+ relocatorPackage.applyToSourceContent(
+ relocator.applyToSourceContent( sourceFile )
+ )
+ )
+ );
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer closed pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer closed pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer commented on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-883162620
Closed in favor of #105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer commented on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-883162620
Closed in favor of #105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] kriegaex edited a comment on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-878093571
I re-implemented your PR in a somewhat different way, also covering additional test cases which would otherwise have failed for my usual test project AspectJ. Please make sure to read the commit comment quoted in the PR description in order to understand what IMO source code shading is meant to be used for and what I think it should not be used for. Your feedback is welcome. Please also test with your own project(s).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer closed pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer closed pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer closed pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer closed pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] kriegaex commented on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-878093571
I re-implemented your PR in a somewhat different way, also covering additional test cases which would otherwise have failed for my usual test project AspectJ. Please make sure to read the commit comment quoted in the PR description in order to understand what IMO source code shading is meant to be used for and what I think it should not be used for. Your feedback is welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] kriegaex commented on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-876058443
Sorry for the late feedback. I just checked out this PR and ran it against my shading configuration is AspectJ, then diffed the shaded source archives before and after the patch. I found a regression, the functionality I implemented before is broken: Shading `org.objectweb.asm` to `aj.org.objectweb.asm` is not working anymore in package declarations. I added a simple test case in a quick & dirty way to `SimpleRelocatorTest.testRelocateSourceWithMultipleRelocators`. Just apply the patch and run that test. Of course, the change breaks `testRelocateSourceWithExcludes` for now because I simply hacked another package declaration into the test strings, but it should be enough for you to inspect the problem and fix your PR.
```patch
b/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java
--- a/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java (revision Staged)
+++ b/src/test/java/org/apache/maven/plugins/shade/relocation/SimpleRelocatorTest.java (date 1625708927654)
@@ -191,6 +191,7 @@
private static final String sourceFile =
"package org.apache.maven.hello;\n" +
+ "package org.objectweb.asm;\n" +
"\n" +
"import foo.bar.Bar;\n" +
"import zot.baz.Baz;\n" +
@@ -218,6 +219,7 @@
private static final String relocatedFile =
"package com.acme.maven.hello;\n" +
+ "package aj.org.objectweb.asm;\n" +
"\n" +
"import foo.bar.Bar;\n" +
"import zot.baz.Baz;\n" +
@@ -270,6 +272,14 @@
Arrays.asList( "irrelevant.exclude", "org.apache.maven.exclude1", "org.apache.maven.sub.exclude2" ) );
SimpleRelocator relocatorPackage = new SimpleRelocator( "io", "shaded.io", null, null);
- assertEquals( relocatedFile, relocatorPackage.applyToSourceContent( relocator.applyToSourceContent( sourceFile ) ) );
+ SimpleRelocator asmPackage = new SimpleRelocator( "org.objectweb.asm", "aj.org.objectweb.asm", null, null);
+ assertEquals(
+ relocatedFile,
+ asmPackage.applyToSourceContent(
+ relocatorPackage.applyToSourceContent(
+ relocator.applyToSourceContent( sourceFile )
+ )
+ )
+ );
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-shade-plugin] Reamer commented on pull request #100: [MSHADE-396] Improve SourceContent Shading
Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #100:
URL: https://github.com/apache/maven-shade-plugin/pull/100#issuecomment-883162620
Closed in favor of #105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org