-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Replace xargs -0 with POSIX-compliant alternative (fixes #11485) #11489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @gnodet, I just checked. For AIX: we could work around that by requiring AIX users of Maven to use the "Open Source Toolbox". I tested that successfully. However, the assumption is correct that Given we can work around this OR just require it anyway, I am open to either solution. |
18ab53a to
fbdb26b
Compare
The xargs -0 option is not POSIX-compliant and is not available on AIX, FreeBSD, and other systems. This commit replaces the use of xargs -0 in the concat_lines function with a pure shell solution that reads NUL-terminated strings using the read builtin with -d '' option. The new implementation: - Maintains the same functionality of handling special characters like pipes, quotes, and spaces correctly - Avoids the non-portable xargs -0 option - Uses a while loop with read -r -d '' to process NUL-terminated strings - Builds the result string incrementally instead of using tr to convert newlines to spaces This fix ensures that the mvn script works correctly on all POSIX-compliant systems while preserving the behavior introduced in commit aeff353 to handle quoted pipes and other special characters in .mvn/jvm.config. Fixes apache#11485
fbdb26b to
3f5f5d4
Compare
| result="" | ||
| # Read the file line by line | ||
| while IFS= read -r line || [ -n "$line" ]; do | ||
| # Convert CR to LF | ||
| line=$(echo "$line" | tr '\r' '\n') | ||
| # Remove comments | ||
| line=$(echo "$line" | sed 's/#.*$//') | ||
| # Skip empty lines | ||
| [ -z "$(echo "$line" | tr -d ' \t')" ] && continue | ||
|
|
||
| # Process each argument in the line using eval to handle quotes | ||
| eval "set -- $line" | ||
| for arg in "$@"; do | ||
| # Replace variables | ||
| arg=$(echo "$arg" | sed \ | ||
| -e "s@\${MAVEN_PROJECTBASEDIR}@$MAVEN_PROJECTBASEDIR@g" \ | ||
| -e "s@\$MAVEN_PROJECTBASEDIR@$MAVEN_PROJECTBASEDIR@g") | ||
|
|
||
| # Quote the argument if it contains spaces or special shell characters | ||
| case "$arg" in | ||
| *[\ \|\&\;\<\>\(\)\$\`\\\"\'\~\*\?\[\]\#\~\=]*) | ||
| arg="\"$arg\"" | ||
| ;; | ||
| esac | ||
|
|
||
| if [ -n "$result" ]; then | ||
| result="$result $arg" | ||
| else | ||
| result="$arg" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- imho this should be an extra function
- we might want to document why we do not use
xargs -0here and that we agreed to use posix-only arguments (did we agree on that?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by extra function. The concat_lines function is the code above (but for an if which verifies the argument is actually a file...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought we might want to extract a method/function here, because we get a lot of indentation and the method gets longer and longer.
Did we agree on "we must be POSIX compliant"? If not, we could leave xargs in here and just concentrate on the sed part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I think 30 lines is still manageable.
-
Yes, unless there's a strong reason, I think keeping POSIX compliant script is better.
maven-wrapperhas an additional job to validate posix compliance, we may want to do the same: https://github.com/apache/maven-wrapper/blob/master/.github/workflows/shellcheck-posix.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I fully agree!
bmarwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIX is gloriously stubborn — ancient userland tools (no GNU xargs -0) and a nonstandard /bin/sh make portability a landmine, so avoiding non‑POSIX flags is absolutely the right goal.
| [ -z "$(echo "$line" | tr -d ' \t')" ] && continue | ||
|
|
||
| # Process each argument in the line using eval to handle quotes | ||
| eval "set -- $line" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows command substitution/globbing -> unsafe and can execute backticks/$() from .mvn/jvm.config. Is this wanted? Seems dangerous, we might want to at least document this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could switch to a solution similar to the one I tried on windows for #11365, i.e. using java to compile the JvmConfigParser
| # Do not use `xargs -0` as this is not POSIX-compliant | ||
| while IFS= read -r line || [ -n "$line" ]; do | ||
| # Convert CR to LF | ||
| line=$(echo "$line" | tr '\r' '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can produce embedded newlines that confuse splitting if \r is in the middle of a line. But I think that is problematic anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean in case of mixed end of lines style, with some \n, some \r and some \r\n ?
|
|
||
| # Quote the argument if it contains spaces or special shell characters | ||
| case "$arg" in | ||
| *[\ \|\&\;\<\>\(\)\$\`\\\"\'\~\*\?\[\]\#\~\=]*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fragile. Does work correctly on backticks and double quotes? If backticks or double quotes are inside, should we not use `arg="'$arg'" instead?
This commit unifies the .mvn/jvm.config parsing approach by using
JvmConfigParser.java on both Unix and Windows platforms, solving
multiple related issues with special character handling.
Issues Fixed:
- MNG-11363: Pipe symbols (|) in jvm.config cause shell parsing errors
- MNG-11485: Non-POSIX xargs -0 fails on AIX, FreeBSD, and other systems
- MNG-11486: sed command fails when paths contain @ symbols
Changes:
- Modified mvn (Unix): Replaced awk/sed pipeline with JvmConfigParser.java
compilation and execution. This ensures POSIX compliance and handles
all special characters (pipes, quotes, @, etc.) correctly.
- Modified mvn.cmd (Windows): Removed debug output and updated comments
to reflect all special characters now handled correctly.
- Added integration test MavenITgh11485AtSignInJvmConfigTest to verify
@ symbol handling in jvm.config files (important for Jenkins workspaces
like workspace/project_PR-350@2).
The Java-based parser approach:
- Handles comment removal (both # at start and end-of-line)
- Performs variable substitution (${MAVEN_PROJECTBASEDIR})
- Correctly parses quoted arguments with special characters
- Works consistently across all platforms
- Is POSIX-compliant (no xargs -0, awk, or complex sed needed)
Performance Impact:
Unix script is ~200ms slower (320ms → 520ms for mvn -v) due to Java
compilation, but this is acceptable for the correctness, maintainability,
and cross-platform consistency benefits.
Related PRs:
- Fixes apache#11365 (MNG-11363: pipe symbols)
- Fixes apache#11489 (MNG-11485: POSIX compliance)
- Fixes apache#11499 (MNG-11486: @ symbols in paths)
|
This PR has been superseded by #11365, which now fixes all three related issues:
The unified solution uses Integration tests from this PR have been added to #11365. |
Description
This PR fixes #11485 by replacing the non-POSIX-compliant
xargs -0option with a pure shell solution that works on all POSIX-compliant systems including AIX, FreeBSD, and others.Problem
The
xargs -0option was introduced in commit aeff353 to handle special characters (pipes, quotes, spaces) in.mvn/jvm.configfiles. However, this option is not part of the POSIX standard and is not available on AIX, FreeBSD, and other systems, causing the Maven wrapper to fail on these platforms.Solution
The fix replaces
xargs -0with a pure shell solution that:readbuiltin with-d ''option to read NUL-terminated stringsChanges
concat_lines()function inapache-maven/src/assembly/maven/bin/mvnxargs -n 1 -0with a while loop usingread -r -d ''Testing
concat_linesfunction with special charactersmvn clean installCompatibility
This change maintains backward compatibility while extending support to more platforms. The behavior for handling special characters in
.mvn/jvm.configremains unchanged.Pull Request opened by Augment Code with guidance from the PR author