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:
[email protected]
