Convert paths for MSYS2 and jvm.config placeholders in bin/mvn

[Hiteshsai007]

The launcher already converted POSIX paths to their native Windows form for Cygwin and MinGW (GH-7958), but a few paths still reached the JVM in a mangled or mixed form:

• uname(1) reports MSYS_NT-* for the MSYS2 shell, which did not match the MINGW* pattern, so no conversion happened at all there. The MSYS2 argument conversion then rewrote the POSIX path into the mixed form D:/a/... instead of D:\a...

• ${MAVEN_PROJECTBASEDIR} placeholders in .mvn/jvm.config were expanded with the POSIX path, since JvmConfigParser was invoked before the conversion took place. The base directory is now converted upfront and both representations are kept: the POSIX one to locate jvm.config from the shell, the native one for everything handed over to the JVM.

• -Dlibrary.jline.path was built by appending /lib/jline-native to the already converted MAVEN_HOME, producing C:\maven/lib/jline-native. It is now converted on its own.

Additionally, the output of JvmConfigParser and of the base directory lookup is printed with printf instead of echo. Several POSIX shells (dash and other Almquist derivatives commonly installed as /bin/sh) expand backslash escapes in echo, which corrupts Windows paths, and the conversion is guarded so a missing cygpath(1) warns and falls back to POSIX paths rather than passing empty paths to the JVM.

A shell test covering all of the above is added; it stubs uname, cygpath and java so the Windows-only code paths can be exercised on any platform, and it can be run on any platform or wired into CI.

Fixes GH-12537

Following this checklist to help us incorporate your
contribution quickly and easily:

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
  Note that commits might be squashed by a maintainer on merge.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  This may not always be possible but is a best-practice.
• Run mvn verify to make sure basic checks pass.
  A more thorough check will be performed on your pull request automatically.
• You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[gnodet]

First review — well-crafted PR! The core changes (MSYS* detection, native basedir split, JLINE_NATIVE_PATH, cygpath safety guard) are all correct and elegant.

Three minor observations:

1. Debug echo with Windows paths (line 182) — The debug line references $MAVEN_PROJECTBASEDIR_NATIVE but still uses echo. On dash/Almquist shells, echo interprets backslash sequences, so a Windows path like C:\foo\bar could corrupt \f/\b. Consider using printf for consistency with the PR's own rationale for the echo→printf changes elsewhere:

   printf '[DEBUG] Parser arguments: %s %s %s\n' "$MAVEN_HOME/bin/JvmConfigParser.java" "$1" "$MAVEN_PROJECTBASEDIR_NATIVE" >&2
   

2. Same issue on line 198 — echo "[DEBUG] JvmConfigParser output: $parser_output" now carries Windows paths in $parser_output. Same backslash risk:

   printf '[DEBUG] JvmConfigParser output: %s\n' "$parser_output" >&2
   

3. Test not wired into CI — test-mvn-path-conversion.sh is standalone. Future changes could regress path handling without anyone running it. Consider hooking it into the build or adding a note about integration.

Overall the approach is solid — the ordering of operations (converting basedir upfront for JvmConfigParser while keeping POSIX form for shell file access) is well thought out, and the test coverage across CYGWIN/MINGW/MSYS variants is comprehensive.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[Hiteshsai007]

Thanks for the review, @gnodet — all three points addressed in the force-push to 033146e.

1 & 2 — echo → printf in the debug logging

You were right, and it was a real inconsistency: the PR argued that echo corrupts Windows paths, then kept using echo for the debug output. With MAVEN_DEBUG_SCRIPT=1 under dash, C:\tmp\build was printed as C:<tab>mp\build.

I went slightly wider than the two lines flagged, since the same flaw applied elsewhere in the script. Every statement that interpolates a path now uses printf '%s':

• [DEBUG] Parser arguments: and [DEBUG] JvmConfigParser output: (the two you pointed out)
• [DEBUG] Found jvm.config file at: and Running JvmConfigParser with Java:
• The three error-path lines: jvm.config path:, Maven basedir:, Java command:
• [DEBUG] Final MAVEN_OPTS:
• The JAVA_HOME is set to "..." diagnostic — pre-existing, same problem

Three echo calls remain deliberately: a numeric exit code and two literal strings with no interpolation. The rationale comment moved to the top of the script so it governs the whole file rather than sitting at one call site.

3 — Test wired into the build

Rather than just documenting it, the test now runs under mvn verify via exec-maven-plugin, bound to the test phase in a shell-script-tests profile activated on <family>unix</family> (it drives the POSIX bin/mvn; Windows ships mvn.cmd). It honours -DskipTests.

Test coverage

Added an assertion on the debug output that reproduces the exact problem you raised — it fails when those statements are reverted to echo and passes with printf, so this can't silently regress.

Also fixed a bug in the test harness itself that I'd missed: it appended the ambient PATH to the stub directory, so on a real Cygwin/MSYS2 machine the genuine cygpath was picked up and the missing-cygpath fallback was never exercised. The launcher now runs with a PATH containing only the stubs, which makes the result identical on every platform. Consequently the stubs use shell built-ins exclusively.

Now 29 assertions, verified green under sh, dash, bash and busybox sh, and still reporting 13 failures against the unpatched launcher.

One note on scope: this targets master (4.x), whereas #12537 asks specifically for a maven-3.10.x backport, so I've changed the trailer to Related to rather than Fixes. Points 1, 3 and the echo/printf issue apply equally to the 3.x script — happy to open a separate PR against maven-3.10.x if that's useful.

[gnodet]

All three concerns from the previous review are addressed in the new commit:

1. ✅ Debug echo with $MAVEN_PROJECTBASEDIR_NATIVE now uses printf
2. ✅ Debug echo with $parser_output now uses printf
3. ✅ Test is now wired into the build via exec-maven-plugin in a unix-only profile

The architecture is sound — POSIX form for shell file access, Windows-converted form for the JVM. The cygpath safety guard and test coverage (Linux, CYGWIN, MINGW32, MINGW64, MSYS_NT, missing-cygpath fallback, dash/Almquist shell escape handling) are thorough.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

The macOS CI failures (all 22 assertion failures in test-mvn-path-conversion.sh) are caused by a double-slash in the temp directory path.

Root cause: On macOS, $TMPDIR ends with a trailing / (e.g. /var/folders/.../T/). When the test runs mktemp -d "${TMPDIR}/mvn-path-conversion.XXXXXX", it produces a path with // (e.g. .../T//mvn-path-conversion.XNBzZw). The pwd builtin inside the mvn launcher normalizes this to a single /, so every assertion comparing the test's $project_dir / $maven_home against the actual launcher output mismatches.

Fix: Normalize work_dir through cd/pwd right after mktemp:

work_dir=`mktemp -d "${TMPDIR:-/tmp}/mvn-path-conversion.XXXXXX"`
work_dir=`cd "$work_dir" && pwd`

I've opened #12563 with this one-line fix applied on top of this PR's commit.

[Hiteshsai007]

The macOS failures were a bug in my test harness (double slash from mktemp when TMPDIR has a trailing slash), not the launcher — which is why only macOS was affected while Linux and Windows passed.

@gnodet has fixed it in #12563, which includes my commit 033146e plus the normalisation. Happy to close this in favour of that PR, or to push the one-line fix here if you'd prefer to keep this one — whichever is easier for you.

[gnodet]

New commit (afcb7db) looks good — normalizes the temp dir path through cd/pwd after mktemp to avoid the double-slash issue on macOS where $TMPDIR ends with a trailing slash. Clean fix. Prior APPROVE still stands.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[github-actions]

@gnodet Please assign appropriate label to PR according to the type of change.