[MSHADE-391] Do not write modified class files for no-op relocations #95

kriegaex · 2021-05-21T09:27:17Z

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.

Make sure there is a JIRA issue 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.
Each commit in the pull request should have a meaningful subject line and body.
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.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
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 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
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

eolivelli

great work
thank you very much.

I wonder if you could try to add a test case in order to prevent regressions in the future

src/main/java/org/apache/maven/plugins/shade/DefaultShader.java

kriegaex · 2021-05-21T11:16:59Z

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.

kriegaex · 2021-05-21T11:18:23Z

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.

src/main/java/org/apache/maven/plugins/shade/DefaultShader.java

kriegaex · 2021-05-22T05:01:40Z

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).

kriegaex · 2021-05-22T08:51:34Z

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.

kriegaex · 2021-05-22T09:15:38Z

BTW, can anyone tell me why running a single IT by

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:

[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

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".

eolivelli

Thanks for adding the test.

The patch looks great to me

+1

eolivelli · 2021-05-22T14:39:30Z

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

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.

Test src/it/projects/MSHADE-391_noRelocationKeepOriginalClasses shades two dependencies, relocating one of them. For the non-relocated one, it compares pre-calculated MD5 checksums to the actual ones in the uber JAR, making sure that they are identical. The test will fail in Maven Shade 3.2.4 or older, but pass with the changes made in MSHADE-391.

kriegaex · 2021-05-23T01:20:36Z

FYI, I rebased on master after #88 was merged. Otherwise no changes.

… visitor to host the state in the visitor

rmannibucau · 2021-05-24T08:20:48Z

Hi @kriegaex , as said last week I had a look to this and have multiple comments to do:

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
I tested an alternative to define a custom reduced remapper API to reduce the API surface to what we use (rmannibucau@50e101f), 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?

kriegaex · 2021-05-24T08:46:58Z

@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. Going to take a look now.

There was some suboptimal naming: We had class names - Relocator, - SimpleRelocator, - Relocators, - DefaultRelocators. That was a bit too much of '*Relocator(s)', and the plural was not nice either. We are actually dealing with one mapper (interface), the default implementation of which happens to own a list of relocators. We now have - Relocator, - SimpleRelocator, - PackageMapper, - DefaultPackageMapper. I also changed PackageMapper.map to have two boolean parameters now, even though the first one is always true in current use cases. But I wanted the method signature to more clearly describe what the boolean parameters do. Before there was just 'boolean clazz', which does not communicate anything to the caller. Now we have the method signature String map(String entityName, boolean mapPaths, boolean mapPackages) which is better in this respect. Also, 'entityName' is a clearer than just 'name'. Despite the interface being private, I also added Javadoc to this method, because it is a crucial one for understanding how class relocation and entity name mapping work. Test case DefaultShaderTest.testNoopWhenNotRelocated was refactored to be a bit more readable and also to explicitly test that the byte code for a relocated class has actually changed and not only its path within the uber JAR has been adjusted. I also renamed the helper method 'areEquals' to 'areEqual'.

kriegaex · 2021-05-24T12:30:41Z

@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?

Tibor17 · 2021-07-18T23:37:24Z

@eolivelli
@rmannibucau
It looks like you have approved all commits.
Are there any objections to push this?

rmannibucau · 2021-07-19T07:02:12Z

@Tibor17 no, go for it

Tibor17 · 2021-07-19T21:35:59Z

@kriegaex Pls squash these commits to one. We will include this PR in a release. Thx

kriegaex · 2021-07-20T00:47:05Z

@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:

Adress the core issue.
Add an integration test.
The other committer refactored something because he did not like my use of thread-local.
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.

rmannibucau · 2021-07-20T05:42:15Z

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.

Tibor17 · 2021-07-20T08:46:36Z

@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.

kriegaex · 2021-07-20T09:50:17Z

@Tibor17, that is not sound reasoning IMO. I completely disagree. Whether it is in a PR or not, later I might need to bisect when there is a problem. Smaller commits mean, it is easier to find bugs. The older a commit gets, the more important it becomes. So commits are more than just part of an iterative development process.

The reason why it should be squashed is one Jira issue MSHADE-391.

Seriously? That is your argument? Because there is one Jira issue, there must be a single commit? Sorry, I do not mean to disrespect you, but that is complete nonsense.

The commits are worth for thinking process in the PR but it is useless for the history in master

The opposite is true.

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.

Again, I disagree. It helps understand where problems come from and also the opposite, i.e. why some things we would not understand without the history are not problems. Besides, you also want me to squash commits with non-intersecting lines of code.

You know what? If you want to be the dictator micro-managing people, just squash the commits by yourself. I know you think you are always right and you have the power to merge, I don't. But that does not mean I need to always do what you demand. Merge as is or squash by yourself, if it is so important for you, or close the issue. I really don't care, the way you treat contributors like children both here and in Surefire.

Edit: You are a competent developer, and I respect that. I cannot agree with your world view of how good software development should be done, though. I also cannot deal with the way you treat people. Sorry.

Tibor17 · 2021-07-21T20:56:15Z

@kriegaex
Thx for contributing! See the master...

kriegaex · 2021-07-22T02:26:05Z

Thanks for the merge, @Tibor17. Maybe next time you want to retain at least part of the squashed commit comments.

kriegaex force-pushed the MSHADE-391 branch from e9d5cda to 603ce00 Compare May 21, 2021 09:35

eolivelli reviewed May 21, 2021

View reviewed changes

src/main/java/org/apache/maven/plugins/shade/DefaultShader.java Outdated Show resolved Hide resolved

eolivelli reviewed May 21, 2021

View reviewed changes

src/main/java/org/apache/maven/plugins/shade/DefaultShader.java Outdated Show resolved Hide resolved

kriegaex force-pushed the MSHADE-391 branch from 603ce00 to d9068c8 Compare May 22, 2021 04:51

kriegaex force-pushed the MSHADE-391 branch from 159fdcd to 7420f3d Compare May 22, 2021 09:03

eolivelli approved these changes May 22, 2021

View reviewed changes

kriegaex added 2 commits May 23, 2021 08:15

[MSHADE-391] Do not write modified class files for no-op relocations

c755ad4

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.

kriegaex force-pushed the MSHADE-391 branch from 7420f3d to 626b2eb Compare May 23, 2021 01:16

[MSHADE-391] drop the threadlocal in favor of a remapper instance per…

50e101f

… visitor to host the state in the visitor

rmannibucau approved these changes May 24, 2021

View reviewed changes

kriegaex mentioned this pull request Jul 12, 2021

[MSHADE-366] - "Access denied" during 'minimizeJar' #83

Closed

1 task

Tibor17 closed this Jul 21, 2021

kriegaex deleted the MSHADE-391 branch July 21, 2021 23:14

kriegaex restored the MSHADE-391 branch July 21, 2021 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MSHADE-391] Do not write modified class files for no-op relocations #95

