Skip to content

Consider adding dependency convergence detection #13990

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

Open
sjohnr opened this issue Oct 10, 2023 · 10 comments
Open

Consider adding dependency convergence detection #13990

sjohnr opened this issue Oct 10, 2023 · 10 comments
Labels
in: build An issue in the build type: enhancement A general enhancement

Comments

@sjohnr
Copy link
Contributor

sjohnr commented Oct 10, 2023

We should consider adding dependency convergence detection to our build to prevent issues like gh-13843. For example, the following dependencies must agree on the version of com.nimbusds:nimbus-jose-jwt.

  • spring-security-oauth2-client -> com.nimbusds:oauth2-oidc-sdk -> com.nimbusds:nimbus-jose-jwt
  • spring-security-oauth2-jose -> com.nimbusds:nimbus-jose-jwt

See comment and related issue for more information.

@sjohnr sjohnr added in: build An issue in the build type: enhancement A general enhancement labels Oct 10, 2023
@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented Oct 31, 2023

@sjohnr if there aren't more dependencies that have to be checked, this might have been resolved via #14047. I'll close this but we can reopen if needed.

@sjohnr
Copy link
Contributor Author

sjohnr commented Nov 2, 2023

@marcusdacoregio based on this comment, I think the idea was to add general purpose detection when convergence issues arise. But I don't have too much experience with that. gh-14047 definitely solves the most immediate problem, so I'll let others weigh in on whether more general detection is still needed. @ThomasKasene

@ThomasKasene
Copy link
Contributor

I'll weigh in my own experience - make of it what you will 😄

My team maintains a large number of Spring Boot applications which all use Spring Security for OAuth 2.0 stuff. When Boot 3.1.1 (I think) was released, Dependabot gave us a bunch of PRs to upgrade, but all the builds broke because we use the maven-enforcer-plugin to detect dependency convergence errors. The culprit was, of course, the fact that spring-security-oauth2-jose and oauth2-oidc-sdk depended on different versions of nimbus-jose-jwt.

We could have overridden the versions of the offending dependency in all the apps, and that would've been the "correct" thing to do. Doing so would've meant having to look out for changes/fixes in Spring Security to this issue (like the one provided through #13843). In the meantime, we'd have to manage any Dependabot PRs about nimbus-jose-jwt in all the apps, which would possibly all be invalid anyway because both Spring Security and oauth2-oidc-sdk use very old versions of nimbus-jose-jwt.

In the end, a general lack of bandwidth forced us to jump to plan B, which was simply to not upgrade to Spring Boot 3.1.1 at all. The fix didn't arrive until 3.1.5, so our adoption rate crashed down to zero in the meantime.

So yeah, a dependency convergence error on this exact artifact has happened before, and perhaps the Nimbus stuff is particularly prone to it because their release cycles are so out of sync. But the idea was to get something in place to prevent these kinds of issues from happening at all in the future. 😃

@marcusdacoregio
Copy link
Contributor

Thanks, @ThomasKasene. I'll keep this open and label it as ideal for contribution because I think it can be worth adding that to prevent such problems. It would be great if someone could investigate what would be needed to apply the plugin that you mentioned.

@marcusdacoregio marcusdacoregio added the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Nov 13, 2023
@andreilisa
Copy link

Hello @marcusdacoregio, can I take this one ?

@marcusdacoregio
Copy link
Contributor

Hello, @andreilisa. Yes, the issue is yours.

I'd start by adding the plugin to the build and check what dependencies should be aligned to have a successful compilation.
Additionally, we might even be able to remove this if the plugin can detect such a setup.

@andreilisa
Copy link

Yes got it, I will ask here if I have additional questions.

@andreilisa
Copy link

andreilisa commented Dec 5, 2023

Hey @marcusdacoregio, after stating to ExcludeDependencies.

enforce {
	rule(enforcer.rules.DependencyConvergence)
	rule(enforcer.rules.ExcludeDependencies) { r ->
		r.exclude("org.slf4j:slf4j-api:1.7.26")
		r.exclude("org.slf4j:slf4j-api:1.7.25")
		r.exclude("com.puppycrawl.tools:checkstyle:9.3")
		r.exclude("com.puppycrawl.tools:checkstyle:8.33")
		r.exclude("net.bytebuddy:byte-buddy:1.12.21")
		r.exclude("org.junit.jupiter:junit-jupiter-api:5.10.0")
//		r.exclude("commons-collections:commons-collections:3.2.1")
	}
}

I get next one error message and I can`t understand where is the problem:

Could not resolve all files for configuration ':spring-security-config:apiDependenciesMetadataCopy'.
> Could not find org.springframework:spring-aop:.
  Required by:
      project :spring-security-config
      project :spring-security-config > project :spring-security-core

But if I comment
// r.exclude("org.junit.jupiter:junit-jupiter-api:5.10.0")
I will get next one message:

Could not resolve all dependencies for configuration ':spring-security-acl:testRuntimeClasspathCopy'.
> Conflict found for the following module:
    - org.junit.jupiter:junit-jupiter-api between versions 5.10.1 and 5.10.0

Can you have a look at dependency_convergence_detection ?

@marcusdacoregio
Copy link
Contributor

Hi, @andreilisa.

It seems that the plugin performs the check in every configuration available, where, ideally, I think it should only check the compileClasspath in our case.

@andreilisa
Copy link

andreilisa commented Dec 7, 2023

@marcusdacoregio, Related PR 14256 ?

@marcusdacoregio marcusdacoregio removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: build An issue in the build type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants