Fix bug with mavenSettings in WrapperRunner while running tests locally #21890

ia3andy · 2021-12-02T15:06:40Z

[ERROR] Error executing Maven.
[ERROR] The specified user settings file does not exist: /Users/ia3andy/.m2/settings.xml

gastaldi · 2021-12-02T15:09:33Z

The current behavior is the same as Maven's, so I'm -1 to this change

aloubyansky · 2021-12-02T15:11:51Z

What kind of use-case is this fixing?

aloubyansky · 2021-12-02T15:12:52Z

I suppose there could be a relative path evaluation that could go wrong. But just to clarify.

gastaldi · 2021-12-02T15:15:53Z

I suppose there could be a relative path evaluation that could go wrong. But just to clarify.

IMHO if the -s flag was explicitly set to a non-existent file it should fail, so it would give the dev a hint of what's wrong. Silently ignoring it in this case is bad.

aloubyansky · 2021-12-02T15:19:22Z

If it's a relative path though, it could be resolved correctly against the current project but not the one it is passed to. So, perhaps, here we should be making sure we are always propagating an absolute path. But again it would be good to clarify what issue Andy ran into.

ia3andy · 2021-12-02T15:27:51Z

maven is working fine locally without this file.. this bug was introduced when we started reading maven.settings in WrapperRunner.

ia3andy · 2021-12-02T15:29:01Z

@gastaldi I don't think we should propagate the -s with a non existing file.

ia3andy · 2021-12-02T15:30:04Z

Here is when it started: #21673

gastaldi · 2021-12-02T15:32:38Z

I think the question is if the -s is explicitly set in the original command.

ia3andy · 2021-12-02T15:34:06Z

@gastaldi no it's using the properties, I am not setting it manually.

gastaldi · 2021-12-02T15:34:18Z

@gastaldi I don't think we should propagate the -s with a non existing file.

We should propagate what's been set in the original command. If it's an invalid path, that's the user's fault, the error should not be swallowed

gastaldi · 2021-12-02T15:35:34Z

So I guess the fix is to make sure that getMavenSettingsArg() returns a value ONLY if it's set. Checking if the file exists here is bad because it may lead to unexpected behavior when the command executes succesfully with a non-existent path

ia3andy · 2021-12-02T15:45:46Z

So I guess the fix is to make sure that getMavenSettingsArg() returns a value ONLY if it's set. Checking if the file exists here is bad because it may lead to unexpected behavior when the command executes succesfully with a non-existent pathII

I don't think we can can know if -s is set or not.

Maven is failing only when explicitly using -s with a non existent file. Here this is propagation, we shouldn't propagate something which makes the runner fail.

So maybe it's not the best solution, but it's better than the current code..

ia3andy · 2021-12-02T15:48:20Z

@gastaldi if you have a better solution to not propagate -s when you should not, let me know

gastaldi · 2021-12-02T15:49:25Z

Maven is failing only when explicitly using -s with a non existent file. Here this is propagation, we shouldn't propagate something which makes the runner fail.

It's okay to fail if the parameters are wrong, I think the bug is that -s is specified here but not in the original command

aloubyansky · 2021-12-02T15:50:34Z

We check whether maven.settings is set first. If it is we can check whether it's a valid path. If it's not, we should return null.
If maven.settings isn't set we can check whether -s was provided. If it was we should propagate it as an absolute path.

aloubyansky · 2021-12-02T15:51:33Z

We should always propagate it with -s because maven.settings is a Quarkus only property.

...projects/tools/devtools-testing/src/main/java/io/quarkus/devtools/testing/WrapperRunner.java

gastaldi · 2021-12-02T16:55:09Z

...projects/tools/devtools-testing/src/main/java/io/quarkus/devtools/testing/WrapperRunner.java

