gnodet commented on PR #13478:
URL: https://github.com/apache/maven/pull/13478#issuecomment-6193266022
Thanks for the detailed PR and for adding both unit tests and an IT to cover
the fix. I spent some time digging into the IT failure and found the root cause
— it's not a bug in the shell parser itself.
**Root cause: `maven-executor` strips `MAVEN_ARGS` from the subprocess
environment**
The shell parser logic in `bin/mvn` is correct. Running the script directly
with `MAVEN_ARGS='"..."'` set in the environment works as expected. The unit
tests in `test-mvn-args.sh` confirm this too.
The issue is in how the IT infrastructure invokes Maven. The
`ForkedMavenExecutor` (from `maven-executor` 1.0.0) explicitly removes
`MAVEN_ARGS` from the child process environment before launching `bin/mvn`:
```java
env.remove("MAVEN_ARGS"); // we already used it if configured to do so
// ...
pb.environment().remove("MAVEN_ARGS"); // from inherited env too
```
So when `bin/mvn` runs inside the IT, `$MAVEN_ARGS` is always empty — the
new parsing block is skipped, and the `-D` properties are never passed to
Maven. This is confirmed by the `MAVEN_DEBUG_SCRIPT=1` output in the CI
artifact (`log-stderr.txt`): the JVM launch command contains only the
verifier-supplied CLI args, with no trace of the `MAVEN_ARGS` words.
**Why the old approach worked**
The previous `eval exec "$cmd" '$MAVEN_ARGS' '"$@"'` passed `MAVEN_ARGS` as
a literal token in the command string assembled by the script itself — it
didn't depend on the variable being present in the subprocess environment.
`maven-executor` stripping `MAVEN_ARGS` from the env had no effect.
**The architectural tension**
`maven-executor` strips `MAVEN_ARGS` because it has its own
`useMavenArgsEnv` mechanism to handle it (splitting on spaces, prepending to
the command line). The new `bin/mvn` approach requires `MAVEN_ARGS` to survive
in the environment of the shell process — but that assumption doesn't hold when
Maven is invoked via `maven-executor` (which covers all ITs in the Maven
project itself).
This means the breakage isn't just a test-writing issue: any user or tool
that invokes Maven via `maven-executor` (or anything that strips `MAVEN_ARGS`
from the env) would silently lose all `MAVEN_ARGS` content with this PR, since
the old passthrough via `eval` is gone.
**Suggestion**
One way to reconcile this: keep the new shell parser for the quoting
feature, but also retain a fallback so that `MAVEN_ARGS` is still forwarded
even when it isn't parsed (e.g. fall back to the old eval approach when the env
var is non-empty but the parsing block doesn't run — or make the parsing block
unconditional of the env var being set in the subprocess by reading it
differently). Alternatively, the IT could be written to bypass `maven-executor`
and invoke `bin/mvn` via a plain `ProcessBuilder` to test the shell quoting in
isolation.
--
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]
