You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/10/20 13:21:19 UTC
[PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
gnodet opened a new pull request, #13652:
URL: https://github.com/apache/maven/pull/13652
Fix Windows-only CI failures where jvm.config options were silently dropped,
causing `MavenITmng4559MultipleJvmArgsTest`, `MavenITmng4559SpacesInJvmOptsTest`,
`MavenITmng6255FixConcatLines.testJvmConfigFileLF` and related tests to fail
with `expected: <ok> but was: <${prop}>`.
## Root cause
After commit b28f385ca5 (precompiled `JvmConfigParser`), the startup latency
dropped from ~450 ms (source-launch JVM) to ~30 ms. This made the Windows
Defender real-time scan lock race far more likely to be hit:
1. `JvmConfigParser` writes its output to a temp `.txt` file.
2. Windows Defender briefly holds a read lock on the new file.
3. `set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%"` fails silently (or emits
a "system cannot find the file" error) and leaves the variable empty.
4. The retry guard `if not defined JVM_CONFIG_MAVEN_OPTS` never fires because
`SET JVM_CONFIG_MAVEN_OPTS=` (earlier in the script) assigns an **empty
string**, and in cmd.exe an empty-string assignment counts as *defined*.
The net result: jvm.config options are dropped, `-D` properties never reach the
Maven JVM, and tests that assert a property value see the unresolved literal
`${prop.name}`.
## Fix
Replace `if not defined JVM_CONFIG_MAVEN_OPTS` with
`if "%JVM_CONFIG_MAVEN_OPTS%"==""` — a string comparison that correctly
identifies the empty case and triggers the retry.
Also isolate the `MavenITmng6255FixConcatLines` test variants so each
line-ending variant (LF, CRLF, CR) writes its `.mvn/jvm.config` into its own
project copy (`mng-6255-lf/`, `mng-6255-crlf/`, etc.) rather than sharing a
single directory, preventing cross-contamination between sequential (and future
parallel) test runs.
- [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config handling — escape backslashes in @argfile output and pass via @argfile token [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13652:
URL: https://github.com/apache/maven/pull/13652#issuecomment-6341279531
<!-- maven-backport-bot -->
**Conflicts — manual resolution needed:**
- ❌ `maven-4.0.x`: Cherry-pick conflict in:
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4347308147
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,21 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ endlocal & set "JVM_CONFIG_MAVEN_OPTS=!JVM_CONFIG_MAVEN_OPTS!"
Review Comment:
⚠️ **Bug:** After `endlocal`, delayed expansion is disabled (the outer `@setlocal` at line 53 does not enable it). `!JVM_CONFIG_MAVEN_OPTS!` on this line is therefore treated as a literal string — not expanded — and `JVM_CONFIG_MAVEN_OPTS` is exported as `!JVM_CONFIG_MAVEN_OPTS!`.
Use the `FOR /F` workaround to capture the value while still inside the delayed-expansion scope:
```suggestion
for /f "delims=" %%V in ("!JVM_CONFIG_MAVEN_OPTS!") do (
endlocal
set "JVM_CONFIG_MAVEN_OPTS=%%V"
)
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4335669782
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,14 +129,40 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java @argfile format: one argument per line.
+ * Arguments are quoted when they contain whitespace, double-quotes, backslashes,
+ * or any shell metacharacter that {@code eval} would re-interpret on Unix
+ * (pipe, ampersand, semicolon, redirection, backtick).
+ * Quoting is always valid in {@code @argfile} format, so this is safe for
+ * both the Windows {@code @argfile} consumer and the Unix {@code eval} consumer.
Review Comment:
The Javadoc says "the Unix `{@code eval}` consumer" — but Unix no longer uses `eval` to process the argfile content. The `@argfile` is now passed directly to the JVM as `@"$JVM_CONFIG_ARGFILE"`; the JVM's own @argfile parser reads it. Shell metacharacters (`|`, `&`, `;`, `<`, `>`, `` ` ``) have no special meaning in Java's @argfile format, so the shell-metachar quoting rationale is technically no longer needed (though harmless).
```suggestion
* Append parsed arguments in Java @argfile format: one argument per line.
* Arguments are quoted when they contain whitespace, double-quotes, or backslashes.
* Quoting is always valid in {@code @argfile} format and ensures safe parsing
* by both the Windows and Unix JVM @argfile consumers.
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -473,17 +474,16 @@ if $cygwin || $mingw ; then
MAVEN_PROJECTBASEDIR_NATIVE=`cygpath --windows "$MAVEN_PROJECTBASEDIR"`
fi
-# Read JVM config and append to MAVEN_OPTS, preserving special characters
-_jvm_config="`concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config"`"
-if [ -n "$_jvm_config" ]; then
- if [ -n "$MAVEN_OPTS" ]; then
- MAVEN_OPTS="$MAVEN_OPTS $_jvm_config"
- else
- MAVEN_OPTS="$_jvm_config"
- fi
-fi
+# Parse jvm.config into a temp argfile (avoids newline-in-MAVEN_OPTS regression).
+# The temp file path is stored in JVM_CONFIG_ARGFILE; if no jvm.config exists,
+# JvmConfigParser creates an empty file and the @argfile is silently ignored.
+JVM_CONFIG_ARGFILE=$(mktemp "${TMPDIR:-/tmp}/mvn-jvm-config.XXXXXX" 2>/dev/null || echo "${TMPDIR:-/tmp}/mvn-jvm-config.$$")
+prepare_jvm_config_argfile "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config" "$JVM_CONFIG_ARGFILE"
Review Comment:
`mktemp` runs unconditionally here — even when `.mvn/jvm.config` does not exist. Since the script ends with `eval exec` (replacing the shell process), no EXIT trap fires and the file is never cleaned up. Every `mvn` invocation leaks a temp file.
The comment on the line above is also incorrect: it says "JvmConfigParser creates an empty file", but JvmConfigParser is not called at all when no jvm.config exists — `mktemp` is what creates the file.
```suggestion
JVM_CONFIG_ARGFILE=
if [ -f "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config" ]; then
JVM_CONFIG_ARGFILE=$(mktemp "${TMPDIR:-/tmp}/mvn-jvm-config.XXXXXX" 2>/dev/null || echo "${TMPDIR:-/tmp}/mvn-jvm-config.$$")
prepare_jvm_config_argfile "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config" "$JVM_CONFIG_ARGFILE"
fi
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4325569039
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -337,8 +337,10 @@ if exist "%JVM_CONFIG_TEMP%" (
type "%JVM_CONFIG_TEMP%"
)
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
- rem Retry once after a brief delay if the read failed (Windows Defender file lock)
- if not defined JVM_CONFIG_MAVEN_OPTS (
+ rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
+ rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
+ rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
+ if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
Review Comment:
⚠️ **Parse-time expansion bug — this condition is always true.**
cmd.exe expands `%JVM_CONFIG_MAVEN_OPTS%` when it first parses the opening `(` at line 334 — before `set /p` on line 339 has run. At parse time, `JVM_CONFIG_MAVEN_OPTS` is still the empty string set at line 283, so this becomes `if ""==""` — unconditionally true.
The `ping -n 2 127.0.0.1` retry will execute on **every** `mvn` invocation with a `.mvn/jvm.config` file, adding ~1 second of unnecessary delay.
Fix: use delayed expansion (`!JVM_CONFIG_MAVEN_OPTS!` with `setlocal EnableDelayedExpansion`) or move the check outside this compound block.
```suggestion
if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
```
(Requires adding `setlocal EnableDelayedExpansion` before this block and `endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"` after it.)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config handling — escape backslashes in @argfile output and pass via @argfile token [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #13652:
URL: https://github.com/apache/maven/pull/13652
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4334419339
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,14 +129,23 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java @argfile format: one argument per line.
+ * Arguments containing spaces or double-quotes are quoted; pipes and other
+ * special characters are literal (not interpreted by the shell).
*/
private static void appendQuotedArguments(StringBuilder result, List<String> args) {
for (String arg : args) {
if (result.length() > 0) {
- result.append(' ');
+ result.append(System.lineSeparator());
+ }
+ // Quote the argument if it contains whitespace or double-quotes
+ boolean needsQuoting = arg.indexOf(' ') >= 0 || arg.indexOf('\t') >= 0 || arg.indexOf('"') >= 0;
Review Comment:
⚠️ **Unix regression — shell metacharacters now unquoted in `MAVEN_OPTS`**
The old code always double-quoted every arg. The new `needsQuoting` only checks for space, tab, and double-quote. Args like `-Dhttp.nonProxyHosts=de|*.de` (pipe, no space/quote) are output **unquoted**. On Unix, `mvn` captures this stdout, assigns it unquoted to `MAVEN_OPTS`, then does `eval exec "$cmd"`. Eval re-parses the command as shell code → unquoted `|` = shell pipe → Maven fails on Unix.
Fix: extend the check to include shell metacharacters. Quoting is always valid in `@argfile` format, so this is safe for both consumers:
```suggestion
// Quote the argument if it contains whitespace, double-quotes, or any shell
// metacharacter that eval would re-interpret on Unix (|, &, ;, <, >, `, $).
// Quoting is always valid in @argfile format, so this is safe for both the
// Windows @argfile consumer and the Unix eval-based consumer in bin/mvn.
boolean needsQuoting = arg.indexOf(' ') >= 0 || arg.indexOf('\t') >= 0
|| arg.indexOf('"') >= 0 || arg.indexOf('|') >= 0
|| arg.indexOf('&') >= 0 || arg.indexOf(';') >= 0
|| arg.indexOf('<') >= 0 || arg.indexOf('>') >= 0
|| arg.indexOf('`') >= 0 || arg.indexOf('$') >= 0
|| arg.endsWith("\\\\");
```
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,14 +129,23 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java @argfile format: one argument per line.
+ * Arguments containing spaces or double-quotes are quoted; pipes and other
+ * special characters are literal (not interpreted by the shell).
*/
private static void appendQuotedArguments(StringBuilder result, List<String> args) {
for (String arg : args) {
if (result.length() > 0) {
- result.append(' ');
+ result.append(System.lineSeparator());
+ }
+ // Quote the argument if it contains whitespace or double-quotes
+ boolean needsQuoting = arg.indexOf(' ') >= 0 || arg.indexOf('\t') >= 0 || arg.indexOf('"') >= 0;
+ if (needsQuoting) {
+ // Escape embedded double-quotes as \" per @argfile spec
+ result.append('"').append(arg.replace("\"", "\\\"")).append('"');
Review Comment:
⚠️ **`@argfile` trailing-backslash not escaped — line-continuation bug**
Per the Java `@argfile` spec: backslash must be escaped as `\\\\`, and `\\` at end-of-line is a line-continuation. This code does not escape backslashes.
For an arg ending in `\\` (e.g. `-Dpath=C:\\Users\\foo\\`):
- Unquoted output: `arg\\` + newline → line continuation → next arg concatenated to this one.
- Quoted output: `"arg\\"` → the trailing `\\"` is an escaped quote → string never closes → parse error.
Fix: escape backslashes before double-quotes in the quoting branch:
```suggestion
if (needsQuoting) {
// Escape backslashes first (in @argfile, \\\\ is a literal backslash, and a
// trailing \\ before the closing quote would be misread as an escaped quote).
// Then escape embedded double-quotes as \\" per @argfile spec.
String escaped = arg.replace("\\\\", "\\\\\\\\").replace("\"", "\\\\\"");
result.append('"').append(escaped).append('"');
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4346832531
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,21 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"
del "%JVM_CONFIG_TEMP%" 2>nul
Review Comment:
`endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"` is inside the `if exist "%JVM_CONFIG_TEMP%" (` compound block. cmd.exe expands **all** `%VAR%` in the entire compound block at parse time — before any line inside has executed. At parse time, `JVM_CONFIG_MAVEN_OPTS` holds the empty string from the earlier `set JVM_CONFIG_MAVEN_OPTS=` (line 283), so this line becomes `endlocal & set "JVM_CONFIG_MAVEN_OPTS="` unconditionally. The value written by `set /p` is never exported from the `setlocal` scope.
**Reference:** [cmd.exe parse-time expansion in compound blocks](https://stackoverflow.com/questions/16852503): *"When a compound statement enclosed in parentheses is to be executed, the statement is first parsed… At this time, any `%var%` is replaced by that var's value… (its PARSE-TIME value)."*
Note that using `!JVM_CONFIG_MAVEN_OPTS!` here would not help: in the `endlocal & set` idiom, `endlocal` runs first, destroying the inner scope, so `!VAR!` would then expand to the outer scope's (empty) value.
**Fix:** Move `endlocal & set` **outside** the `if exist` compound block (after the closing `)`). When it is a standalone line, `%JVM_CONFIG_MAVEN_OPTS%` is expanded at line-execution time, after `set /p` has run and `setlocal`'s scope is still active for the expansion:
```suggestion
del "%JVM_CONFIG_TEMP%" 2>nul
)
endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4347456660
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,34 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ rem Export JVM_CONFIG_MAVEN_OPTS from the setlocal scope using FOR /F.
+ rem The 'endlocal & set' idiom cannot use !VAR! after endlocal because delayed
+ rem expansion is disabled in the outer scope (@setlocal at line 53 has no
+ rem EnableDelayedExpansion). FOR /F captures the value while still inside the
+ rem delayed-expansion scope, then endlocal inside the loop body restores the
+ rem outer scope — at which point %%V is already bound to the correct value.
+ rem When the value is empty (both reads failed), FOR /F skips the empty token,
+ rem so we use a sentinel prefix trick: prefix with '#', strip it after export.
+ for /f "delims=" %%V in ("#!JVM_CONFIG_MAVEN_OPTS!") do (
+ endlocal
+ set "JVM_CONFIG_MAVEN_OPTS=%%V"
+ )
+ rem Strip the '#' sentinel prefix added above (works in both empty and non-empty cases)
+ set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS:~1%"
Review Comment:
⚠️ **`FOR /F` literal-string quoting breaks when the value contains `"` — which it always does.**
`JvmConfigParser.appendQuotedArguments` wraps every argument in double-quotes before writing to the temp file. For `-Xmx512m`, the file contains `"-Xmx512m"` (with literal `"` characters). After `set /p`, `JVM_CONFIG_MAVEN_OPTS` = `"-Xmx512m"`.
After delayed expansion, the `FOR /F` line becomes:
```
for /f "delims=" %%V in ("#"-Xmx512m"") do (
```
The `in ("...")` literal-string form uses the outermost `"` as delimiters. With embedded `"` in the expanded value, cmd.exe sees `"#"` as the complete string and treats `-Xmx512m""` as garbage outside the `in (...)` clause. Only `#` is captured as `%%V` — after the sentinel strip, `JVM_CONFIG_MAVEN_OPTS` is empty. JVM opts are silently dropped on every non-empty jvm.config.
The `mng-4559-spaces-jvm-opts` test uses `-Dprop.jvm-opts="foo bar"` in jvm.config; `JvmConfigParser` produces `"-Dprop.jvm-opts=foo bar"` in the temp file — this test will fail on Windows.
**Fix:** use the dbenham double-double-quote idiom with `%%~V` to strip the outer quotes — this safely handles values containing embedded `"` and also correctly exports an empty value (no sentinel needed):
```suggestion
rem so we use the dbenham double-double-quote export idiom: wrap in ""..."" and
rem use %%~V to strip the outer quotes. This handles values with embedded " safely.
for /f "delims=" %%V in (""!JVM_CONFIG_MAVEN_OPTS!"") do (
endlocal
set "JVM_CONFIG_MAVEN_OPTS=%%~V"
)
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4347512839
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,34 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ rem Export JVM_CONFIG_MAVEN_OPTS from the setlocal scope using FOR /F.
+ rem The 'endlocal & set' idiom cannot use !VAR! after endlocal because delayed
+ rem expansion is disabled in the outer scope (@setlocal at line 53 has no
+ rem EnableDelayedExpansion). FOR /F captures the value while still inside the
+ rem delayed-expansion scope, then endlocal inside the loop body restores the
+ rem outer scope — at which point %%V is already bound to the correct value.
+ rem When the value is empty (both reads failed), FOR /F skips the empty token,
+ rem so we use a sentinel prefix trick: prefix with '#', strip it after export.
+ for /f "delims=" %%V in ("#!JVM_CONFIG_MAVEN_OPTS!") do (
+ endlocal
+ set "JVM_CONFIG_MAVEN_OPTS=%%V"
+ )
+ rem Strip the '#' sentinel prefix added above (works in both empty and non-empty cases)
+ set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS:~1%"
Review Comment:
Good catch — the sentinel approach breaks when JvmConfigParser writes quoted args (e.g. `"-Xmx512m"`). Switched to the dbenham double-double-quote idiom (`for /f "delims=" %%V in (""!VAR!"") do`) with `%%~V` to strip outer quotes. Handles embedded `"` safely. Amended in e3bf914c92.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4333635521
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,21 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"
del "%JVM_CONFIG_TEMP%" 2>nul
)
Review Comment:
fixed in https://github.com/apache/maven/pull/13652/commits/ef2e63827adbea641d52142635a2404209c39384
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4334596994
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,14 +129,40 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java @argfile format: one argument per line.
+ * Arguments are quoted when they contain whitespace, double-quotes, backslashes,
+ * or any shell metacharacter that {@code eval} would re-interpret on Unix
+ * (pipe, ampersand, semicolon, redirection, backtick).
+ * Quoting is always valid in {@code @argfile} format, so this is safe for
+ * both the Windows {@code @argfile} consumer and the Unix {@code eval} consumer.
*/
private static void appendQuotedArguments(StringBuilder result, List<String> args) {
for (String arg : args) {
if (result.length() > 0) {
- result.append(' ');
+ result.append(System.lineSeparator());
Review Comment:
⚠️ **Unix regression — newline separator breaks `eval` in `mvn` when `jvm.config` has 2+ arguments**
On Unix, `JvmConfigParser` is invoked **without** an output file (`args[2]` not provided), so it writes to stdout (line 78: `System.out.print(result)`). The Unix `mvn` script's `concat_lines()` captures this stdout and assigns it to `MAVEN_OPTS`, which is then embedded unquoted in `cmd` and passed to `eval exec`.
Changing the separator from `' '` to `System.lineSeparator()` means `MAVEN_OPTS` now contains literal newlines when `jvm.config` has two or more entries. In `eval`, a newline is a command terminator, so only the first argument reaches the JVM — the second becomes a separate (failing) command.
Verified with a live test: `eval` returns exit 127 (`-Dfoo=bar: command not found`) when `MAVEN_OPTS` contains a newline between two args.
The `mvn` Unix script must also be updated to use `@argfile` (pass the temp file via `@"$tmpfile"`) so the same approach is used on both platforms.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4347733646
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,7 +128,10 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java {@code @argfile} format: one argument per line.
+ * Arguments are quoted when they contain whitespace, double-quotes, or backslashes.
+ * Quoting is always valid in {@code @argfile} format and ensures safe parsing
+ * by both the Windows and Unix JVM {@code @argfile} consumers.
Review Comment:
**Javadoc/implementation mismatch:** The Javadoc says *"Arguments are quoted when they contain whitespace, double-quotes, or backslashes"*, implying selective/conditional quoting. But the implementation quotes every argument unconditionally — which is fine. Fix the Javadoc to match the actual behavior, and document the backslash escaping requirement (see the inline comment below on the method body):
```suggestion
* Append parsed arguments in Java {@code @argfile} format.
* Every argument is wrapped in double-quotes. Per the JDK {@code @argfile} spec,
* backslashes inside double-quoted strings are escape characters and must be
* doubled ({@code \\}) to be treated as literal backslashes.
```
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -128,7 +128,10 @@ private static String processLine(String line, String mavenProjectBasedir) {
}
/**
- * Append parsed arguments as quoted strings to the result builder.
+ * Append parsed arguments in Java {@code @argfile} format: one argument per line.
+ * Arguments are quoted when they contain whitespace, double-quotes, or backslashes.
+ * Quoting is always valid in {@code @argfile} format and ensures safe parsing
+ * by both the Windows and Unix JVM {@code @argfile} consumers.
*/
private static void appendQuotedArguments(StringBuilder result, List<String> args) {
Review Comment:
🐛 **`appendQuotedArguments` does not escape backslashes — corrupts Windows paths in @argfile.**
In Java `@argfile` format (JDK 9+), inside a **double-quoted string**, backslash is an **escape character**. The JDK launcher source (`args.c`) processes: `\f` → form-feed, `\n` → newline, `\r` → CR, `\t` → tab, `\\` → literal backslash; unrecognised escapes like `\U`, `\a` silently corrupt the character. Reference: Jonathan Gibbons (JDK launcher engineer), JDK-8216522: *"inside quotes, the backslash character is an escape character, and must be doubled to pass through a backslash character."*
The `processLine` method injects `mavenProjectBasedir` (a Windows path like `C:\Users\alice\project`) via `${MAVEN_PROJECTBASEDIR}` substitution. That path reaches `appendQuotedArguments`, which wraps it in quotes without escaping the backslashes. The argfile then contains `"-Dmy.prop=C:\Users\alice\project"` → the JVM receives `-Dmy.prop=C:` + (corrupted characters).
Fix — escape backslashes before wrapping in double-quotes:
```suggestion
private static void appendQuotedArguments(StringBuilder result, List<String> args) {
for (String arg : args) {
if (result.length() > 0) {
result.append(' ');
}
result.append('"').append(arg.replace("\", "\\")).append('"');
}
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4333566111
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -336,17 +336,21 @@ if exist "%JVM_CONFIG_TEMP%" (
echo [DEBUG] Temp file contents:
type "%JVM_CONFIG_TEMP%"
)
+ setlocal EnableDelayedExpansion
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
rem Retry once after a brief delay if the read returned empty (Windows Defender file lock).
rem Note: use string comparison instead of 'if not defined' because 'set JVM_CONFIG_MAVEN_OPTS='
rem (empty assignment) counts as defined, so 'if not defined' would never trigger the retry.
- if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
+ rem Note: use delayed expansion (!VAR! instead of %VAR%) because cmd.exe expands %VAR% at
+ rem parse time for the entire compound block, before 'set /p' above has run at runtime.
+ if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
if defined MAVEN_DEBUG_SCRIPT (
echo [DEBUG] First read returned empty, retrying after delay...
)
ping -n 2 127.0.0.1 >nul 2>nul
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
)
+ endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"
del "%JVM_CONFIG_TEMP%" 2>nul
)
Review Comment:
⚠️ **Parse-time expansion bug — `endlocal & set` must be outside the compound block.**
This line is inside the `if exist "%JVM_CONFIG_TEMP%" (` compound block (lines 334–355). When cmd.exe first parses that block, it expands all `%VAR%` references to their values **at parse time** — before `set /p` on line 340 has run. `%JVM_CONFIG_MAVEN_OPTS%` at parse time equals the empty string from `set JVM_CONFIG_MAVEN_OPTS=` at line 283, so this entire block becomes:
```
endlocal & set "JVM_CONFIG_MAVEN_OPTS="
del "%JVM_CONFIG_TEMP%" 2>nul
)
```
…unconditionally, regardless of what `set /p` actually read. The `endlocal & set` idiom only works when the line is parsed **outside** a parenthesised block — where cmd.exe parses each statement independently, at execution time, so `%JVM_CONFIG_MAVEN_OPTS%` correctly captures the value set by `set /p`.
Fix: move the `del` inside the block and `endlocal & set` after the closing `)`:
```suggestion
del "%JVM_CONFIG_TEMP%" 2>nul
)
endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: repair Windows jvm.config retry guard (if not defined → string comparison) [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13652:
URL: https://github.com/apache/maven/pull/13652#issuecomment-6339274779
Reverted the FOR /F approach and went back to `endlocal & set "JVM_CONFIG_MAVEN_OPTS=!JVM_CONFIG_MAVEN_OPTS!"` (commit `dac74767f8`).
The FOR /F approaches had compounding issues:
1. `""!VAR!""` + `%%~V` -- breaks when the value is empty (FOR /F skips empty token -> `endlocal` never called -> `setlocal` scope leaks)
2. `"#""!VAR!"""` + `:~1` strip -- JvmConfigParser wraps every arg in `"..."`, so `JVM_CONFIG_MAVEN_OPTS` is e.g. `"-Xmx512m" "-Dprop=foo bar"` -- the embedded quotes and spaces corrupt the `FOR /F in(...)` clause regardless of the sentinel
The bot's concern about `!VAR!` not working after `endlocal` is incorrect. On a single `cmd1 & cmd2` line, cmd.exe parses (and expands `!VAR!`) the entire line while `EnableDelayedExpansion` is still active, then executes `endlocal`, then runs `set` with the already-expanded value. This is the canonical pattern for exporting variables from a `setlocal EnableDelayedExpansion` scope -- see https://stackoverflow.com/a/13871514 (dbenham).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org