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-148] Don't attach exclusions that are already present #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philipl
Copy link

@philipl philipl commented Mar 5, 2019

Although I don't fully understand the scenarios where we re-evaluate
the exclusions of a dependency, I do know that if we do, we will add
them again, leading to a false indication that the dependency list has
changed, leading to a regeneration and re-evaluation of the list,
which then continues indefinitely, as the the same exclusions are
added again and again.

To address the leaf problem, let's ensure that we never add an
exclusion that is already present. That way, even if reevaluation
occurs, it will not result in changes.

Although I don't fully understand the scenarios where we re-evaluate
the exclusions of a dependency, I do know that if we do, we will add
them again, leading to a false indication that the dependency list has
changed, leading to a regeneration and re-evaluation of the list,
which then continues indefinitely, as the the same exclusions are
added again and again.

To address the leaf problem, let's ensure that we never add an
exclusion that is already present. That way, even if reevaluation
occurs, it will not result in changes.
Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

What about adding this logic inside
addExclusion ?

Can you add a test case as well?

@philipl
Copy link
Author

philipl commented Mar 5, 2019

addExclusion is generated code in core maven. That’s also why Exclusions don’t have an equals() implementation. I’d add a test case if I could come up with a repro case that triggered on its own. I added some discussion in the JIRA.

@philipl
Copy link
Author

philipl commented Mar 5, 2019

Note, that the fix in #15 looks like the fix for why exclusions get reevaluated. Even if that is merged, this fix should be taken too for correctness.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

needs a test

Copy link
Member

@maobaolong maobaolong left a comment

Choose a reason for hiding this comment

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

I'm +1 with this PR, hope to merge this before next release. I've patched and tested this PR, it can worked without stuck.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Still needs a test

neo-technology-build-agent pushed a commit to neo4j/neo4j that referenced this pull request Dec 14, 2020
…n shade bug

A bug in the Maven Shade Plugin may cause an infinite loop (livelock) while creating the dependency reduced pom on parallel builds
It seems to happen if the added dependencies are already transitive dependencies
Likely fixed by apache/maven-shade-plugin#16
@gnodet
Copy link
Contributor

gnodet commented Feb 2, 2021

I also have this issue when building Apache Camel with a parallel build, similar to the above example.
Given this looks like a threading issue, it's not always reproducible, so it's quite hard to think about a test which could reliably exercise the problem.

neo-technology-build-agent pushed a commit to neo4j/neo4j that referenced this pull request Jun 17, 2021
…n shade bug

A bug in the Maven Shade Plugin may cause an infinite loop (livelock) while creating the dependency reduced pom on parallel builds
It seems to happen if the added dependencies are already transitive dependencies
Likely fixed by apache/maven-shade-plugin#16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants