fix: lookup absolute path to maven executable #4298

algomaster99 · 2021-11-19T15:10:32Z

Initially, it fetched "maven home" which is the path to the installation directory of maven. I, instead, try to look up the absolute path of the maven executable. I saw that the path was used to build class path and we can directly use the executable there.

I shall open this PR for review once I test the patch in sorald on a Windows machine.

algomaster99 · 2021-11-19T15:52:50Z

@slarse @MartinWitt Qodana tells what the error is, but I am not sure where to look at it. What is the source position of the error? Can we figure that out?

…alue correct now

MartinWitt · 2021-11-19T16:07:15Z

There was an update to qodana, and we had a solution planed. I pushed the (hopefully) fix to your branch as a temporary solution. I hope to create a PR this weekend fixing this with a GitHub action created by @SirYwell

MartinWitt · 2021-11-19T16:24:39Z

Qodana doesn't like the return of null instead of throwing an exception. Returning null instead of a String seems wrong. Maybe add an exception instead of null?

algomaster99 · 2021-11-19T20:18:59Z

src/main/java/spoon/support/compiler/SpoonPom.java

-			if (mvnHome == null) {
-				throw new SpoonException("M2_HOME must be initialized to use this MavenLauncher constructor.");
-			}


No longer needed because guessMavenHome throws an exception if it does not return a string.

algomaster99 · 2021-11-19T20:58:21Z

@MartinWitt

Qodana doesn't like the return of null instead of throwing an exception.

That shouldn't be the case because null is returned by a lot of methods in Spoon. Moreover, severity level of "Return of null" in the job is "warning" so I don't think that the tests would fail because of returning null. Regardless, I threw an exception instead, but it didn't work.

Opening this PR for review as I have tested this patch in Sorald on a Windows laptop. We would no longer need this workaround there.

MartinWitt · 2021-11-19T23:36:45Z

@I-Al-Istannen had the fix, and he is preparing a separate PR for this.

That shouldn't be the case because null is returned by a lot of methods in Spoon. Moreover, severity level of "Return of null" in the job is "warning" so I don't think that the tests would fail because of returning null.

Returning null is a bad smell, either we return Optional or throw Exceptions. Yes, current spoon code returns a lot of nulls, but this isn't a perfect style. We should try to get away from it.

algomaster99 · 2021-11-20T06:58:20Z

Returning null is a bad smell, either we return Optional or throw Exceptions

I agree that returning null is a bad smell. However, we had PRs passing quality checks after qodana was configured even though return null existed in the codebase.

We should try to get away from it.

Completely agree.

Thanks for the fix! Do you plan to review it now or should we wait for @I-Al-Istannen to submit the PR and then I will incorporate that patch into this PR?

slarse · 2021-11-20T10:42:43Z

src/main/java/spoon/support/compiler/SpoonPom.java

+		for (String dirname : System.getenv("PATH").split(File.pathSeparator)) {
+			File file = new File(dirname, executableName);
+			if (file.isFile() && file.canExecute()) {
+				return file.getAbsolutePath();


This doesn't seem right to me. "Maven home" is the path to the Maven install directory, not the path to the Maven executable. For example, on my system the executable is at /opt/maven/bin/mvn, but Maven home is /opt/maven.

The prior version of this script used the Maven executable to parse Maven home from the output of mvn --version. I would probably still do that, it's the most foolproof way of doing it.

Does this actually work for running the MavenLauncher?

I would also advice extracting finding the executable into a separate method ('Path getExecutablePath(String name)`) or something like that, and throw the "can't find executable" exception from there instead.

@slarse Maven home and Maven executable are indeed different things. I tried to look up Maven home in PATH, but it did not exist, so I checked how the path to maven home is being used in the codebase. I found out that there were only two places.

Used to generate a classpath file. I did not meticulously read the method definition, but it seems like it tries to build the classpath if it doesn't exist.

Used to run checkstyle in this test, although it is ignored.

From these two usages, I felt that looking up the path to executable would suffice, and maven home is not actually needed.

Does this actually work for running the MavenLauncher?

Yes, it does, and this corroborated my patch.

I know it's public API, but I don't think this specific API would be used. One may directly use the MavenLauncher if needed.

I decided to keep the intended behaviour. Pushed the changes for it.

…-to-mvn

algomaster99 · 2021-11-23T12:34:57Z

@monperrus ping for reviews.

algomaster99 · 2021-11-23T14:40:48Z

@MartinWitt I am extracting your changes to the workflow in a separate PR.

MartinWitt · 2021-11-23T14:43:26Z

We have a PR already here, #4301. I thought @slarse is merging it after his comment.

…return value correct now" This reverts commit b885ad8.

This reverts commit af7756f.

algomaster99 · 2021-11-23T15:42:01Z

src/test/java/spoon/MavenLauncherTest.java

+	public void testGuessMavenHome() {
+		// contract: it should correctly fetch path to maven home
+		String pathToMavenHome = SpoonPom.guessMavenHome();
+		File mavenHome = new File(pathToMavenHome);
+		assertTrue(mavenHome.exists());
+		assertTrue(mavenHome.isDirectory());


Please note that I also removed using reflection to invoke guessMavenHome.

algomaster99 · 2021-11-23T15:52:09Z

@monperrus ping for merge.

slarse · 2021-11-23T22:12:01Z

We have a PR already here, #4301. I thought @slarse is merging it after his comment.

Well, that was the idea. But I forgot and this week is a bit heavy.

Will be AWOL until the weekend, someone else tag in for this PR or I'll get to it on Saturday.

MartinWitt · 2021-11-24T00:18:18Z

src/main/java/spoon/support/compiler/SpoonPom.java

+		String executableName;
+		if (System.getProperty("os.name").contains("Windows")) {
+			executableName = "mvn.cmd";
+		} else {


Does this work on Mac? You removed the Mac case?

Yes. I tested it here.

I get the absolute path to the maven executable as I have done in this PR.

Execute </absolute/path/to/maven> -version.

MartinWitt · 2021-11-24T00:22:07Z

src/test/java/spoon/MavenLauncherTest.java

+		// contract: it should correctly fetch path to maven home
+		String pathToMavenHome = SpoonPom.guessMavenHome();
+		File mavenHome = new File(pathToMavenHome);
+		assertTrue(mavenHome.exists());


Can you add here a String explaining what failed? The error message of assertTrue is kinda bad.

I throw a SpoonException in guessMavenHome (message: Couldn't find path to maven home.). We will never reach this line if the test fails.

Is this the correct way to write this test because it doesn't make sense to assertDoesNotThrow?

Agree, assertTrue with message is a best practice, but in this case we can proceed.

Keeping trace of this in good first issue #4307

I've been thinking to introduce the full range of Hamcrest matchers into Spoon, they truly bring some wonderful expressiveness to tests. The perfect matcher here would be aFileNamed().

Completely agree. Moreover, it would also help refactor our tests. We currently use Junit4, Junit5, and hamcrest. It's best to have just one library handling it all.

Hamcrest is just the matchers and assertThat, it's not a testing framework. So, it would be Junit5 + Hamcrest matchers.

monperrus · 2021-11-25T10:07:25Z

Thanks a lot @algomaster99 for the solution and @slarse @MartinWitt for the review.

algomaster99 added 4 commits November 19, 2021 16:00

Lookup absolute path to maven executable

9f9382a

Build classpath using maven executable

67581d4

Refactor test for fetching maven binary

6af594b

Remove unused import

282f1f6

chore(qodana): add if: always because qodana handles process return v…

b885ad8

…alue correct now

Throw exception instead of returning null

1ac0ce7

algomaster99 commented Nov 19, 2021

View reviewed changes

algomaster99 marked this pull request as ready for review November 19, 2021 20:58

Ignore process return code from qodana

af7756f

slarse reviewed Nov 20, 2021

View reviewed changes

slarse changed the title ~~chore: lookup absolute path to maven executable~~ fix: lookup absolute path to maven executable Nov 20, 2021

algomaster99 added 3 commits November 23, 2021 12:40

Locate path to maven home using absolute path to mvn executable

4e618d2

Merge branch 'path-to-mvn' of github.com:algomaster99/spoon into path…

62dee64

…-to-mvn

Fix contract and imports

0f2eec5

algomaster99 mentioned this pull request Nov 23, 2021

review: fix: Ignore Qodana action exit code #4301

Merged

algomaster99 added 3 commits November 23, 2021 16:31

Merge branch 'master' into path-to-mvn

33f16e8

Revert "chore(qodana): add if: always because qodana handles process …

011b2b9

…return value correct now" This reverts commit b885ad8.

Revert "Ignore process return code from qodana"

0ae92f3

This reverts commit af7756f.

algomaster99 commented Nov 23, 2021

View reviewed changes

MartinWitt reviewed Nov 24, 2021

View reviewed changes

monperrus mentioned this pull request Nov 25, 2021

good first issue: check if assertTrue has a message argument #4307

Open

monperrus merged commit 5842bfd into INRIA:master Nov 25, 2021

algomaster99 deleted the path-to-mvn branch November 25, 2021 19:36

algomaster99 mentioned this pull request Nov 26, 2021

No need of using M2_HOME environment variable as fallback to find path to maven executable in Windows #4288

Open

algomaster99 mentioned this pull request Dec 9, 2021

chore: upgrade Spoon to 10.0.1-beta-2 ASSERT-KTH/sorald#676

Merged

monperrus mentioned this pull request Mar 14, 2022

WIP release: next release #4636

Closed

slarse mentioned this pull request Jul 9, 2022

[Bug] Bug descriptionspoon.SpoonException: Maven invocation failed to build a classpath. #4658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: lookup absolute path to maven executable #4298

fix: lookup absolute path to maven executable #4298

algomaster99 commented Nov 19, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

MartinWitt commented Nov 19, 2021 •

edited

Loading

MartinWitt commented Nov 19, 2021

algomaster99 Nov 19, 2021

algomaster99 commented Nov 19, 2021

MartinWitt commented Nov 19, 2021 •

edited

Loading

algomaster99 commented Nov 20, 2021

slarse Nov 20, 2021 •

edited

Loading

algomaster99 Nov 22, 2021 •

edited

Loading

algomaster99 Nov 23, 2021

algomaster99 commented Nov 23, 2021

algomaster99 commented Nov 23, 2021

MartinWitt commented Nov 23, 2021

algomaster99 Nov 23, 2021

algomaster99 commented Nov 23, 2021

slarse commented Nov 23, 2021

MartinWitt Nov 24, 2021

algomaster99 Nov 24, 2021

MartinWitt Nov 24, 2021

algomaster99 Nov 24, 2021 •

edited

Loading

monperrus Nov 25, 2021

slarse Nov 25, 2021

algomaster99 Nov 25, 2021

slarse Nov 25, 2021 •

edited

Loading

monperrus commented Nov 25, 2021

fix: lookup absolute path to maven executable #4298

fix: lookup absolute path to maven executable #4298

Conversation

algomaster99 commented Nov 19, 2021 • edited Loading

algomaster99 commented Nov 19, 2021 • edited Loading

MartinWitt commented Nov 19, 2021 • edited Loading

MartinWitt commented Nov 19, 2021

Choose a reason for hiding this comment

algomaster99 commented Nov 19, 2021

MartinWitt commented Nov 19, 2021 • edited Loading

algomaster99 commented Nov 20, 2021

slarse Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

algomaster99 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Nov 23, 2021

algomaster99 commented Nov 23, 2021

MartinWitt commented Nov 23, 2021

Choose a reason for hiding this comment

algomaster99 commented Nov 23, 2021

slarse commented Nov 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slarse Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

monperrus commented Nov 25, 2021

algomaster99 commented Nov 19, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

MartinWitt commented Nov 19, 2021 •

edited

Loading

MartinWitt commented Nov 19, 2021 •

edited

Loading

slarse Nov 20, 2021 •

edited

Loading

algomaster99 Nov 22, 2021 •

edited

Loading

algomaster99 Nov 24, 2021 •

edited

Loading

slarse Nov 25, 2021 •

edited

Loading