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

Fix test-distribution classpath re resolver #8187

Merged

Conversation

cstamas
Copy link
Contributor

@cstamas cstamas commented Jun 21, 2022

As currently it seems mixed versions of resolver artifacts are
present on classpath. Resolver 1.8.0 introduces a binary
breakage against 1.6.x.

Updated to bugfix 1.8.1 resolver

Anyway, this fixes the thing, as impl was actually coming in
as transitive dep of maven-resolver-provider, that is NOT
the same version as the rest of resolver dependencies.

Relying on transitive while fixing other versions is wrong.

image

As currently it seems mixed versions of resolver artifacts are
present on classpath. Resolver 1.8.0 introduces a binary
breakage.

Anyway, this fixes the thing, as impl was actually coming in
as transitive dep of maven-resolver-provider, that is NOT
the same version as the rest of resolver dependencies.

Relying on transitive while fixing other versions is wrong.
@cstamas cstamas mentioned this pull request Jun 21, 2022
And add exclusion to banned dependency, that is used when
running in SISU container.
<artifactId>maven-resolver-impl</artifactId>
<version>${maven.resolver.version}</version>
<exclusions>
<!-- Used when running in SISU container to manage beans lifecycle (of locking factories) -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/apache/maven-resolver/blob/master/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/synccontext/DefaultSyncContextFactory.java#L84

Sisu container is able to manage lifecycle, and on container shutdown will invoke this method. For your case is not needee, as you craft resolver manually, but also do not use any special kind of named locks that would require this cleanup (you use defaults).

@cstamas
Copy link
Contributor Author

cstamas commented Jun 21, 2022

Strange, this PR now made module far-far before change to break and seems completely unrelated

[ERROR] Failures: 
[ERROR]   HttpClientRedirectTest.testRedirectFailed:303 Unexpected exception type thrown ==> expected: <java.util.concurrent.ExecutionException> but was: <java.util.concurrent.TimeoutException>
[ERROR]   HttpClientRedirectTest.testRedirectFailed:303 Unexpected exception type thrown ==> expected: <java.util.concurrent.ExecutionException> but was: <java.util.concurrent.TimeoutException>
[INFO] 
[ERROR] Tests run: 655, Failures: 2, Errors: 0, Skipped: 15

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@joakime joakime merged commit 3fabe54 into jetty:jetty-10.0.x Jun 21, 2022
@cstamas
Copy link
Contributor Author

cstamas commented Jun 21, 2022

FTR, created eclipse/sisu.inject#58

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.

None yet

2 participants