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/05/21 09:27:32 UTC
[GitHub] [maven-shade-plugin] kriegaex opened a new pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
kriegaex opened a new pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95
Also, rename method 'DefaultShader.shadeSingleJar' to 'shadeJarEntry' in
order to reflect what it actually does: It does *not* shade a JAR but a
JAR entry, i.e. not a "single JAR" but rather a single file.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MSHADE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MSHADE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MSHADE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [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)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637089383
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> Start by creating a ShadeClassRemapper...
It is after midnight here, maybe I can look into it tomorrow when I can think straight again. Thanks.
> Another thing to test is just to compare the byte[] before and after
No, that is exactly what is **not** working and what my change is about: to determine that no relocation had taken place, even though the byte code is different due to rewriting with ASM. Those different files, even though I was sure they were unrelated to any relocation, were what triggered my attention and made me discover what was going on here. So that idea is a dead end. Maybe check out and use my code, looking at it as a whole instead of focusing on the thread-local, if you have not done so yet, don't just look at it on GitHub.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637097894
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
I didn't say or mean, you didn't review it. You obviously did. I was not sure if you checked out the branch and watched what it does and compared to what it did before. If you did, fine. Then you know the byte code comparison doesn't cut it. Possibly your other idea does. I never understood this cryptic way of nesting and delegating readers, writers, visitors and remappers in ASM. So this is me being inexperienced in that field or maybe also being not smart enough or not conditioned on this kind of usage pattern. But I do promise to try and understand what you mean tomorrow.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636836933
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
@eolivelli, I can make the TL final, no problem.
@rmannibucau, the plugin already uses one `ClassRemapper` per file (i.e. per JAR entry, not per JAR) in order to take care of source file references in each class file. I think we should not create extra objects (one per file in each source JAR) just so as to store a single value. I am not an ASM buff either, BTW. To me, a thread-local is just a tool, it is not hacky if used the right way. I know, there is no multi-threading anywhere in Shade at the moment, but if somebody decides to run a parallel Maven build within one VM, it could easily happen that multiple shading processes are going on at the same time.
But if the thread-local is not acceptable, I would change it before the PR does not get merged. I am happy to get quick feedback, after my other PR went unnoticed for so long, even though it provides higher value than this one.
--
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] eolivelli commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636824524
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
usually ThreadLocals are declared a "static final" members
--
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 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846887726
@rmannibucau: I showed you mine. If you show me yours, I can say more about it. Is it a PR on top of my PR or a reimplementation of it? I would not be sad to throw away mine, because the sole purpose of it is to fix the problem. If it just remains to be the trigger for you to fix it in a more elegant way, it still served its purpose. So how are we going to do this?
--
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] rmannibucau commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883079617
Not sure it helps but personally I don't care at all if commits are squashed or not, it is up to the code writter to do it (I see it as a doc writer task). Also not having my name in "blame" is fine if you judge it is saner to squash commits.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846350498
Sorry for the force-push, but I think my change was not worth a separate commit, which would still be visible after merging this (unless the person merging it squashes commits).
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the method which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass all the way through to the `map` method, it would not only be ugly but also mean that in the remapper it would have to be kept in a thread-local again, because the (quasi singleton) remapper can be called multiple times.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882877678
@kriegaex Pls squash these commits to one. We will include this PR in a release. Thx
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637089383
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> Start by creating a ShadeClassRemapper...
It is after midnight here, maybe I can look into it tomorrow when I can think straight again. Thanks.
> Another thing to test is just to compare the byte[] before and after
No, that is exactly what is **not** working and what my change is about: to determine that no relocation has taken place, even though the byte code is different due to rewriting with ASM. Those different files, even though I was sure they were unrelated to any relocation, were what triggered my attention and made me discover what was going on here. So that idea is a dead end. Maybe check out and use my code, looking at it as a whole instead of focusing on the thread-local, if you have not done so yet, don't just look at it on GitHub.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637392426
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Actually, after my experience with questions asked on the mailing list, issues opened on Jira and PRs on GitHub, I am kind of afraid that this issue is going to lose momentum and trickle out instead of being merged and closed. I keep ending up in meta discussions with few or no comments about the actual issue discussed. So let's not bloat this. We got the code and integration test in place, I would love to see this one merged.
Then I want to move on to my MSHADE-252 feature PR next. Still no feedback there, and this is one I really have used for months from my private repository with a temporary version number, because without it Maven Shade is basically useless for me and the AspectJ project, as we also produce shaded and relocated source JARs which should respect excludes, too.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637353675
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Why would I rewrite it twice, other than in a test? In a test, I could rewrite without relocation, then again with, hoping that that way a comparison would be easier. But we are talking about the production code here.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the method which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once, like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass a remapper all the way through to the `map` method, it would not only be ugly but also mean that in the remapper itself (the one instantiated once per JAR entry) would have to be kept in a thread-local again, because the (quasi singleton) `ClassRemapper` can be called multiple times. Then we would not have won anything.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882877678
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
@Tibor17, I see no reason to squash the commits, because they are doing separate things. I see you use separate commits in one PR too, e.g. lately in Surefire, and I think small commits are a good practice. If ever you need to revert something, chances are that you can revert a single commit instead of a part of a big one. Bisecting bugs is easier, too. Furthermore, in this case there are two separate committers. Squashing everything into one commit would mean to disrespect other people's contribution and copyright. So I am not confident squasing @rmannibucau's commit into mine. The 4 commits so the following:
1. Adress the core issue
2. Add an integration test
3. The other committer refactored something because he did not like my use of thread-local.
4. I refactored something in his commit because of suboptimal naming.
This code evolution should remain visible and traceable, IMO. If you insist, I can squash 1+2, but I would rather not. Not a big commit history is a problem, big commits are.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846486020
FYI, I rebased on master after #88 was merged. Otherwise no changes.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the methods which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass all the way through to the `map` method, it would not only be ugly but also mean that in the remapper it would have to be kept in a thread-local again, because the (quasi singleton) remapper can be called multiple times.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took.
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637082823
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Start by creating a ShadeClassRemapper (a visitor) and each time remapper can be call test it was called or not.
You can even just use a boolean in the remapper since it is single thread and can be simpler at the end but in terms of arch it is the visitor which would hold that logic by decorating remapper usages.
Another thing to test is just to compare the byte[] before and after, guess in several cases it will be the same and it can be a sufficient first impl but the remapper decoration is what makes the code the more straight forward and less hacky IMHO.
Can surely hack it next week if you need help about this.
--
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 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846380070
BTW, can anyone tell me why running a single IT by
```text
mvn -DskipTests=true -Dinvoker.test=MSHADE-391_noRelocationKeepOriginalClasses verify -P run-its
```
does not work as expected? My test passes, but the setup job fails:
```text
[INFO] --- maven-invoker-plugin:3.2.1:integration-test (integration-test) @ maven-shade-plugin ---
[INFO] Running 1 setup job:
[INFO] Building: setup-parent\pom.xml
[INFO] The build exited with code 1. See C:\Users\alexa\Documents\java-src\maven-shade-plugin\target\it\setup-parent\build.log for details.
[INFO] setup-parent\pom.xml ............................. FAILED (2.4 s)
[INFO] Setup done.
[INFO] Building: MSHADE-391_noRelocationKeepOriginalClasses\pom.xml
[INFO] run post-build script verify.groovy
[INFO] MSHADE-391_noRelocationKeepOriginalClasses\pom.xml SUCCESS (7.3 s)
(...)
[INFO] --- maven-invoker-plugin:3.2.1:verify (integration-test) @ maven-shade-plugin ---
[INFO] -------------------------------------------------
[INFO] Build Summary:
[INFO] Passed: 1, Failed: 1, Errors: 0, Skipped: 0
[INFO] -------------------------------------------------
[ERROR] The following builds failed:
[ERROR] * setup-parent\pom.xml
```
If I explicitly add `invoker.test` to the command line
```text
mvn -DskipTests=true -Dinvoker.test=setup-parent,MSHADE-391_noRelocationKeepOriginalClasses verify -P run-its
```
the build passes. That is not so nice, because it requires "sacred knowledge".
--
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] rmannibucau commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846870345
Hi @kriegaex , as said last week I had a look to this and have multiple comments to do:
1. I tested the 1 remapper instance per visitor (ClassRemapper) and perfs are not impacted it seems (as expected since the visitor will allocate already way more instance than the class visitor) so I think it can be a good simplicity/perf/design solution actually
2. I tested an alternative to define a custom reduced remapper API to reduce the API surface to what we use (https://github.com/rmannibucau/maven-shade-plugin/commit/50e101feb19f4b82d14346afe613fe6064db99c4), it still relies on "1" (or its variant to set the visitor in the remapper to avoid this allocation but don't think it is worth) but makes the relocating API we use not dependent on ASM which simplifies it a bit at the cost of another interface. Think "1" is simpler but this option has a nice hidden gem: it enables to reimplement ClassRemapper which has limitations in relocations and is inconsistent with our configuration (we dont relocate bytecode but packages only semantically - not sure it is intended but what all the impl do). Don't think we need to go that far for now - once again I'm actually pretty happy with 1 since it is fast and does the work.
So overall, after some more advanced testing on small and "medium/big" (~50M) fatjars with relocations, I think it is good to just store the remapped flag in the remapper and have one instance per class jar entry. It avoids some verbosity we can try to avoid until we do a complete and adapted implementation (which would be way more than this PR tries to do IMHO).
Wdyt?
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637091439
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
I did review it, except this threadlocal issue which will explode in a few more iterations at our face it looks acceptable to me
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the method which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once (per JAR), like I said. It has no reference to the per-file `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass a per-file remapper all the way through to the per-JAR `map` method, it would not only be ugly but also mean that in the per-JAR remapper it would have to be kept in a thread-local again, methods of than instance can theoretically be called by multiple threads. Then we would not have won anything. We would still have a thread-local and one more level of indirection.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-884493721
@kriegaex
Thx for contributing! See the master...
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637365707
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
@kriegaex the thread local issue is that it is easily breakable - it is fragile code, wrongly auto-document the code (threadlocal is mainly about multithreaded code and here you have a single thread) and is easily manipulable by code not belonging to this layer (it is not the remapper which owns that logic but the visitor which actually delegates to the writer or writer itself which creates the class). This is why I say it is a quick and dirty fix/workaround and not a future proof solution. Will work on it on monday forking your branch.
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636826996
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
guess there is no need of any threadlocal, the boolean can be stored in a custom ClassRemapper, sounds simpler/less hacky in terms of code to me, wdyt?
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637391903
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Well to have tested such options, unless you have tons of relocations (really tons) it will be slower.
Let's wait a few dayd i PR on your pr with my proposal and move forward from that.
Hope it sounds ok.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637089383
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> Start by creating a ShadeClassRemapper...
It is after midnight here, maybe I can look into it tomorrow when I can think straight again. Thanks.
> Another thing to test is just to compare the byte[] before and after
No, that is exactly what is **not** working and what my change is about: to determine that no relocation has taken place, even though the byte code might be (and often really is) different due to rewriting with ASM. Those different files, even though I was sure they were unrelated to any relocation, were what triggered my attention and made me discover what was going on here. So that idea is a dead end. Maybe check out and use my code, looking at it as a whole instead of focusing on the thread-local, if you have not done so yet, don't just look at it on GitHub.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637390118
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> multithreading is unlikely to be faster to write a zip
Good point, I agree. But reading and processing could be done concurrently, writing then queued depending on which thread or pipeline is done with its transformation. I do admit it is a theoretical situation. If after my PR is merged you want to ditch the thread-local in favour of an ordinary boolean (then no longer final in that case, of course) in a subsequent refactoring step, I will not object. The thread-local can always be re-introduced on demand. But at least we have one commit documenting what an (imperfect, but working) solution could look like.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637354880
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
I made the `ThreadLocal` final, as requested. I decided not to refactor to create more extra ASM visitor instances just so as to avoid the thread-local field. I do not see that is should "blow up anyone's face", as @rmannibucau said. The code is simple enough and nothing I am doing there is shady or rocket science. Of course, you can always solve a problem by introducing another layer of complexity, but I have decided against it here and not to cater to someone's personal antipathy for thread-locals. Sorry, but this is my decision for this contribution. We wasted too much time fior discussing this tiny detail already. I would rather have gotten feedback concerning the functionality as such.
@rmannibucau, @eolivelli, would you please also review #88. I have been waiting for a review and for the final merge for too long already. I basically added a new feature for source code shading respecting package exclusions, hence fixing the existing, fragmentary functionality, which was completely broken IMO. Now it is slower, but it does what it should.
--
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] Tibor17 edited a comment on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883216671
@kriegaex
The reason why it should be squashed is one Jira issue MSHADE-391. The commits are worth for thinking process in the PR but it is useless for the history in master since we do not need to see the reason why this line appeared in one commit and it was deleted in another commit within the same issue MSHADE-391.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882133546
@eolivelli
@rmannibucau
It looks like you have approved all commits.
Are there any objections to push this?
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637392426
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Actually, after my experience with questions asked on the mailing list, issues opened on Jira and PRs on GitHub, I am kind of afraid that this issue is going to lose momentum and trickle out instead of being merged and closed. Let's not bloat this. We got the code and integration test in place, I would love to see this one merged and then MSHADE-252 next. Still no feedback there, and this is one I really have used for months from my private repository with a temporary version number, because without it Maven Shade is basically useless for me and the AspectJ project, as we also produce shaded and relocated source JARs which should respect excludes, too.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
@Tibor17, I see no reason to squash the commits, because they are doing separate things. I see you use separate commits in one PR too, e.g. lately in Surefire, and I think small commits are a good practice. If ever you need to revert something, chances are that you can revert a single commit instead of a part of a big one. Bisecting bugs is easier, too. Furthermore, in this case there are two separate committers. Squashing everything into one commit would mean to disrespect other people's contribution and copyright. So I am not confident squasing @rmannibucau's commit into mine. The 4 commits do the following:
1. Adress the core issue.
2. Add an integration test.
3. The other committer refactored something because he did not like my use of thread-local.
4. I refactored something in his commit because of suboptimal naming.
This code evolution and protocol of my collaboration with Romain should remain visible and traceable, IMO. If you insist, I can squash 1+2, but I would rather not. Not a big commit history is a problem, big commits are.
--
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] rmannibucau commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883079617
Not sure it helps but personally I don't care at all if commits are squashed or not, it is up to the code writter to do it (I see it as a doc writer task). Also not having my name in "blame" is fine if you judge it is saner to squash commits.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-884616319
Thanks for the merge, @Tibor17. Maybe next time you want to retain at least part of the squashed commit comments.
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637384689
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
It will likely not change cause multithreading is unlikely to be faster to write a zip until you load all inputs/output in mem which means risking an oom.
Fact encapsulation is not respected and contract is noy consistent with the usage makes me think it is saner to just impl the feature for whta it is rather than doing a quick fix.
Once again, no issue if _you_ do not change it while it is done ;).
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637354880
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
I made the `ThreadLocal` final, as requested. I decided not to refactor to create more extra ASM visitor instances just so as to avoid the thread-local field. I do not see that or why it should "blow up anyone's face", as @rmannibucau said. The code is simple enough and nothing I am doing there is shady or rocket science. Of course, you can always solve a problem by introducing another layer of complexity, but I have decided against it here and not to cater to someone's personal antipathy for thread-locals. Sorry, but this is my decision for this contribution. We wasted too much time for discussing this tiny detail already. I would rather have gotten feedback concerning the functionality as such.
@rmannibucau, @eolivelli, would you please also review #88. I have been waiting for a review and for the final merge for too long already. I basically added a new feature for source code shading respecting package exclusions, hence fixing the existing, fragmentary functionality, which was completely broken IMO. Now it is slower, but it does what it should.
--
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] rmannibucau commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882293803
@Tibor17 no, go for it
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636857368
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -706,45 +719,25 @@ public boolean hasRelocators()
public Object mapValue( Object object )
{
- if ( object instanceof String )
- {
- String name = (String) object;
- String value = name;
-
- String prefix = "";
- String suffix = "";
-
- Matcher m = classPattern.matcher( name );
- if ( m.matches() )
- {
- prefix = m.group( 1 ) + "L";
- suffix = ";";
- name = m.group( 2 );
- }
-
- for ( Relocator r : relocators )
- {
- if ( r.canRelocateClass( name ) )
- {
- value = prefix + r.relocateClass( name ) + suffix;
- break;
- }
- else if ( r.canRelocatePath( name ) )
- {
- value = prefix + r.relocatePath( name ) + suffix;
- break;
- }
- }
-
- return value;
- }
-
- return super.mapValue( object );
+ return object instanceof String ? map( (String) object, true ) : super.mapValue( object );
}
public String map( String name )
{
- String value = name;
+ // TODO: Before the refactoring of duplicate code to 'private String map(String, boolean)', this method did
Review comment:
You see the duplication in the original code before my change, but also the slight difference. That might be an oversight or on purpose, but nobody documented the intent of duplicating with a minimal difference. The tests I ran locally (all unit and integration tests plus the project I am working on) did not show any differences if I just always check for both dotty and slashy package names.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637390118
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> multithreading is unlikely to be faster to write a zip
Good point, I agree. But reading and processing could be done concurrently, then queuing to a single writing thread in the random order of which thread or pipeline is done with its transformation first. I do admit it is a theoretical situation. If after my PR is merged you want to ditch the thread-local in favour of an ordinary boolean (then no longer final in that case, of course) in a subsequent refactoring step, I will not object. The thread-local can always be re-introduced on demand. But at least we have one commit documenting what an (imperfect, but working) solution could look like.
--
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] Tibor17 edited a comment on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883216671
@kriegaex
The reason why it should be squashed is one Jira issue MSHADE-391. The commits are worth for thinking process in the PR but it is useless for the history in master since we do not need to see the reason why this line appeared in one commit and it was deleted in another commit within the same issue MSHADE-391.
--
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] eolivelli commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636855651
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -706,45 +719,25 @@ public boolean hasRelocators()
public Object mapValue( Object object )
{
- if ( object instanceof String )
- {
- String name = (String) object;
- String value = name;
-
- String prefix = "";
- String suffix = "";
-
- Matcher m = classPattern.matcher( name );
- if ( m.matches() )
- {
- prefix = m.group( 1 ) + "L";
- suffix = ";";
- name = m.group( 2 );
- }
-
- for ( Relocator r : relocators )
- {
- if ( r.canRelocateClass( name ) )
- {
- value = prefix + r.relocateClass( name ) + suffix;
- break;
- }
- else if ( r.canRelocatePath( name ) )
- {
- value = prefix + r.relocatePath( name ) + suffix;
- break;
- }
- }
-
- return value;
- }
-
- return super.mapValue( object );
+ return object instanceof String ? map( (String) object, true ) : super.mapValue( object );
}
public String map( String name )
{
- String value = name;
+ // TODO: Before the refactoring of duplicate code to 'private String map(String, boolean)', this method did
Review comment:
I am sorry, I am not sure how to help.
I think that keeping the current behaviour is safer.
we can work on this topic on a follow up change
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the method which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once (per JAR file), like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass a remapper all the way through to the `map` method, it would not only be ugly but also mean that in the remapper itself (the one instantiated once per file inside a JAR) would have to be kept in a thread-local again, because the (quasi singleton) `ClassRemapper` can be called multiple times. Then we would not have won anything.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637105372
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Well it does if you rewrite it twice which is likely slower - didnt test but being in mem we can be surprised, was my point.
Agree asm API is not that nice when used in visitor but it is needed copying a class so let's keep it for this iteration maybe.
Can help with code on monday (too late for this week ;))
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636836933
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
@eolivelli, I can make the TL final, no problem.
@rmannibucau, the plugin already uses one `ClassRemapper` per file (i.e. per JAR entry, not per JAR) in order to take care of source file references in each class file. I think we should not create extra objects (one per file in each source JAR) just so as to store a single value. I am not an ASM buff either, BTW. But if the thread-local is not acceptable, I would change it before the PR does not get merged. I am happy to get quick feedback, after my other PR went unnoticed for so long, even though it provides higher value than this one.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637379490
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Yes, in a way it is premature optimisation. I call it defensive programming. I know that it is single-threaded at the moment, but that could change in the future. The funny thing is, if I would have ignored the thread-unsafety here, nobody would have raised a red flag during the code review. The fact that I did take some precaution, suddenly makes it bad. I disagree with that kind of assessment. But feel free to fork this, if you think you must. I am always happy to learn. I just hink, this should be done in a future PR, not here. I would rather have addressed my question from the TODO comment here, but there the reaction was to do it next time. I guess, different people just have a different sense of priorities, because it is a matter of personal judgement.
I simply disagree with you here about "easily breakable" or "fragile". I am not going to change it.l
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Yes, in a way it is premature optimisation. I call it defensive programming. I know that it is single-threaded at the moment, but that could change in the future. The funny thing is, if I would have ignored the thread-unsafety here, nobody would have raised a red flag during the code review. The fact that I did take some precaution, suddenly makes it bad. I disagree with that kind of assessment. But feel free to fork this, if you think you must. I am always happy to learn. I just hink, this should be done in a future PR, not here. I would rather have addressed my question from the TODO comment here, but there the reaction was to do it next time. I guess, different people just have a different sense of priorities, because it is a matter of personal judgement.
I simply disagree with you here about "easily breakable" or "fragile". I am not going to change it.
--
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 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846377157
> I wonder if you could try to add a test case in order to prevent regressions in the future
I just added an integration test. Please review and execute.
--
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] Tibor17 closed pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 closed pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637379490
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Yes, in a way it is premature optimisation. I call it defensive programming. I know that it is single-threaded at the moment, but that could change in the future. The funny thing is, if I would have ignored the thread-unsafety here, nobody would have raised a red flag during the code review. The fact that I did take some precaution, suddenly makes it bad. I disagree with that kind of assessment. But feel free to fork this, if you think you must. I am always happy to learn. I just think, this should be done in a future PR, not here. I would rather have addressed my question from the TODO comment here, but there the reaction was to do it next time. I guess, different people just have a different sense of priorities, because it is a matter of personal judgement.
I simply disagree with you here about "easily breakable" or "fragile". I am not going to change it.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
@Tibor17, I see no reason to squash the commits, because they are doing separate things. I see you use separate commits in one PR too, e.g. lately in Surefire, and I think small commits are a good practice. If ever you need to revert something, chances are that you can revert a single commit instead of a part of a big one. Bisecting bugs is easier, too. Furthermore, in this case there are two separate committers. Squashing everything into one commit would mean to disrespect other people's contribution and copyright. So I am not confident squasing @rmannibucau's commit into mine. The 4 commits so the following:
1. Adress the core issue
2. Add an integration test
3. The other committer refactored something because he did not like my use of thread-local.
4. I refactored something in his commit because of suboptimal naming.
This code evolution and protocol of my collaboration with Romain should remain visible and traceable, IMO. If you insist, I can squash 1+2, but I would rather not. Not a big commit history is a problem, big commits are.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882877678
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637390118
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
> multithreading is unlikely to be faster to write a zip
Good point, I agree. But reading and processing could be done concurrently, then queuing to a single writing thread in the random order of which thread or pipeline is done with its transformation first. I do admit it is a theoretical situation. If after my PR is merged you want to ditch the thread-local in favour of an ordinary boolean (then no longer final in that case, of course) in a subsequent refactoring step, I will not object. The thread-local can always be re-introduced on demand. But at least we have one commit documenting what an (imperfect, but working) solution could look like. And work it does.
--
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] rmannibucau commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883079617
Not sure it helps but personally I don't care at all if commits are squashed or not, it is up to the code writter to do it (I see it as a doc writer task). Also not having my name in "blame" is fine if you judge it is saner to squash commits.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-847010353
@rmannibucau, I pushed both your commit and my refactoring of it to this PR, please review again. I hope you are OK with what I changed. The commit comment is quite extensive, if you want to read that first before looking at the diff. I am happy to collaborate with you in this way, that is so much better than a reviewer micro-managing the contributor to do exactly as he says. Taking turns and refining this together is great! I hope we can learn from each other.
Do you think that this one is mergeable now?
--
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] eolivelli commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846417356
I am sorry I cannot help with the single run issue.
For some plugjns I modify the pom.xml file in order to select only one single IT.
I have never really worked on this plugin
--
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 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-845878650
> I wonder if you could try to add a test case in order to prevent regressions in the future
Such a test would be heuristic, relying on whether ASM happens to restructure certain class files during transformation or not. It would also be an integration test with some real world dependencies and classes using them. Indirectly, we would be testing ASM rather than Shade, but the test is certainly doable. It would be more expensive to create than the actual change, though.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636852467
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Update: I forgot to mention, that the custom `Remapper` called `RelocatorRemapper` is instantiated only once, and I did not want to refactor the whole plugin. So I stuck with the idea of one instance and used the thread-local field.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-846887726
@rmannibucau: I showed you mine. If you show me yours, I can say more about it. Is it a PR on top of my PR or a reimplementation of it? I would not be sad to throw away mine, because the sole purpose of it is to fix the problem. If it just remains to be the trigger for you to fix it in a more elegant way, it still served its purpose. So how are we going to do this?
**Update:** OK, I just noticed the link in your message and can see that your commit [piggy-backs on mine](https://github.com/apache/maven-shade-plugin/compare/master...rmannibucau:MSHADE-391). Going to take a look now.
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the method which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once, like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass all the way through to the `map` method, it would not only be ugly but also mean that in the remapper it would have to be kept in a thread-local again, because the (quasi singleton) remapper can be called multiple times.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] Tibor17 edited a comment on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883216671
@kriegaex
The reason why it should be squashed is one Jira issue MSHADE-391. The commits are worth for thinking process in the PR but it is useless for the history in master since we do not need to see the reason why this line appeared in one commit and it was deleted in another commit within the same issue MSHADE-391.
--
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 a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637392426
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Actually, after my experience with questions asked on the mailing list, issues opened on Jira and PRs on GitHub, I am kind of afraid that this issue is going to lose momentum and trickle out instead of being merged and closed. Let's not bloat this. We got the code and integration test in place, I would love to see this one merged.
The I want to move on to my MSHADE-252 feature PR next. Still no feedback there, and this is one I really have used for months from my private repository with a temporary version number, because without it Maven Shade is basically useless for me and the AspectJ project, as we also produce shaded and relocated source JARs which should respect excludes, too.
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Actually, after my experience with questions asked on the mailing list, issues opened on Jira and PRs on GitHub, I am kind of afraid that this issue is going to lose momentum and trickle out instead of being merged and closed. Let's not bloat this. We got the code and integration test in place, I would love to see this one merged.
Then I want to move on to my MSHADE-252 feature PR next. Still no feedback there, and this is one I really have used for months from my private repository with a temporary version number, because without it Maven Shade is basically useless for me and the AspectJ project, as we also produce shaded and relocated source JARs which should respect excludes, too.
--
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] rmannibucau commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r636874735
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
A thread local is a hack in the sense it is a solution to keep track of a data in a multithreaded application with reference breakage which is not the case here. Here you have 1 instance of classreader/writer per class file which can track the state of the rewritten class (plain boolean).
An example of such thing can be seen in sirona: https://github.com/rmannibucau/sirona/blob/trunk/agent/javaagent/src/main/java/com/github/rmannibucau/sirona/javaagent/SironaClassVisitor.java#L139
So no additional instantiation in object count, no plugin refactoring or so and simpler code path since the state becomes an attribute and not an unrelated threadlocal so overall I don't see any reason to use a quick and dirty option for this.
hope it makes sense
--
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 commented on a change in pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on a change in pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#discussion_r637018710
##########
File path: src/main/java/org/apache/maven/plugins/shade/DefaultShader.java
##########
@@ -694,6 +702,11 @@ private void addResource( Set<String> resources, JarOutputStream jos, String nam
List<Relocator> relocators;
+ // Use thread-local, just in case 'map*' calls are ever done concurrently. Make sure that the using class
+ // initialises this value according to its needs, usually setting the value to false per file before starting
+ // relocation.
+ ThreadLocal<Boolean> wasRelocated = new ThreadLocal<>();
Review comment:
Probably it does, but I do not understand how you would implement it, because the methods which knows that a relocation has taken place is `DefaultShader.RelocatorRemapper.map(String, boolean)`, and that object is only instantiated once like I said. It has no reference to the `ClassRemapper` used locally in `DefaultShader.addRemappedClass()`, but the other way around. If I would pass all the way through to the `map` method, it would not only be ugly but also mean that in the remapper it would have to be kept in a thread-local again, because the (quasi singleton) remapper can be called multiple times.
So if you mean something else and have thought this through, either explain it as clearly as you can, so I can implement it according to your idea, or accept the PR with the thread-local and refactor it by yourself in a later stage. What I am presenting is a substantial improvement. I see no need to make it perfect, just better than before without any new bugs. Let us apply the boyscout rule: leave the camp ground behind a little cleaner than we found it, but let us not stay there longer than the actual trip took. I wanted to address one problem and did so. My plan was not to refactor the whole plugin architecture. I see these classes for the first time and have zero Maven plugin development knowledge. Please do not expect too much of my humble contribution here.
--
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] Tibor17 commented on pull request #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-883216671
@kriegaex
The reason why it should be squashed is one Jira issue MSHADE-391. The commits are worth for thinking process in the PR but it is useless for the history in master since we do not need to see the reason why this line appeared and it was deleted in another commit within the same issue MSHADE-391.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex commented on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-845879392
Please also find the TODO in the diff and provide some feedback as to whether or not I should simplify the mapping method to an unparametrised one used by both callers. If so, I would rather inline it and chain the calls.
--
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 #95: [MSHADE-391] Do not write modified class files for no-op relocations
Posted by GitBox <gi...@apache.org>.
kriegaex edited a comment on pull request #95:
URL: https://github.com/apache/maven-shade-plugin/pull/95#issuecomment-882959624
@Tibor17, I see no reason to squash the commits, because they are doing separate things. I see you use separate commits in one PR too, e.g. lately in Surefire, and I think small commits are a good practice. If ever you need to revert something, chances are that you can revert a single commit instead of a part of a big one. Bisecting bugs is easier, too. Furthermore, in this case there are two separate committers. Squashing everything into one commit would mean to disrespect other people's contribution and copyright. So I am not confident squasing @rmannibucau's commit into mine. The 4 commits do the following:
1. Adress the core issue
2. Add an integration test
3. The other committer refactored something because he did not like my use of thread-local.
4. I refactored something in his commit because of suboptimal naming.
This code evolution and protocol of my collaboration with Romain should remain visible and traceable, IMO. If you insist, I can squash 1+2, but I would rather not. Not a big commit history is a problem, big commits are.
--
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