Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local #15530

aloubyansky · 2021-03-08T09:40:21Z

Otherwise devtools tests will be failing to locate the 999-SNAPSHOT artifacts.

aloubyansky · 2021-03-08T09:44:23Z

@famod I actually can't recall what depends on maven.repo. Could you remind me please?

aloubyansky · 2021-03-08T10:06:16Z

Ok, there is one other issue.

build-parent/pom.xml

famod · 2021-03-08T12:52:26Z

@famod I actually can't recall what depends on maven.repo. Could you remind me please?

Well, it was first introduced here (back in 2018): 83a48e9

Occurences in current code:

grep -R --exclude-dir=target --include='*.java' '"maven.repo"' .
./test-framework/maven/src/main/java/io/quarkus/maven/it/MojoTestBase.java:        String repo = System.getProperty("maven.repo");
./test-framework/maven/src/main/java/io/quarkus/maven/it/verifier/RunningInvoker.java:        String repo = System.getProperty("maven.repo");

famod · 2021-03-08T12:55:44Z

I wonder if this should be set in Codestarts etc. as well?

PS: The more system properties we have to set, the more appealing becomes an automatic "bridging" via a Maven extension (the discussion we had just recently).

famod · 2021-03-08T12:59:43Z

I also wonder whether the localRepository system property should be used instead, which is already set by surefire: #11449 (comment)

aloubyansky · 2021-03-08T13:03:06Z

I also wonder whether the localRepository system property should be used instead, which is already set by surefire: #11449 (comment)

Of course, If it's set by surefire. Sorry, I missed it. I should adjust the bootstrap maven resolver to pick it up directly.

aloubyansky · 2021-03-08T13:04:17Z

@famod I actually can't recall what depends on maven.repo. Could you remind me please?

Well, it was first introduced here (back in 2018): 83a48e9

Occurences in current code:
grep -R --exclude-dir=target --include='*.java' '"maven.repo"' .
./test-framework/maven/src/main/java/io/quarkus/maven/it/MojoTestBase.java:        String repo = System.getProperty("maven.repo");
./test-framework/maven/src/main/java/io/quarkus/maven/it/verifier/RunningInvoker.java:        String repo = System.getProperty("maven.repo");

Ah, thanks. We could just use one property instead of multiple.

famod · 2021-03-08T13:12:15Z

TBH, I do get that it's important that the right local repo ist used, but isn't there more to it?
I mean, it seems the remote (!) repos from the custom settings.xml are not being picked up...

famod · 2021-03-08T13:14:34Z

Regarding all these properties: I'm +10 to reduce the number of them, to like primarily maven.repo.local and secondarily (as a fallback) localRepository.

aloubyansky · 2021-03-08T13:16:41Z

TBH, I do get that it's important that the right local repo ist used, but isn't there more to it?
I mean, it seems the remote (!) repos from the custom settings.xml are not being picked up...

Perhaps, I missed something. What's the evidence of that?

famod · 2021-03-08T13:23:41Z

TBH, I do get that it's important that the right local repo ist used, but isn't there more to it?
I mean, it seems the remote (!) repos from the custom settings.xml are not being picked up...

Perhaps, I missed something. What's the evidence of that?

This: #15513 (comment)
It's trying to download from repo.maven.apache.org but should be using maven-central.storage-download.googleapis.com from https://github.com/quarkusio/quarkus/blob/master/.github/mvn-settings.xml instead, no?

aloubyansky · 2021-03-08T13:28:17Z

Sorry, which log is this quoting?

famod · 2021-03-08T14:16:07Z

https://github.com/quarkusio/quarkus/pull/15513/checks?check_run_id=2043450734
Better download the log though. Btw, there are also successful downloads from central in that log...

aloubyansky · 2021-03-08T17:02:25Z

Thanks @famod Apparently the env vars aren't propagated by the Maven invoker. It seems like we've always had this issue to me. I'll include the fix in this PR.

famod · 2021-03-08T17:18:09Z

@aloubyansky beware though that JVM_TEST_MAVEN_OPTS is passed to mvn explicitly (as arguments):

quarkus/.github/workflows/ci-actions-incremental.yml

Line 347 in 0ec090d

    
           run: mvn $JVM_TEST_MAVEN_OPTS install -pl 'integration-tests/maven' ${{ needs.build-jdk11.outputs.gib_args }}

It's only an env var because it makes it easier to handle in a GH workflow.

