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-326] Hide shaded dependencies from the rest of the reactor build #26

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

@nielsbasjes
Copy link
Contributor

@nielsbasjes nielsbasjes commented Aug 28, 2019

Jira: https://issues.apache.org/jira/browse/MSHADE-326
In several multi module projects I have created I ran into the same problems with shading dependencies. See https://yauaa.basjes.nl/NOTES-shading-dependencies.html

This is a proposed change to fix those problems.

==============

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

  • Jira: https://issues.apache.org/jira/browse/MSHADE-326
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like `[MSHADE-XXX] - ... '
  • 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).

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.

@khmarbaise
Copy link
Member

@khmarbaise khmarbaise commented Aug 28, 2019

It's great that you created pull request but unfortunately you should make a separate PR for each JIRA you are trying to solve....also squash your commits into a single commit...that makes it easier to merge it and review it....many thanks for your support.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Aug 28, 2019

Yes, normally I fully agree.
However in my opinion this fix is useless/meaningless without MSHADE-36 fix I created.
That is why I chose to make this a commit on top of that one.

Loading

@GigabyteProductions
Copy link

@GigabyteProductions GigabyteProductions commented Aug 28, 2019

Squashing commits hides details of the history.

Loading

@GigabyteProductions
Copy link

@GigabyteProductions GigabyteProductions commented Aug 28, 2019

... in addition to coupling otherwise independent commits, which hurts ability to cherry pick or rebase commits away before merge.

Loading

@khmarbaise
Copy link
Member

@khmarbaise khmarbaise commented Aug 28, 2019

Have you taken a look in the apache maven projects? We use single commit setup and don't want to merge a number of 2-n commits into the history to fix/add a single feature. Single commits makes it easier to understand the history and make it easier to undo changes if needed....

Can you make the PR for MSHADE-36 (single commit) which means to be applied first and afterwards we can apply MSHADE-326 ....

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Aug 30, 2019

Working on it.

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-326-Hide-shaded-dependencies-from-the-rest-of-the-reactor-build branch from 691331f to 75463c5 Aug 30, 2019
@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Aug 30, 2019

I've updated #25 to be a single commit as requested.

Because these two changes are dependent this pull request will look like 'two commits' but it is actually a single commit that requires #25 to be already applied. I hope this is what you meant.

Please review (especially the code changes for both of these proposals themselves).

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Aug 30, 2019

FYI: I did some additional testing in my own project and these changes work as I expect them to work.
See: https://github.com/nielsbasjes/yauaa/tree/VerifyMSHADE36-MSHADE326

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-326-Hide-shaded-dependencies-from-the-rest-of-the-reactor-build branch from 75463c5 to 8f3858b Aug 30, 2019
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

@felfert felfert mentioned this pull request Jun 13, 2021
8 tasks
Copy link
Member

@michael-o michael-o left a comment

@nielsbasjes This PRs suffers from the same issues as #25, please modify.

Loading

@nielsbasjes nielsbasjes force-pushed the MSHADE-326-Hide-shaded-dependencies-from-the-rest-of-the-reactor-build branch from 8f3858b to 16cbe06 Jun 17, 2021
@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 17, 2021

@michael-o
I rebased the code.
Moved the tests to the right place.
Checked the review comments and applied similar changes (like the @since 3.3.0) .
Applied formatting to the javadoc.

And I double checked and my test fails if I remove the new flag.

Loading

@nielsbasjes nielsbasjes requested a review from michael-o Jun 17, 2021
@michael-o
Copy link
Member

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

Will first try to understand the problem and then get back to you.

Loading

}
}


Copy link
Member

@michael-o michael-o Jun 18, 2021

Choose a reason for hiding this comment

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

I think this entire block is redudant because it duplicates the same test as in the previous PR. Where is the benefit here?

Loading

Copy link
Contributor Author

@nielsbasjes nielsbasjes Jun 18, 2021

Choose a reason for hiding this comment

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

Yes you are right. This is essentially the same test, yet here it is in a multi module project.
In cases where I really want to be sure it is going right I'm in the "Check, check, double check" mindset.
This is one of those cases because of the multi-module situation.

Loading

@michael-o
Copy link
Member

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

So I reviewed this PR, the JIRA issue as well as MSHADE-206 and MNG-5899 and I see a few issues here:

  • The description on the JIRA issue is wrong
  • From my PoV this PR only fixes the symptom, not the cause
  • Looking at this plugin's source code this change (regardless of cause or symptom) can only be applied if a DPR is created, i.e., this has to happen unconditionally in rewriteDependencyReducedPomIfWeHaveReduction() attaching a new POM with modified deps, but not modifying the dep list is inconsistent.
  • So why does this only solve the symptom? If you look at the discussion in MSHADE-206 and MNG-5899 it refers to an old commit in Maven which introduced a hack with a simple cache which does not re-read POMs even if they have changed in-flight. There is no cache validation/eviction. Without haven't do any live testing and knowing how reasonable the cache performance gains are, one could consider dropping the cache. I always prefer consistent behavior over performance.

I would like to ask the following: Please branch off Maven master, drop the hacky cache and retry w/o this PR and let me know whether this fixes the issue already for you. If so, we need to reconsider the cache. Maybe this new parameter can be introduced, but only as @Deprecated because it fixes the symptom only.

@rfscholte Since you left a new comments on MNG-5899, do you think we could safely drop the cache and see whether we will see complains about performance drop?

Loading

@michael-o
Copy link
Member

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

A friendly reminder, I want to start the release by mid of next week.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 28, 2021

@michael-o I'm trying to see what happens. Will report here what I find.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 28, 2021

@michael-o @rfscholte
[Sorry, earlier I got it wrong. So I edited this comment]

What I think you meant:
If this hack in maven is removed the test I created for this merge request should "just pass", even without the code change.

Side note: This comment was removed in this commit apache/maven@d3ace78

What I did:

  1. I cloned the maven main source repo and removed the code fragment which was originally documented as a hack.
    https://github.com/nielsbasjes/maven/tree/MSHADE-326-RemoveHack
    nielsbasjes/maven@b52e10c
  2. I built maven with
    mvn -DdistributionTargetDir="$HOME/app/maven/apache-maven-4.0.x-SNAPSHOT" clean package.
    For this build I used maven 3.8.1 on Java 11.0.11 .
    This build passed all available tests.
  3. In my local copy of this merge request I reverted the code change and left the test intact.
  4. I built the maven-shade-plugin with
    export M2_HOME=$HOME/app/maven/apache-maven-4.0.x-SNAPSHOT
    ${M2_HOME}/bin/mvn verify -Prun-its
    I ran it without the invoker.parallelThreads to make the output better readable.

This last build fails over this test:

[INFO] Building: MSHADE-340_shadedTestJarArtifactAttached/pom.xml
[INFO] run post-build script verify.groovy
[INFO]   The post-build script did not succeed. assert originalUberJar.exists()
       |               |
       |               false
       /home/nbasjes/workspace/Apache/maven-shade-plugin/target/it/MSHADE-340_shadedTestJarArtifactAttached/uber/target/mshade-340-uber-1.0.jar
[INFO]           MSHADE-340_shadedTestJarArtifactAttached/pom.xml . FAILED (2.5 s)

Underlying error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-jar-plugin:3.2.0:jar (default) on project mshade-340-api: 
You have to use a classifier to attach supplemental artifacts to the project instead of replacing them.

Most notably the test I created (which fails on maven 3.8.1) now passed.

[INFO] Building: MSHADE-326-Hide-dependencies-from-reactor/pom.xml
[INFO] run post-build script verify.bsh
[INFO]           MSHADE-326-Hide-dependencies-from-reactor/pom.xml  SUCCESS (2.9 s)

So apparently without this hack indeed my test does what I want.

Please advise how to proceed.
Do you want me to put up a merge request for maven?

Loading

@michael-o
Copy link
Member

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

I will reintroduce the comment about the hack because it remains what it is: a hack!

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jun 29, 2021

So I tracked down the cause of this 340 test failing and fixed it.
I put up a merge request for maven apache/maven#483

Now this version of the plugin ONLY passes (because of the extra test I created) when built using maven that includes this change apache/maven#483

So I expect this build to fail at this point.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jul 2, 2021

Just to clarify the current status as I understand it:

  1. apache/maven#483
    fixes the real problem in Maven itself.
  2. #103
    fixes the MSHADE-340 test that fails under the changed maven.
  3. #26
    is essentially reduced to a new integration test that only verifies if everything now works correctly.
    So this will only pass with a "fixed" maven version.

One thing that worries me right now is the potential impact of existing projects.
The need for a fix in the above mentioned test is a clear indicator that there will be cases this can cause problems.

I do not know if this is a significant impact or just that existing configuration problems now surface.

@michael-o @rfscholte is there anything additional I can do to help make all of this a success?

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jul 2, 2021

I build several of my own projects with this updated maven version and they all passed without any hickups.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jul 2, 2021

I have now reintroduced the hack comment in master and 3.8.x. Will process your message later.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jul 4, 2021

  1. The fix from you is incomplete, at the end we likely need to revert d3ace78602405079d6416a63c13216568ba97995 (MNG-6638)
  2. Can you verify that adding the id also works with stock Maven w/o the fix?
  3. Makes sense

See also objections in apache/maven#69. We need to please both, invalidate cache when necessary, but use it as often as possible. The "hack" shouldn't stay forever. A better approach should be discussed after 4.0.0-alpha-1.

@nielsbasjes Can you work out a Maven IT which exposes the correct behavior and will fail for now. Then we will drop the hacks in a branch and will see how it performs?

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jul 4, 2021

@michael-o

  1. Yes, this is indeed a hard problem with a lot of side effects to think about.
  2. I already did that check (as I mentioned in the description) with both 3.8.1 and a maven that includes my patch; Note that it also passed the CI build.
  3. I'll leave this open for now.

I read in the comments that someone had a single project with over 5000 modules which in my experience sounds very extreme.
Perhaps a monorepo that is also a single build?
I checked the mentioned Apache Camel (586 pom.xml files) and some projects which I expected to have a large number of modules (just counted the pom.xml files): Flink (179), Hadoop (113), Nifi (592) and Spark (36)

So my main question is to what extend should maven support so many modules?
Even for very large projects having > 1000 modules seems like "too many".

I'll see what I can do at the maven end to make a test for this.

  1. I read about the algorithms in maven that determine the actual build ordering that are affected by this change.
    At this point I do not have enough knowledge about this to come up with a test that verifies this behavior to remain correct.
  2. As far as I can tell the only way is to add something that changes the reactor build ... like the test of this pull request.
    Simply migrating this (now failing) test to maven would make the maven-shade-plugin a part of the tests in the maven build.
    Is this ok ? Or do you have a better proposal on how to reproduce this?

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jul 4, 2021

My personal opinion: A broken cache should be removed or improved. @rfscholte Remove or try to solve.

Loading

@rfscholte
Copy link
Contributor

@rfscholte rfscholte commented Jul 5, 2021

IIUC this is caused by Maven 3 using the pom.xml of the reactor for both build and consume. With Maven 4 we've started to separate this, but currently not inside the reactor. As long as Maven can ensure that the consumer-pom won't introduce new reactor dependencies, it should be possible inside Maven.
I don't think this should be solved at plugin level as there are more that suffer from the same issue.

Loading

@nielsbasjes
Copy link
Contributor Author

@nielsbasjes nielsbasjes commented Jul 21, 2021

Hi @michael-o & @rfscholte,
I'm curious about the next steps.
Is there anything else I can do / try / write / test to make all of this a success?

Loading

@rfscholte
Copy link
Contributor

@rfscholte rfscholte commented Jul 21, 2021

As said, I expect this needs to be fixed in Maven Core. We're still working on this very tricky piece of code. I'd suggest to close this PR, close MSHADE-326 and create a ticket for MNG referring to MSHADE-326.
If you like extreme challenges, you could give it a try, otherwise you need to be patient until one of the Maven developers picks it up.

Loading

@michael-o
Copy link
Member

@michael-o michael-o commented Jul 21, 2021

Let's keep this and the ticket open until we are able to fix this in Maven. This PR will remind us that something is broken in Maven. As @rfscholte said, This isn't trivial, and cannot be addressed before 4.0.0-alpha-2 given that it takes time to understand, implement, test and not cause regressions.

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
6 participants