gnodet-bot commented on code in PR #13447:
URL: https://github.com/apache/maven/pull/13447#discussion_r4251070960
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -398,13 +398,20 @@ concat_lines() {
}
# Run the parser using source-launch mode
- # Capture both stdout and stderr for comprehensive error reporting
- parser_output=$("$JAVACMD" "$MAVEN_HOME/bin/JvmConfigParser.java" "$1"
"$MAVEN_PROJECTBASEDIR_NATIVE" 2>&1)
+ # Capture stdout for JVM options; capture stderr separately so JVM notices
Review Comment:
💡 **Advisory: missing signal trap — temp file may leak on SIGTERM/SIGINT**
`mktemp` creates the file unconditionally, but there is no `trap` to clean
it up if a signal arrives between `mktemp` and either `rm -f` call. On
long-running Maven builds this could accumulate stale files in `/tmp`.
Consider adding a trap immediately after `mktemp`:
```suggestion
parser_stderr_file=$(mktemp
"${TMPDIR:-/tmp}/mvn-jvm-config-stderr.XXXXXX" 2>/dev/null || echo
"${TMPDIR:-/tmp}/mvn-jvm-config-stderr.$$")
trap 'rm -f "$parser_stderr_file"' EXIT INT TERM
```
Low severity — `/tmp` is self-cleaning on most systems, but the fix is
trivial.
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -414,10 +421,18 @@ concat_lines() {
printf ' Maven basedir: %s\n' "$MAVEN_PROJECTBASEDIR" >&2
printf ' Java command: %s\n' "$JAVACMD" >&2
echo " Parser output:" >&2
- printf '%s\n' "$parser_output" | sed 's/^/ /' >&2
+ if [ -s "$parser_stderr_file" ]; then
+ while IFS= read -r parser_err_line || [ -n "$parser_err_line" ]; do
+ printf ' %s\n' "$parser_err_line" >&2
+ done < "$parser_stderr_file"
+ elif [ -n "$parser_output" ]; then
+ printf ' %s\n' "$parser_output" >&2
+ fi
+ rm -f "$parser_stderr_file" 2>/dev/null || :
exit 1
Review Comment:
💡 **Advisory: `elif` silently drops stdout content in error path**
In the error-reporting block, `elif [ -n "$parser_output" ]` is only
evaluated when stderr is empty. If a future parser version (or a partially
failing run) writes to both stdout and stderr, the stdout content is silently
discarded from the error message.
Current structure:
```sh
if [ -s "$parser_stderr_file" ]; then
# show stderr
elif [ -n "$parser_output" ]; then
# show stdout only if stderr was empty
fi
```
This is intentional for the current parser (notices go to stderr, options to
stdout — they're mutually exclusive on failure), but worth a comment explaining
the assumption so future maintainers don't inadvertently drop diagnostic output.
No code change required if the invariant is documented.
--
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]
