Hiteshsai007 commented on PR #12546: URL: https://github.com/apache/maven/pull/12546#issuecomment-5100397641
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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
