Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSHADE-36] Use reduced dependencies #25

Conversation

@nielsbasjes
Copy link
Contributor

@nielsbasjes nielsbasjes commented Aug 27, 2019

This is a proposed fix for the issue https://issues.apache.org/jira/browse/MSHADE-36 which has been open for over 11 years.

The implementation is activated when the setting "useDependencyReducedPomInJar" is set to true.
It then essentially injects two ResourceTransformers from code

  1. Remove the original "META-INF/maven/${project.groupId}/${project.artifactId}/pom.xml".
  2. Include the dependency-reduced-pom.xml under the name of the original pom.xml into the final jar file.

This includes an integration test to verify this actually happens.


Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it).
    https://issues.apache.org/jira/browse/MSHADE-36
  • 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.

@nielsbasjes nielsbasjes changed the title [MSHADE 36] Use reduced dependencies [MSHADE-36] Use reduced dependencies Aug 27, 2019
@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Aug 27, 2019

I noticed that there will be a conflict with #22 because I moved the creation of the dpr to a slightly different place (more forward instead of more backward).

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-36-Use-Reduced-Dependencies branch 2 times, most recently from 08d08a6 to 6c25ac7 Aug 30, 2019
@struberg
Copy link
Member

@struberg struberg commented Dec 17, 2019

What is the status of this ticket? @Tibor17 was there something missing still?

Loading

@rfscholte
Copy link
Contributor

@rfscholte rfscholte commented Dec 17, 2019

I talked some time ago with @hboutemy about shading and poms. Which is the right pom? Think about licenses, etc. I can image we need a shaded scope, which keeps all original information and respects the shading-aspects. But that is another topic.

Loading

Copy link
Contributor

@elharo elharo left a comment

Is this still under consideration/development or should we close it?

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Apr 10, 2020

@elharo I would like to see this problem fixed. If there is anything I need to change then I'm happy to do that. Just say what the desired solution is.

Loading

@elharo
Copy link
Contributor

@elharo elharo commented Apr 11, 2020

Looks like for this issue to move forward it needs a proposal (not a PR) that can be discussed on the dev mailing list. If you can get consensus from the project maintainers on what a fix should look like--i.e. what would go into a project POM and what the output would be--then it could be implemented. But right now it doesn't look like there's consensus on the form of the solution.

Loading

@felfert
Copy link

@felfert felfert commented Mar 17, 2021

Looks like for this issue to move forward it needs a proposal (not a PR) that can be discussed on the dev mailing list.

Perhaps, there could be a pragmatic solution to this: Instead of directly using the dependency-reduced pom directly, there
could be an option to use a user-supplied pom instead. Then the user could copy the generated pom, edit it (adding licence info etc) and then in a second run, that pom could be used in the jar?

Loading

@Snipx
Copy link

@Snipx Snipx commented Apr 29, 2021

Dear Maintainers/Contributors,
I think this is a terribly useful feature - is there any chance it can be given a bit more priority?

Loading

Loading
src/it/MSHADE-36-inject-dep-reduced-pom-in-final/pom.xml Outdated Show resolved Hide resolved
Loading
src/it/MSHADE-36-inject-dep-reduced-pom-in-final/pom.xml Outdated Show resolved Hide resolved
Loading
Loading
Loading
@@ -437,6 +447,24 @@ public void execute()

List<ResourceTransformer> resourceTransformers = getResourceTransformers();

if ( createDependencyReducedPom )
Copy link
Member

@michael-o michael-o Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have moved this before the Shader works. No changes in behavior?

Loading

Copy link
Contributor Author

@nielsbasjes nielsbasjes Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the tests present in the project today 2 fail with my changes in place.
So this does indeed need some rework.

Loading

Copy link
Contributor Author

@nielsbasjes nielsbasjes Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the problem that failed the tests made two files.
Fix that now all tests pass.

Still the question "No changes in behavior?" remains a valid one.

For now: Let's assume there IS a change the current tests did not pick up.
I'll look into it.

Loading

Copy link
Contributor Author

@nielsbasjes nielsbasjes Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My verification steps:

  1. All integration tests pass.
  2. Checkout both master and my version into separate directories.
  3. Run in both: MAVEN_OPTS=-Dinvoker.parallelThreads=2 mvn clean verify -Prun-its under javac 11.0.11
  4. Get this tool to compare the jars https://github.com/scala/jardiff
  5. Run these magical incantations on my Linux machine:
    find target/it -type f -name '*.jar' | while read JAR ; do java -jar ../jardiff.jar ${JAR} ../maven-shade-plugin-master/${JAR} ; done > jar-changes.log
    find target/it -type f -name '*pom*' | while read POM ; do diff ${POM} ../maven-shade-plugin-master/${POM} ; done > pom-changes.log
  6. Manually go through the output.
    Differences logged:
  • Files like surefirebooter2116138492290729396.jar are unique (Ignore)
  • The test files I created are only present in mine (Ignore)
  • All files have a difference in the timestamps when the files were generated (pom.properties) (Ignore)
  • Some tests include the plugin itself and show differences because of the new feature (Ignore)

@michael-o Is this sufficient verification?

Loading

Copy link
Member

@michael-o michael-o Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will verify!

Loading

Loading
@michael-o
Copy link
Member

@michael-o michael-o commented Jun 13, 2021