aloubyansky · 2021-03-09T15:18:58Z

I removed maven.repo in favor of maven.repo.local and added maven.settings.

build-parent/pom.xml

integration-tests/maven/src/test/java/io/quarkus/maven/it/AddExtensionIT.java

famod · 2021-03-09T17:15:40Z

test-framework/maven/src/main/java/io/quarkus/maven/it/verifier/RunningInvoker.java

@@ -119,6 +120,7 @@ public MavenProcessInvocationResult execute(List<String> goals, Map<String, Stri
            //just running GC
            request.setMavenOpts("-Xmx128m");
        }
+        MojoTestBase.passUserSettings(request);


In conjunction with https://github.com/quarkusio/quarkus/pull/15530/files#r590560562, maybe that method should be located in some util class in the verifier package instead of the test base class?

I'm also wondering whether MavenProcessInvoker should do the same.

That's pretty much where this is used though. I liked the idea of wrapping the invoker. And the RunningInvoker is already overriding execute.

I don't see MavenProcessInvoker being used directly though.

…m property and the maven settings as mave.settings property

famod

LGTM now!

I'm a bit puzzled by localRepository though, but I'll switch to Zulip to clarify that.

aloubyansky requested a review from famod March 8, 2021 09:40

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Mar 8, 2021

aloubyansky mentioned this pull request Mar 8, 2021

Bump sentry-jul from 4.2.0 to 4.3.0 #15513

Merged

aloubyansky commented Mar 8, 2021

View reviewed changes

build-parent/pom.xml Outdated Show resolved Hide resolved

aloubyansky force-pushed the maven-repo-local-test-plugins branch from 87a3da2 to b57dc39 Compare March 8, 2021 10:28

aloubyansky force-pushed the maven-repo-local-test-plugins branch from b57dc39 to 16ea369 Compare March 8, 2021 17:36

quarkus-bot bot added area/jbang Issues related to when using jbang.dev with Quarkus area/maven area/testing labels Mar 8, 2021

aloubyansky force-pushed the maven-repo-local-test-plugins branch from 16ea369 to f499aee Compare March 9, 2021 15:17

aloubyansky changed the title ~~Pass the local maven repo path to the tests as maven.repo.local system property~~ Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local Mar 9, 2021

famod reviewed Mar 9, 2021

View reviewed changes

build-parent/pom.xml Show resolved Hide resolved

famod reviewed Mar 9, 2021

View reviewed changes

integration-tests/maven/src/test/java/io/quarkus/maven/it/AddExtensionIT.java Outdated Show resolved Hide resolved

famod reviewed Mar 9, 2021

View reviewed changes

Pass the local maven repo path to the tests as maven.repo.local syste…

66cd0da

…m property and the maven settings as mave.settings property

aloubyansky force-pushed the maven-repo-local-test-plugins branch from f499aee to 66cd0da Compare March 9, 2021 20:26

quarkus-bot bot added area/kotlin area/scala labels Mar 9, 2021

famod approved these changes Mar 9, 2021

View reviewed changes

aloubyansky merged commit e0ad931 into quarkusio:master Mar 10, 2021

quarkus-bot bot added this to the 1.13 - master milestone Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local #15530

Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local #15530

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021 •

edited

Loading

famod commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

famod commented Mar 8, 2021 •

edited

Loading

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021 •

edited

Loading

aloubyansky commented Mar 9, 2021

famod Mar 9, 2021

famod Mar 9, 2021

aloubyansky Mar 9, 2021

aloubyansky Mar 9, 2021

famod left a comment

Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local #15530

Add maven.settings property to the test plugins and remove maven.repo in favor of maven.repo.local #15530

Conversation

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021 • edited Loading

famod commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

famod commented Mar 8, 2021 • edited Loading

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021

aloubyansky commented Mar 8, 2021

famod commented Mar 8, 2021 • edited Loading

aloubyansky commented Mar 9, 2021

famod Mar 9, 2021

Choose a reason for hiding this comment

famod Mar 9, 2021

Choose a reason for hiding this comment

aloubyansky Mar 9, 2021

Choose a reason for hiding this comment

aloubyansky Mar 9, 2021

Choose a reason for hiding this comment

famod left a comment

Choose a reason for hiding this comment

famod commented Mar 8, 2021 •

edited

Loading

famod commented Mar 8, 2021 •

edited

Loading

famod commented Mar 8, 2021 •

edited

Loading