Improve Maven invocations: adjust maven.executable settings & use pushd/popd instead of -f

[anthonyvdotbe]

Describe the bug Instead of doing something like mvn install -f C:\path\to\pom.xml, the extension should always open the terminal in the project's directory and just do mvn install (or else, do something like pushd C:\path\to; mvn install; popd)

To Reproduce

1. Clone code from https://github.com/Microsoft/todo-app-java-on-azure.git
2. Open the cloned project in VS Code
3. Make sure "terminal.integrated.shell.windows" is set to PowerShell
4. Right click on the project item and select a command, such as package

Notes:

1. to reproduce, the terminal will need to open in a different project than the project above. I don't know how the extension decides in which directory to open the terminal though
2. there are other cases where using -f doesn't work. For example, when using Checkstyle's suppression filter, it looks for the suppressions.xml file relative to the current working directory. So when running in the project's directory things work as expected, but when running outside the project's directory things may or may not work.

Expected behavior

PS C:\path\to\todo-app-java-on-azure> cmd /c mvnw package
... (Maven runs successfully)

Actual behavior

PS C:\path\to\projectA> cmd /c "c:\path\to\todo-app-java-on-azure\mvnw" package -f "c:\path\to\todo-app-java-on-azure\pom.xml"
Error: Could not find or load main class org.apache.maven.wrapper.MavenWrapperMain
Caused by: java.lang.ClassNotFoundException: org.apache.maven.wrapper.MavenWrapperMain

Environments

• OS: Windows 10
• VS Code version: 1.41.1
• Extension version 0.20.2

[anthonyvdotbe]

@Eskibear thanks for the information, I understand better now how the extension works with terminals. However, it also raised additional questions about the Maven executable settings, so I'd like to propose a change for that as well.

For the Maven executable settings, I believe they should be specified & implemented as follows:

• maven.executable.path:

Specifies absolute path of your mvn executable. When this value is empty, it tries using 'mvn' and 'mvnw' as specified by 'maven.executable.preferMavenWrapper'.

So relative paths, or an absolute path to a mvnw executable, are forbidden.

• maven.executable.preferMavenWrapper:

Specifies whether you prefer to use Maven wrapper. If true, it tries using 'mvnw' by walking up the parent folder hierarchy. If false, or if 'mvnw' is not found in the parent folder hierarchy, it tries 'mvn' in PATH instead.

So the algorithm for locating mvnw is changed: it simply walks up the parent folder hierarchy. This is useful, in cases like the following:

• you use a single mvnw installation for a cluster of projects, so you'd have C:\clusterA\mvnw and C:\clusterA\projectA\pom.xml. And another cluster with C:\clusterB\mvnw and C:\clusterB\projectB\pom.xml. In VS Code, you'd open C:\clusterA\projectA and C:\clusterB\projectB as workspace roots. With the current algorithm, mvnw would not be found in this case (and it's not possible to solve by setting maven.executable.path either, because for projectA, the mvnw in clusterA should be used, and analog for projectB (and as mentioned above, I believe specifying a mvnw executable for that setting must be forbidden anyway))
• you have both C:\clusterA\mvnw and C:\clusterA\projectA\mvnw, and you open C:\clusterA as a workspace root. In that case, when running a Maven goal in project A, the current algorithm would incorrectly use C:\clusterA\mvnw

For running a Maven goal, I believe it should be done by doing pushd c:\path\to\pom.xml; <exec-path> <goals>; popd;, where exec-path is determined as above. This way, after having run a command, the user is always in the same directory (unlike with using cd, where you would end up in a different directory, which, as you rightly explained, is not desired).

[Eskibear]

For Maven executable settings

So relative paths, or an absolute path to a mvnw executable, are forbidden.

Agree.

it simply walks up the parent folder hierarchy. This is useful

I like the idea. BTW, as I observed, location of mvnw or mvnw.cmd doesn't matter at all. The key is .mvn folder, which contains maven-wrapper.jar and specifies Maven distribution url. As long as it finds a .mvn when walking up the parent folders from cwd, it should be ok to execute the maven command with mvnw.

In your original case, you can copy .mvn folder to any parent folder of projectA, try again, it should work.

PS C:\path\to\projectA> cmd /c "c:\path\to\todo-app-java-on-azure\mvnw" package -f "c:\path\to\todo-app-java-on-azure\pom.xml"

For running a Maven goal

I believe it should be done by doing pushd c:\path\to\pom.xml; <exec-path> <goals>; popd;

I believe so, and by doing that, we can get rid of the -f args. But then we have to tune it with some tricks, as different terminals have their own way to handle path stuff, multiple commands in one line...

E.g. in Windows CMD, we cannot use semicolon ; to join commands. It should be &

C:\Users\yanzh\Work>pushd spring-petclinic; mvnw.cmd clean; popd
The system cannot find the path specified.

C:\Users\yanzh\Work>pushd spring-petclinic & mvnw.cmd clean & popd
[INFO] Scanning for projects...
...

While in powershell, & is used to execute a file

PS C:\Users\yanzh\Work> pushd .\spring-petclinic & .\mvnw.cmd clean & popd
At line:1 char:26
+ pushd .\spring-petclinic & .\mvnw.cmd clean & popd
+                          ~
The ampersand (&) character is not allowed. The & operator is reserved for future use; wrap an ampersand in double quotation marks ("&") to pass it as part of a string.

PS C:\Users\yanzh\Work> pushd .\spring-petclinic; & .\mvnw.cmd clean ; popd
[INFO] Scanning for projects...

[anthonyvdotbe]

As long as it finds a .mvn when walking up the parent folders from cwd, it should be ok to execute the maven command with mvnw.

Yes, that's a great idea. Maybe that's what you meant, but just to make sure: it should check that the file .mvn\wrapper\maven-wrapper.properties exists. Just checking for the .mvn or .mvn\wrapper directory is not sufficient. The .mvn directory is used for several other purposes by Maven (see e.g. here) so it's possible that it exists, even though the project doesn't use Maven Wrapper. I experimented a bit, and .mvn\wrapper\maven-wrapper.properties is the only file that must exist for Maven Wrapper to work (of course, one of mvnw or mvnw.cmd is required as well): all the other files, such as the .jar, are downloaded/generated by Maven Wrapper as needed.

But then we have to tune it with some tricks, as different terminals have their own way to handle path stuff, multiple commands in one line...

Yes, good point. I don't know all the shells that VS Code has to support, but from testing with cmd/powershell/bash (in WSL), the only difference I can find, is the one you already pointed out: in cmd, the separator is &, while in powershell/bash, the separator is ;. And pushd/popd works in every shell.

[Eskibear]

Then we have below actions to take:

• [ ] improve the way to locate maven wrapper
  • walk through parent folders, looking for mvnw / mvnw.cmd (depending on which OS) https://github.com/microsoft/vscode-maven/pull/460
  • then check if .mvn\wrapper\maven-wrapper.properties exists
• [ ] improve the way to execute goals
  • change -f path-to-pom.xml to pushd/popd mechanism.
  • format to support different kinds of shells

[anthonyvdotbe]

Will you also please update the description of the executable settings with something like the following?

• for maven.executable.path:

Specifies absolute path of your mvn executable. When this value is empty, it tries using 'mvn' and 'mvnw' as specified by 'maven.executable.preferMavenWrapper'.

• for maven.executable.preferMavenWrapper:

Specifies whether you prefer to use Maven wrapper. If true, it tries using 'mvnw' by walking up the parent folder hierarchy. If false, or if 'mvnw' is not found in the parent folder hierarchy, it tries 'mvn' in PATH instead.

[Eskibear]

yes, I'll also update the description according to the changes.