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