Avoid parsing MAVEN_OPTS (master/4.x)

[BobVul]

Fixes #10937 by introducing an additional INTERNAL_MAVEN_OPTS for any arguments that need to be inserted by the script. Parsing the externally-defined MAVEN_OPTS variable can lead to incorrect processing of quotes and special characters, so use the separate variable to avoid doing so.

Additionally JVM_CONFIG_MAVEN_OPTS is introduced as its own variable to preserve the append behaviour.

Specifically, this fixes this case:

$env:MAVEN_OPTS='"-Dfoo=bar|baz"'
mkdir .mvn
New-Item .mvn\jvm.config
mvn

by implementing proposed fix 2 from #10937

The related fix for 3.9.x branch is in PR #10969

---

Following this checklist to help us incorporate your
contribution quickly and easily:

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
  Note that commits might be squashed by a maintainer on merge.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  This may not always be possible but is a best-practice.
• Run mvn verify to make sure basic checks pass.
  A more thorough check will be performed on your pull request automatically.
• You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[BobVul]

Sorry, I think I did a rename of the folder before the last push that moved it over the line length limit. Fixed now.

[BobVul]

Looks like in 4.x the bash script also changed, such that it now evals the command and introduces a similar pipe parsing error in a completely different way.

The changes made here (purely to the Windows .cmd!) should have no impact on that, but the new tests do expose that issue.

I'll consolidate findings/discussion back in issue #10937 but I'm honestly not too sure what to do here -- complex bash scripts are not my forte.

[BobVul]

@gnodet Since you made the original change in #2213, could I run this by you, as I'm a bit unsure.

The existing behaviour is:

1. The change there updated concat_lines to perform more advanced parsing of jvm.config.
2. In the process it runs the lines through xargs, which processes quotes specially, in the process stripping them from the input
3. Later, quotes are added back in if the line is not surrounded by them and contains spaces (i.e. first and last char only)

But the behaviour seen with pipes specifically is that the quotes are being stripped but not added back in. i.e. -Dopt="foo|bar" is transformed into -Dopt=foo|bar and causing problems on Linux. I can see two ways of handling this:

1. Add pipes to the "check for spaces" bit, which will then re-surround the entire line in quotes. This would probably work but seems a tad hacky?
2. Change out the xargs -n 1 command for tr '\n' '\0' | xargs -n 1 -0, i.e. use xargs in NUL mode. This prevents xargs from handling quotes specially, and therefore quotes are not stripped from the input. However, this also means the code that re-adds quotes must be removed else it will cause e.g. -Dopt="foo bar" to become "-Dopt="foo bar"" which effectively leaves the space unquoted again (much like the original issue on Windows!). I think this is probably fine since in this case xargs won't strip initial quotes, though it does force the user to add quotes manually.

I'm leaning towards using option 2, but I'm not familiar enough with either this area of code nor bash to be certain that it won't cause other issues. Do you have any suggestions here?

Proposed new concat_lines:

concat_lines() {
  if [ -f "$1" ]; then
    # First convert all CR to LF using tr
    tr '\r' '\n' < "$1" | \
    sed -e '/^$/d' -e 's/#.*$//' | \
    # Replace LF with NUL for xargs
    tr '\n' '\0' | \
    # Split into words and process each argument
    # Use -0 with NUL to avoid special behaviour on quotes
    xargs -n 1 -0 | \
    while read -r arg; do
      # Replace variables first
      arg=$(echo "$arg" | sed \
        -e "s@\${MAVEN_PROJECTBASEDIR}@$MAVEN_PROJECTBASEDIR@g" \
        -e "s@\$MAVEN_PROJECTBASEDIR@$MAVEN_PROJECTBASEDIR@g")

      echo "$arg"
    done | \
    tr '\n' ' '
  fi
}

e: This proposed new version passes both the new gh10937 test and the old mng4559 tests. So as far as what's tested, it appears to work correctly. I'll push the change to the PR but happy to rollback if it's the wrong approach.

e2: I just realised xargs -0 might not be available in POSIX xargs but it looks like it was added in issue 8 / 2024. And it's currently available in gnu/macos/(net|free|open)bsd/busybox so that's about as universal as I can find

[BobVul]

So um... what's the next step here? Is there anything else I should do here or on the related 3.9.x PR?

Also, what's the best way to get this into 4.0.x branch? I think the changes should be identical, should I wait for this one to be reviewed/merged first?

[gnodet]

LGTM, thx for this nice patch ! I'll do the backport to maven-4.0.x branch.

[gnodet]

💚 All backports created successfully

Status	Branch	Result
✅	maven-4.0.x

Questions ?

Please refer to the Backport tool documentation

[BobVul]

LGTM, thx for this nice patch ! I'll do the backport to maven-4.0.x branch.

Thank you!

Should the 3.9.x IT also be merged? apache/maven-integration-testing#407

[slawekjaranowski]

Should the 3.9.x IT also be merged? apache/maven-integration-testing#407

should 😄 done