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:
us...@infra.apache.org
