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

Assure Maven 3.1.0 compatibility and fix dependencies #397

Merged
merged 4 commits into from
Dec 26, 2021
Merged

Assure Maven 3.1.0 compatibility and fix dependencies #397

merged 4 commits into from
Dec 26, 2021

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented May 4, 2019

#391 reported an incompatibility of the spotless-maven-plugin with Maven 3.3.9.
The Spotless Maven ReadMe claims that the spotless-maven-plugin is compatible with Maven 3.1.0.
Research suggested that the problem is related to version conflicts regarding provided dependencies with other plugins. A similar issue occurred in the Spotbugs Maven plugin.

Due to Maven dependency mediation, it seems incorrect to use newer dependencies than required and provided by Maven 3.1.0.

The original problem reported in #391 could not be reproduced.
But problems with Maven 3.1.0 showed that the Maven System class loader is a transitive dependency of the Spotless plugin, which should not be the case. Further investigation showed that this was most likely not caused by spotless.

Current PR just improves Maven plugin test and dependencies.

@nedtwigg
Copy link
Member

nedtwigg commented May 6, 2020

Since a full year has elapsed, I'm guessing this will end up not getting merged. Happy to reopen or discuss further if you'd like to revisit.

@nedtwigg nedtwigg closed this May 6, 2020
@fvgh
Copy link
Member Author

fvgh commented Nov 1, 2021

The core problems addressed by this PR remains:

  1. Spotless does not check with the minimum Maven version
  2. Spotless mixes provided Aether version with own Aether version

@fvgh fvgh reopened this Nov 1, 2021
@nedtwigg
Copy link
Member

nedtwigg commented Nov 1, 2021

I don't know anything about maven. If you'd like to merge this PR @fvgh feel free, but probably best to first request a review from lutovich in case they've got any opinions.

@fvgh fvgh changed the base branch from master to main November 2, 2021 19:49
@fvgh fvgh changed the title Assure Maven 3.1.0 compatibility Assure Maven 3.1.0 compatibility and fix dependencies Nov 2, 2021
@fvgh fvgh requested a review from lutovich November 2, 2021 21:59
@fvgh
Copy link
Member Author

fvgh commented Nov 2, 2021

@lutovich I am afraid I am not a Maven expert myself. I started this PR, since the initial #391 description suggested some spotless-eclipse-base problems with SLF4J and the underlying Maven. But this was not the case. The problem I see with the Plexus Classworld should not only affect Maven 3.1.0, but also higher version. Can you have a look? It is not urgent, since the issue is anyhow open since many month.

Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@fvgh thank you for investigating this problem! It does look very complex. I added one comment on this PR.

@@ -69,7 +69,9 @@ dependencies {
implementation "com.diffplug.spotless:spotless-lib-extra:${libVersion}"
}

implementation "org.codehaus.plexus:plexus-resources:${VER_PLEXUS_RESOURCES}"
implementation("org.codehaus.plexus:plexus-resources:${VER_PLEXUS_RESOURCES}") {
exclude group: 'classworld', module: 'classworld'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it "classworld" or "classworlds"?
I think the chain of transitive dependencies goes like this:

org.codehaus.plexus:plexus-resources:jar:1.0.1:compile
+- org.codehaus.plexus:plexus-utils:jar:3.0.8:compile
\- org.codehaus.plexus:plexus-container-default:jar:1.0-alpha-9-stable-1:compile
   \- classworlds:classworlds:jar:1.1-alpha-2:compile

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads-up. I need another look...

Copy link
Member Author

Choose a reason for hiding this comment

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

I am afraid the problem I have found during investigation of #391 is sporadic, hence I assumed that my findings fixed the problem. So to conclude:

gradle.properties Show resolved Hide resolved
@lutovich
Copy link
Contributor

lutovich commented Nov 3, 2021

Would it be okay to bump plexus-resources from 1.0.1 to 1.2.0? The latter does not depend on plexus-container-default and does not have classworlds as a transitive dependency.

@fvgh fvgh marked this pull request as draft November 3, 2021 19:06
@fvgh
Copy link
Member Author

fvgh commented Nov 3, 2021

Would it be okay to bump plexus-resources from 1.0.1 to 1.2.0? The latter does not depend on plexus-container-default and does not have classworlds as a transitive dependency.

This was also an idea. But I found the reasoning behind the change in plexus-resources #2 unrelated to the issue I found. So I just want to avoid regression for now. Anyhow, I have to have another look about the reproducibility of the error and the assumed solution.

@fvgh fvgh force-pushed the issue_391 branch 2 times, most recently from 5b83ed7 to b2b956b Compare December 26, 2021 14:34
@fvgh fvgh marked this pull request as ready for review December 26, 2021 15:29
@fvgh
Copy link
Member Author

fvgh commented Dec 26, 2021

@lutovich Please have a final look. All the changes are just enhancements, but not bug fixes.

@fvgh fvgh merged commit eddfc38 into main Dec 26, 2021
@fvgh fvgh deleted the issue_391 branch December 26, 2021 21:22
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.

3 participants