@@ -105,7 +107,7 @@ public static int run(Path projectDir, Wrapper wrapper) {
    private static String getMavenSettingsArg() {
        final String mavenSettings = System.getProperty("maven.settings");
        if (mavenSettings != null) {
-            return mavenSettings;
+            return Files.exists(Paths.get(mavenSettings)) ? mavenSettings : null;


Minor nitpick: use Path.of instead :)

Yeah it's a shortcut you are right. I think we can live with it :)

gastaldi · 2021-12-02T16:59:16Z

...projects/tools/devtools-testing/src/main/java/io/quarkus/devtools/testing/WrapperRunner.java

@@ -105,7 +107,7 @@ public static int run(Path projectDir, Wrapper wrapper) {
    private static String getMavenSettingsArg() {
        final String mavenSettings = System.getProperty("maven.settings");
        if (mavenSettings != null) {
-            return mavenSettings;
+            return Files.exists(Paths.get(mavenSettings)) ? mavenSettings : null;
        }
        return BootstrapMavenOptions.newInstance().getOptionValue(BootstrapMavenOptions.ALTERNATE_USER_SETTINGS);


BTW shouldn't we make this result an absolute path?

It's worth making sure the ALTERNATE_USER_SETTINGS is an absolute path.

quarkus-bot · 2021-12-02T18:22:45Z

Failing Jobs - Building `49dd31b`

Status	Name	Step	Failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

ia3andy · 2021-12-03T09:15:47Z

Error seems unrelated. @aloubyansky should we merge?

ia3andy requested a review from aloubyansky December 2, 2021 15:06

quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Dec 2, 2021

ia3andy force-pushed the fix-bug-wrapper-runner branch from 35c030a to 6eb683c Compare December 2, 2021 16:24

gastaldi reviewed Dec 2, 2021

View reviewed changes

...projects/tools/devtools-testing/src/main/java/io/quarkus/devtools/testing/WrapperRunner.java Outdated Show resolved Hide resolved

Fix bug with mavenSettings in WrapperRunner while running tests locally

49dd31b

ia3andy force-pushed the fix-bug-wrapper-runner branch from 6eb683c to 49dd31b Compare December 2, 2021 16:53

ia3andy requested a review from gastaldi December 2, 2021 16:53

aloubyansky approved these changes Dec 2, 2021

View reviewed changes

gastaldi reviewed Dec 2, 2021

View reviewed changes

gastaldi approved these changes Dec 2, 2021

View reviewed changes

gastaldi reviewed Dec 2, 2021

View reviewed changes

aloubyansky merged commit 0c752d9 into quarkusio:main Dec 3, 2021

quarkus-bot bot added this to the 2.6 - main milestone Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with mavenSettings in WrapperRunner while running tests locally #21890

Fix bug with mavenSettings in WrapperRunner while running tests locally #21890

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021 •

edited

Loading

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

gastaldi commented Dec 2, 2021 •

edited

Loading

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

gastaldi Dec 2, 2021

ia3andy Dec 3, 2021

gastaldi Dec 2, 2021

aloubyansky Dec 2, 2021

quarkus-bot bot commented Dec 2, 2021

ia3andy commented Dec 3, 2021

Fix bug with mavenSettings in WrapperRunner while running tests locally #21890

Fix bug with mavenSettings in WrapperRunner while running tests locally #21890

Conversation

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021 • edited Loading

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

gastaldi commented Dec 2, 2021 • edited Loading

ia3andy commented Dec 2, 2021

ia3andy commented Dec 2, 2021

gastaldi commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

aloubyansky commented Dec 2, 2021

gastaldi Dec 2, 2021

Choose a reason for hiding this comment

ia3andy Dec 3, 2021

Choose a reason for hiding this comment

gastaldi Dec 2, 2021

Choose a reason for hiding this comment

aloubyansky Dec 2, 2021

Choose a reason for hiding this comment

quarkus-bot bot commented Dec 2, 2021

Failing Jobs - Building 49dd31b

Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

ia3andy commented Dec 3, 2021

aloubyansky commented Dec 2, 2021 •

edited

Loading

gastaldi commented Dec 2, 2021 •

edited

Loading

Failing Jobs - Building `49dd31b`