gnodet-bot commented on code in PR #13478: URL: https://github.com/apache/maven/pull/13478#discussion_r4260867471
########## apache-maven/src/test/scripts/test-mvn-args.sh: ########## @@ -0,0 +1,248 @@ +#!/bin/sh +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# ----------------------------------------------------------------------------- +# Tests how bin/mvn hands the MAVEN_ARGS environment variable over to Maven. +# +# The test runs on any POSIX platform: a throw-away Maven home is populated with +# the real bin/mvn, and uname(1), dirname(1) and java(1) are stubbed out so that +# the JVM command line can be asserted upon, one bracketed token per argument. +# +# The launcher is always run with a PATH holding nothing but those stubs, so it +# may only rely on shell built-ins, as the stubs themselves do. +# +# Usage: sh apache-maven/src/test/scripts/test-mvn-args.sh +# +# Exits with 0 when all assertions pass, 1 otherwise. +# ----------------------------------------------------------------------------- + +set -e + +script_dir=`cd "\`dirname "$0"\`" && pwd` +mvn_script="$script_dir/../../assembly/maven/bin/mvn" + +if [ ! -f "$mvn_script" ]; then + echo "Cannot locate the mvn script at $mvn_script" >&2 + exit 1 +fi + +# Absolute path to the shell: it has to be invoked while PATH holds the stubs only. +sh_bin=`unset -f command; command -v sh` + +work_dir=`mktemp -d "${TMPDIR:-/tmp}/mvn-args.XXXXXX"` +work_dir=`cd "$work_dir" && pwd` +trap 'rm -rf "$work_dir"' EXIT INT TERM + +stub_dir="$work_dir/stubs" +maven_home="$work_dir/maven-home" +project_dir="$work_dir/project" + +mkdir -p "$stub_dir" "$maven_home/bin" "$maven_home/boot" "$project_dir" + +cp "$mvn_script" "$maven_home/bin/mvn" +touch "$maven_home/bin/m2.conf" "$maven_home/boot/plexus-classworlds-9.9.9.jar" + +# Files matching a glob, to prove that MAVEN_ARGS is never expanded as a pattern. +touch "$project_dir/one.txt" "$project_dir/two.txt" + +cat > "$stub_dir/uname" <<'STUB' +#!/bin/sh +echo Linux +STUB + +# dirname(1) stub: bin/mvn needs it to locate the Maven home. +cat > "$stub_dir/dirname" <<'STUB' +#!/bin/sh +path="$1" +case "$path" in + */*) + path="${path%/*}" + [ -n "$path" ] || path="/" + ;; + *) path="." ;; +esac +printf '%s\n' "$path" +STUB + +# java(1) stub: answers version probes, otherwise echoes the arguments it was +# invoked with, one bracketed token per argument. +cat > "$stub_dir/java" <<'STUB' +#!/bin/sh +case " $* " in + *" -version "*) exit 0 ;; +esac + +for arg in "$@"; do printf '[%s]' "$arg"; done +printf '\n' +STUB + +chmod +x "$stub_dir/uname" "$stub_dir/dirname" "$stub_dir/java" "$maven_home/bin/mvn" + +failures=0 + +# The text following the launcher class: the arguments Maven itself receives. +launcher='[org.codehaus.plexus.classworlds.launcher.Launcher]' + +# maven_args <MAVEN_ARGS value> [args...] +# Prints the arguments handed over to Maven. +maven_args() { + value="$1" + shift + output=`cd "$project_dir" && + MAVEN_ARGS="$value" PATH="$stub_dir" JAVA_HOME= MAVEN_SKIP_RC=1 \ + "$sh_bin" "$maven_home/bin/mvn" "$@" 2>/dev/null || :` + printf '%s' "${output#*"$launcher"}" +} + +# maven_args_stderr <MAVEN_ARGS value> [args...] +# Prints what the launcher wrote to the standard error. +maven_args_stderr() { + value="$1" + shift + ( cd "$project_dir" && + MAVEN_ARGS="$value" PATH="$stub_dir" JAVA_HOME= MAVEN_SKIP_RC=1 \ + "$sh_bin" "$maven_home/bin/mvn" "$@" 2>&1 >/dev/null || : ) +} + +# assert_equals <description> <expected> <actual> +assert_equals() { + if [ "$2" = "$3" ]; then + printf 'ok - %s\n' "$1" + else + printf 'FAILED - %s\n' "$1" + printf ' expected: %s\n' "$2" + printf ' actual: %s\n' "$3" + failures=$((failures + 1)) + fi +} + +# assert_contains <description> <haystack> <needle> +assert_contains() { + case "$2" in + *"$3"*) printf 'ok - %s\n' "$1" ;; + *) + printf 'FAILED - %s\n' "$1" + printf ' expected to contain: %s\n' "$3" + printf ' actual: %s\n' "$2" + failures=$((failures + 1)) + ;; + esac +} + +# check <description> <expected> <MAVEN_ARGS value> [args...] +check() { + description="$1" + expected="$2" + value="$3" + shift 3 + actual=`maven_args "$value" "$@"` + assert_equals "$description" "$expected" "$actual" +} + +nl=' +' +tab=`printf '\t'` +dq='"' +sq="'" + +check "no MAVEN_ARGS: only the command line arguments" \ + '[validate]' '' validate + +check "white space only: no argument" \ + '[validate]' ' ' validate Review Comment: 💡 **Suggestion:** Consider adding a test for whitespace-only *inside* quotes, e.g. `MAVEN_ARGS="' '"`. The current suite covers: - unquoted whitespace-only (`' '` → no argument) - empty double-quoted word (`""` → empty argument) - paths with spaces inside quotes But a quoted single-space (`' '`) would produce one argument containing a space character, which is a distinct edge case worth covering explicitly. ########## apache-maven/src/assembly/maven/bin/mvn: ########## @@ -557,22 +558,74 @@ fi cmd="$cmd $LAUNCHER_CLASS" +# MAVEN_ARGS is only passed for the default Maven build command (MavenCling), +# not for sub-commands like --up, --enc, --shell or --val which have their own options. +# +# Its words are separated by white space, and single or double quotes group a word +# containing white space, so that a path with a space can be passed. +# Nothing else is interpreted: no backslash escape, no ${...} placeholder, no globbing +# and no command substitution, so the values reach Maven as they were written. +# A word is made of adjacent quoted and unquoted parts, and "" is an empty word. +# The words go in front of the user arguments, "$@" is rotated to get them in order. +if [ "$MAVEN_MAIN_CLASS" = "org.apache.maven.cling.MavenCling" ] && [ -n "$MAVEN_ARGS" ]; then + maven_args_user_count=$# + maven_args_rest=$MAVEN_ARGS + maven_args_word= + maven_args_in_word=false + maven_args_quote= + maven_args_tab=`printf '\t'` + maven_args_nl=' +' + while [ -n "$maven_args_rest" ]; do + maven_args_tail=${maven_args_rest#?} + maven_args_char=${maven_args_rest%"$maven_args_tail"} + maven_args_rest=$maven_args_tail + if [ -n "$maven_args_quote" ]; then + if [ "$maven_args_char" = "$maven_args_quote" ]; then + maven_args_quote= + else + maven_args_word=$maven_args_word$maven_args_char + fi + else + case $maven_args_char in + '"' | "'") + maven_args_quote=$maven_args_char + maven_args_in_word=true + ;; + ' ' | "$maven_args_tab" | "$maven_args_nl") + if [ "$maven_args_in_word" = true ]; then + set -- "$@" "$maven_args_word" + maven_args_word= + maven_args_in_word=false + fi + ;; + *) + maven_args_word=$maven_args_word$maven_args_char + maven_args_in_word=true + ;; + esac + fi + done + if [ -n "$maven_args_quote" ]; then + echo "Warning: MAVEN_ARGS has an unterminated $maven_args_quote quote, the end of the value is taken as is" >&2 Review Comment: 💡 **Suggestion:** The warning message could be slightly more explicit about what "taken as is" means: ```suggestion echo "Warning: MAVEN_ARGS has an unterminated $maven_args_quote quote, the remaining characters are treated as part of the last word" >&2 ``` "taken as is" is a bit vague — clarifying that the trailing unquoted characters are appended to the last word makes the behavior clearer to the user. -- 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]