[MSHADE-391] Do not write modified class files for no-op relocations #95

kriegaex commented May 21, 2021 •

edited

eolivelli left a comment

kriegaex commented May 21, 2021

kriegaex commented May 21, 2021

kriegaex commented May 22, 2021

kriegaex commented May 22, 2021

kriegaex commented May 22, 2021

eolivelli left a comment

eolivelli commented May 22, 2021

kriegaex commented May 23, 2021

rmannibucau commented May 24, 2021

kriegaex commented May 24, 2021 •

edited

kriegaex commented May 24, 2021

Tibor17 commented Jul 18, 2021

rmannibucau commented Jul 19, 2021

Tibor17 commented Jul 19, 2021

kriegaex commented Jul 20, 2021 •

edited

rmannibucau commented Jul 20, 2021

Tibor17 commented Jul 20, 2021 •

edited

kriegaex commented Jul 20, 2021 •

edited

Tibor17 commented Jul 21, 2021

kriegaex commented Jul 22, 2021

[MSHADE-391] Do not write modified class files for no-op relocations #95

[MSHADE-391] Do not write modified class files for no-op relocations #95

Conversation

kriegaex commented May 21, 2021 • edited

eolivelli left a comment

Choose a reason for hiding this comment

kriegaex commented May 21, 2021

kriegaex commented May 21, 2021

kriegaex commented May 22, 2021

kriegaex commented May 22, 2021

kriegaex commented May 22, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented May 22, 2021

kriegaex commented May 23, 2021

rmannibucau commented May 24, 2021

kriegaex commented May 24, 2021 • edited

kriegaex commented May 24, 2021

Tibor17 commented Jul 18, 2021

rmannibucau commented Jul 19, 2021

Tibor17 commented Jul 19, 2021

kriegaex commented Jul 20, 2021 • edited

rmannibucau commented Jul 20, 2021

Tibor17 commented Jul 20, 2021 • edited

kriegaex commented Jul 20, 2021 • edited

Tibor17 commented Jul 21, 2021

kriegaex commented Jul 22, 2021

kriegaex commented May 21, 2021 •

edited

kriegaex commented May 24, 2021 •

edited

kriegaex commented Jul 20, 2021 •

edited

Tibor17 commented Jul 20, 2021 •

edited

kriegaex commented Jul 20, 2021 •

edited