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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
