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

Use ChainedLocalRepositoryManager to support -Dmaven.repo.local.tail #43352

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

tiagobento
Copy link
Contributor

@tiagobento tiagobento commented Sep 17, 2024

Hi Quarkus team! 👋 My first PR here! (still draft as I didn't have time to go through all the contribution guidelines yet)


maven-resolver introduced Chained LRM a while back. This PR adds support to it, making it possible for Quarkus Maven Plugin to find dependencies on local repositories declared as tail.

Tested locally with a project I have and it worked great, but still need to research how to create tests for it and update the relevant parts of the documentation.

All feedback is appreciated. Thanks!

@quarkus-bot quarkus-bot bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Sep 17, 2024
@geoand
Copy link
Contributor

geoand commented Sep 18, 2024

Thanks a lot for the contribution!

@aloubyansky can you have a look?

@tiagobento
Copy link
Contributor Author

@aloubyansky Any pointers on how to test this? And on what parts of the documentation need to be updated?

@aloubyansky
Copy link
Member

I guess create a couple of local Maven repos in some temp dir and init the resolver, such as in https://github.com/quarkusio/quarkus/blob/main/independent-projects/bootstrap/maven-resolver/src/test/java/io/quarkus/bootstrap/resolver/maven/test/BootstrapMavenContextTestBase.java?

Copy link
Contributor Author

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

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

@aloubyansky Added tests and fixed a few bugs that the tests helped me catch :)

I took the liberty to make a small change to BootstrapMavenContextTestBase.java to make it easier to configure BootstrapMavenContext, and had to adapt another test (PreferPomsFromWorkspaceTest.java)

I gave some thought to the documentation changes, and maybe there isn't a need to update anything, given -Dmaven.repo.local.tail is a feature of Maven itself, so actually it would be expected that it works out of the box with Quarkus. From this perspective, this PR is more like a "bug fix" then adding new stuff.

Let me know what you think. I converted the PR to "ready for review" :)

Thanks!

@tiagobento tiagobento marked this pull request as ready for review September 18, 2024 18:44
Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

Thanks @tiagobento
Would you mind adding a test for multiple locations in the tail? Perhaps you want to use different artifacts for that. Also if the same version of the artifact is found in multiple locations, I suppose the first location in the list will win?
BTW, you could use txt artifacts instead of actual binaries for test artifacts.

public void testValidTailViaConfig() throws Exception {
final BootstrapMavenContext mvn = bootstrapMavenContextForProject("workspace-with-local-repo-tail",
BootstrapMavenContext.config()
.setLocalRepositoryTail(new String[] { M2_LOCAL }));
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why this one passes? In this case ignore availability will be true, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the baseline test, the "happy path"... You're correct that ignoreAvailability will be true by default. It works because the artifact is there, thus is resolved.

@tiagobento
Copy link
Contributor Author

Added two more tests:

  • testValidTailResolutionOrder; and
  • testValidTailMultiplicity

Fixed the ArrayList creation with a fixed length.


Kept the .jar extension since they're only empty files. I can change to .txt, though. Just let me know.

BootstrapMavenContext.config()
.setLocalRepositoryTail(new String[] { M2_FROM_REMOTE }));

assertNotNull(resolveOrgAcmeFooJar001(mvn));
Copy link
Member

Choose a reason for hiding this comment

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

@tiagobento could you please explain this one. ignoreAvailability is true. Is it availability of artifacts that should be ignored?

Copy link
Contributor Author

@tiagobento tiagobento Sep 19, 2024

Choose a reason for hiding this comment

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

Sure. So maybe the ignoreAvailability name kind of gives the wrong idea of what it actually does, and maybe a more explicit, albeit verbose, name would be ignoreRemoteAvailability? cc @cstamas

This flag is only applicable for dependencies that have been downloaded from a remote repository prior to being consumed as part of a tail local repository ("tracked", in Maven Resolver's terminology) . See the _remote.repositories file.

This particular test succeeds because maven.repo.local.tail.ignoreAvailability ends up being true, so even though org.acme:foo:jar:0.0.1 was supposedly initially resolved from a remote repository with id = some-repo-id, this fact is ignored, and it being available in the local repository suffices. When maven.repo.local.tail.ignoreAvailability is false, it fails. That case is exactly what is covered by testValidTailFromRemoteCheckingForAvailabilityViaSystemProp.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. The property name is indeed very confusing. It sounds like the meaning is "trust any remote repo" that provided an artifact.

Copy link
Contributor

Choose a reason for hiding this comment

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

"availability" in resolver means that tracked remote repository (by repoID) is available in the context artifact is asked for. So "remote" may come in as "remote repo ID is available in current context asking for artifact" :)

@aloubyansky
Copy link
Member

@tiagobento could you please squash the commits? Thanks.

@tiagobento
Copy link
Contributor Author

@aloubyansky Done. Thanks!

Copy link

🎊 PR Preview bddf9a3 has been successfully built and deployed to https://quarkus-pr-main-43352-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 19, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit e69ede4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 20, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5ae6c3b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/redis-client/deployment

io.quarkus.redis.deployment.client.devmode.RedisClientPreloadDevModeTestCase.testImportFileUpdateInDevMode - History

  • java.lang.RuntimeException: Unable to start Quarkus test resource class io.quarkus.redis.deployment.client.RedisTestResource - java.util.concurrent.CompletionException
java.util.concurrent.CompletionException: java.lang.RuntimeException: Unable to start Quarkus test resource class io.quarkus.redis.deployment.client.RedisTestResource
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320)
	at java.base/java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1807)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: java.lang.RuntimeException: Unable to start Quarkus test resource class io.quarkus.redis.deployment.client.RedisTestResource

@aloubyansky aloubyansky merged commit 14af184 into quarkusio:main Sep 20, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 20, 2024
@tiagobento
Copy link
Contributor Author

@aloubyansky Thanks for merging it!

Is there a chance to get this back-ported to 3.15? I see the tag is already created for 3.15.0, but maybe for 3.15.1? My team is waiting for 3.15 to be out so we can upgrade from 3.8. Having this feature on the LTS release would be very valuable to us 🙏 Let me know and I'll send the PR if that's ok. Thanks again!

@aloubyansky
Copy link
Member

I think it can be backported. I added a label, it'll be considered.
FYI, I opened #43420 as a follow up.

Thank you and congratulations on your first contribution to Quarkus! It's a nice one, I'll use it in our tests.

@tiagobento
Copy link
Contributor Author

@aloubyansky Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation triage/backport triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants