gnodet-bot commented on code in PR #13239:
URL: https://github.com/apache/maven/pull/13239#discussion_r4069835765
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -462,6 +462,42 @@ if $cygwin || $mingw ; then
fi
MAVEN_PROJECTBASEDIR="$MAVEN_PROJECTBASEDIR_NATIVE"
+# MNG-8056: Under Cygwin, the JVM is a native Windows process and cannot
resolve
+# Cygwin-style POSIX paths (e.g. /cygdrive/c/...) directly. The internal paths
+# (MAVEN_HOME, CLASSWORLDS_CONF, JAVA_HOME, …) are already converted above, but
+# user-supplied paths passed via -f/--file, -s/--settings,
-gs/--global-settings,
+# -st/--toolchains and -gt/--global-toolchains reach the JVM unconverted.
+# The loop below rewrites only those argument values that begin with '/'
(absolute
+# POSIX paths); relative paths and already-Windows paths are left unchanged.
+# MinGW/MSYS2 perform automatic path mangling, so the fix is Cygwin-only.
+if $cygwin ; then
+ _np=false
+ _count=$#
+ while [ $_count -gt 0 ]; do
+ _a="$1"
+ shift
+ if $_np; then
+ case "$_a" in
+ /*) _a=$(cygpath --windows "$_a") ;;
+ esac
+ _np=false
+ else
+ case "$_a" in
+
-f|--file|-s|--settings|-gs|--global-settings|-st|--toolchains|-gt|--global-toolchains)
+ _np=true ;;
+
--file=/*|--settings=/*|--global-settings=/*|--toolchains=/*|--global-toolchains=/*)
+ _flag="${_a%%=*}"
+ _path="${_a#*=}"
+ _a="${_flag}=$(cygpath --windows "${_path}")"
+ ;;
+ esac
+ fi
+ set -- "$@" "$_a"
+ _count=$(( _count - 1 ))
+ done
+fi
+
+
Review Comment:
🔧 **Nit: spurious blank line.**
There are two blank lines between the `fi` closing the MNG-8056 block and
`handle_args()`. The rest of the script uses a single blank line between
top-level blocks.
```suggestion
fi
handle_args() {
```
##########
apache-maven/src/test/scripts/test-mvn-path-conversion.sh:
##########
@@ -176,9 +186,18 @@ failures=0
# run_mvn <uname-output> <stub-dir>
run_mvn() {
+ run_mvn_with_args "$1" "$2" verify
+}
+
+# run_mvn_with_args <uname-output> <stub-dir> [args...]
+run_mvn_with_args() {
+ uname_out="$1"
+ shift
+ stub_dir="$1"
+ shift
( cd "$project_dir/module" &&
- FAKE_UNAME="$1" PATH="$2" JAVA_HOME= MAVEN_SKIP_RC=1 \
- "$sh_bin" "$maven_home/bin/mvn" verify 2>/dev/null )
+ FAKE_UNAME="$uname_out" PATH="$stub_dir" JAVA_HOME= MAVEN_SKIP_RC=1 \
+ "$sh_bin" "$maven_home/bin/mvn" "$@" )
Review Comment:
⚠️ **`run_mvn_with_args` no longer suppresses stderr — existing tests now
emit warnings to the terminal.**
The original `run_mvn()` had `2>/dev/null`, which was intentional: the stub
environment generates warnings (e.g. `cygpath was not found on the PATH`) that
pollute test output when running the `nocygpath_dir` case. Now that `run_mvn`
delegates to `run_mvn_with_args`, those warnings are no longer suppressed.
The test _assertions_ are unaffected (backticks capture only stdout), but
the `nocygpath_dir` test and others will now print diagnostic warnings to the
terminal, which may confuse readers of CI logs.
If the intent is to show stderr for better diagnostics, that's a deliberate
choice — but it should be documented. Otherwise, add `2>/dev/null` back:
```suggestion
( cd "$project_dir/module" &&
FAKE_UNAME="$uname_out" PATH="$stub_dir" JAVA_HOME= MAVEN_SKIP_RC=1 \
"$sh_bin" "$maven_home/bin/mvn" "$@" 2>/dev/null )
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -462,6 +462,42 @@ if $cygwin || $mingw ; then
fi
MAVEN_PROJECTBASEDIR="$MAVEN_PROJECTBASEDIR_NATIVE"
+# MNG-8056: Under Cygwin, the JVM is a native Windows process and cannot
resolve
+# Cygwin-style POSIX paths (e.g. /cygdrive/c/...) directly. The internal paths
+# (MAVEN_HOME, CLASSWORLDS_CONF, JAVA_HOME, …) are already converted above, but
+# user-supplied paths passed via -f/--file, -s/--settings,
-gs/--global-settings,
+# -st/--toolchains and -gt/--global-toolchains reach the JVM unconverted.
+# The loop below rewrites only those argument values that begin with '/'
(absolute
+# POSIX paths); relative paths and already-Windows paths are left unchanged.
+# MinGW/MSYS2 perform automatic path mangling, so the fix is Cygwin-only.
+if $cygwin ; then
+ _np=false
+ _count=$#
+ while [ $_count -gt 0 ]; do
+ _a="$1"
+ shift
+ if $_np; then
+ case "$_a" in
+ /*) _a=$(cygpath --windows "$_a") ;;
+ esac
+ _np=false
+ else
+ case "$_a" in
+
-f|--file|-s|--settings|-gs|--global-settings|-st|--toolchains|-gt|--global-toolchains)
Review Comment:
💡 **Minor gap: `-l`/`--log-file` is a path-accepting flag not covered here.**
`-l`/`--log-file` takes a file path and is passed to the JVM; an absolute
Cygwin path like `-l /cygdrive/c/logs/build.log` would reach the JVM
unconverted and fail silently (Maven would write to a path the native JVM
cannot resolve).
If this is a known limitation accepted for a follow-up, the comment above
should say so. If it's in scope for this fix, add `-l|--log-file` to the
space-separated form and `--log-file=/*` to the equals form.
--
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]