I am inclined to merge this after a few small changes since this is optional and does not affect default behavior. Respecing licenses and other attributions is solely a dev's task. Not ours.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 13, 2021

@nielsbasjes Please also rebase on top of master to resolve merge conflicts.

Loading

@felfert
Copy link

@felfert felfert commented Jun 13, 2021

Wow. Finally some progress.
@michael-o Please would you be so kind to have a look at #26 as well? Those two PR kind of go hand-in-hand and finally would make shading usable in multi-module projects. Thanks.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 13, 2021

@felfert As far as I can see #26 requires this PR, doesn't it? If so, I would like to complete this one and have a look at the other. I need to understand the problem first.

Loading

@felfert
Copy link

@felfert felfert commented Jun 13, 2021

@felfert As far as I can see #26 requires this PR, doesn't it? If so, I would like to complete this one and have a look at the other. I need to understand the problem first.

No, as far as I interpret the comments in #26 (See the very first line in the initial text), #26 includes #25. Also, if you click on the commits tab of #26 you see TWO commits, where one is identical to this PR.
As for understanding the problem, maybe a simple use case shows the dilemma:

Have a look at https://github.com/jenkinsci/jclouds-plugin (A plugin which uses jclouds which in turn uses guava) and
https://github.com/felfert/jclouds-shaded (A shaded version of jclouds, where the guava dependency is shaded, because jenkins itself uses a different guava version). The latter exists for the sole purpose of shading.
Normally, I never would publish a separate project only to facilitate shading. Instead, I would make the shaded jclouds a submodule of the plugin. However maven currently forces me to do this, because even with a workaround for #25, maven still uses the unshaded pom during reactor build.

Also, maybe have a look at @nielsbasjes remarks here: https://yauaa.basjes.nl/NOTES-shading-dependencies.html

Cheers
-Fritz

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 13, 2021

@felfert As far as I can see #26 requires this PR, doesn't it? If so, I would like to complete this one and have a look at the other. I need to understand the problem first.

No, as far as I interpret the comments in #26 (See the very first line in the initial text), #26 includes #25. Also, if you click on the commits tab of #26 you see TWO commits, where one is identical to this PR.

This is a contradiction, isn't it? If #26 includes this PR then it requires it. I will first resolve this one waiting for @nielsbasjes then continue with the rest.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 15, 2021

I'm picking up the reviews and comments.
It has been a while and several things have changed.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 15, 2021

I'm picking up the reviews and comments.
It has been a while and several things have changed.

Thanks, looking forward too. If all goes well we'll have a release this month.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 16, 2021

Regarding #26: That one indeed requires this fix to be in place first.
Once this one (#25) has been merged I'll update #26 for a renewed review round.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 16, 2021

@nielsbasjes Something is wrong, GitHub shows 97 commits to be merged. Did you rebase incorrectly?

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-36-Use-Reduced-Dependencies branch from 031ec4a to 0cf7fc5 Jun 16, 2021
@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 16, 2021

@nielsbasjes Something is wrong, GitHub shows 97 commits to be merged. Did you rebase incorrectly?

Yes, something went wrong here.
I'm fixing it.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 16, 2021

@nielsbasjes Let me explicitly know when you are done from your PoV and I will review and test everything.

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-36-Use-Reduced-Dependencies branch from 13b12cb to 3d1dbbc Jun 16, 2021
@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 16, 2021

@michael-o I have fixed it. CI builds all pass now.
Pushing a squased version for cleaner merging.

Loading

@nielsbasjes nielsbasjes requested a review from michael-o Jun 16, 2021
Loading
src/it/MSHADE-36-inject-dep-reduced-pom-in-final/pom.xml Outdated Show resolved Hide resolved
Loading
src/it/MSHADE-36-inject-dep-reduced-pom-in-final/pom.xml Outdated Show resolved Hide resolved
Loading
Loading
Loading
@nielsbasjes nielsbasjes requested a review from michael-o Jun 16, 2021
@michael-o
Copy link
Member

@michael-o michael-o commented Jun 16, 2021

Going through now...

Loading

Copy link
Member

@michael-o michael-o left a comment

A few issues

Loading

@nielsbasjes nielsbasjes requested review from rfscholte and michael-o Jun 16, 2021
@michael-o
Copy link
Member

@michael-o michael-o commented Jun 17, 2021

Works for me on:

$ mvn -v
Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T20:33:14+02:00)
Maven home: /usr/local/apache-maven-3.5.4
Java version: 1.8.0_292, vendor: OpenJDK BSD Porting Team, runtime: /usr/local/openjdk8/jre
Default locale: de_DE, platform encoding: UTF-8
OS name: "freebsd", version: "12.2-stable", arch: "amd64", family: "unix"

Waiting for ASF Jenkins to complete

Loading

@michael-o michael-o closed this in 83c123d Jun 17, 2021
@michael-o
Copy link
Member

@michael-o michael-o commented Jun 17, 2021

@nielsbasjes Please continue (rebase) the next PR and let me know when you are done.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 17, 2021

Yes, will do

Loading

@nielsbasjes nielsbasjes deleted the MSHADE-36-Use-Reduced-Dependencies branch Jun 17, 2021
@Snipx
Copy link

@Snipx Snipx commented Jun 18, 2021

Excellent cooperation, thank you folks! So glad to see this fixed

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jun 18, 2021

Excellent cooperation, thank you folks! So glad to see this fixed

Took way too long to process, but people are busy.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants