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

[JBPM-9860] fixing conflicts between transitive dependency artifact resolution #223

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

Conversation

Hillrunner2008
Copy link

No description provided.

@kie-ci
Copy link

kie-ci commented Aug 12, 2021

Can one of the admins verify this patch?

4 similar comments
@kie-ci
Copy link

kie-ci commented Aug 12, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 12, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 12, 2021

Can one of the admins verify this patch?

@kie-ci
Copy link

kie-ci commented Aug 12, 2021

Can one of the admins verify this patch?

@Hillrunner2008
Copy link
Author

The change will end up deferring to this method in EmbeddedPomParser.java (https://github.com/kiegroup/kie-soup/blob/main/kie-soup-maven-utils/kie-soup-maven-integration/src/main/java/org/appformer/maven/integration/embedder/EmbeddedPomParser.java#L41) and at this location the MavenProject::getDependencies method (https://github.com/apache/maven/blob/maven-3.3.9/maven-core/src/main/java/org/apache/maven/project/MavenProject.java#L281). This core maven code correctly removes the conflicts from the dependencies.

@elguardian
Copy link
Member

Hi @Hillrunner2008
This code is really used in a broad number of situations (including editors in business central). In this case we need to resolve transitive dependencies as well.
I did left a comment in the jira of this issue.
I think that computing the effective pom is the way to go (also fixes another situatioin regarding offline stuff)
https://issues.redhat.com/browse/JBPM-9829

Please check it and let me know your thoughts . The change is already in 7.59 snapshot.

@Hillrunner2008
Copy link
Author

Hi @Hillrunner2008
This code is really used in a broad number of situations (including editors in business central). In this case we need to resolve transitive dependencies as well.
I did left a comment in the jira of this issue.
I think that computing the effective pom is the way to go (also fixes another situatioin regarding offline stuff)
https://issues.redhat.com/browse/JBPM-9829

Please check it and let me know your thoughts . The change is already in 7.59 snapshot.

The change I made does not remove transitive dependencies, so I'm a little unclear on what you mean. The change only resolves conflicts in the dependency graph. There is no "effective pom" calculation that should ever include conflicts. Maybe I'm missing some context, but in what possible way would having the unresolved graph be useful in one of these many locations you are concerned about impacting? The business central editor and all other relevant code would I believe want to resolve classpath correctly. I think the original author of the code had the intention you are describing (i.e. include transitive dependencies), but they didn't completely think through the implications for a simple traversal and aggregation without conflict resolution.

@mareknovotny mareknovotny changed the title Proposed fix for https://issues.redhat.com/browse/JBPM-9860 [JBPM-9860] fixing conflicts between transitive dependency artifact resolution Oct 18, 2021
@mareknovotny
Copy link
Member

ok to test

@mareknovotny
Copy link
Member

jenkins do fdb

@sonarcloud
Copy link

sonarcloud bot commented Oct 18, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mareknovotny
Copy link
Member

jenkins do fdb

1 similar comment
@bacciotti
Copy link
Contributor

jenkins do fdb

@Hillrunner2008
Copy link
Author

Hi @mareknovotny, @mbiarnes, @elguardian, and @bacciotti,
I submitted this PR a few years ago. At the time, I had personal knowledge of how the bug in question was negatively impacting two different customers. As I explained at the time, it was the effort to prepackage kjar dependencies in an immutable image that led to the discovery of the difference between maven's dependency resolution and the projects internal utility. The side effect of the issue was failed kjar deployments in the immutable containers even when they had the proper dependencies prepackaged.

I definitely never cared for the runtime resolution of classpath from maven repos (instead of binaries like traditional fat jar or war/WEB-INF/lib), but if the use of this utility is going to continue to exist in any supported version of a downstream project I'd imagine merging this PR still would still be beneficial.

I believe I've made the case for this as strongly as I can. However, if there's no movement on this PR the next time I check, I'll plan to close it out as its clearly losing relevance.

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.

5 participants