bmarwell commented on code in PR #11365:
URL: https://github.com/apache/maven/pull/11365#discussion_r2582855879
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -166,30 +166,55 @@ find_file_argument_basedir() {
}
# concatenates all lines of a file and replaces variables
+# Uses Java-based parser to handle all special characters correctly
+# This avoids shell parsing issues with pipes, quotes, @, and other special
characters
+# and ensures POSIX compliance (no xargs -0, awk, or complex sed needed)
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' ' '
+ # Compile and run JvmConfigParser
+ # Use a temporary directory for compilation to avoid conflicts
+ jvm_parser_dir="${TMPDIR:-/tmp}/mvn-jvm-parser-$$"
+ mkdir -p "$jvm_parser_dir"
+
+ # Compile the parser
+ "$JAVACMD" -version >/dev/null 2>&1 || {
+ echo "Error: Java not found. Please set JAVA_HOME." >&2
+ return 1
+ }
+
+ javac_cmd="${JAVACMD%java*}javac"
+ if [ ! -x "$javac_cmd" ]; then
+ # Try to find javac in JAVA_HOME
+ if [ -n "$JAVA_HOME" ]; then
+ javac_cmd="$JAVA_HOME/bin/javac"
+ fi
+ fi
+
+ "$javac_cmd" -d "$jvm_parser_dir" "$MAVEN_HOME/bin/JvmConfigParser.java"
>/dev/null 2>&1
Review Comment:
Why `javac`? We are on 17, we can just do `java JvmConfigParser.java`
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -166,30 +166,55 @@ find_file_argument_basedir() {
}
# concatenates all lines of a file and replaces variables
+# Uses Java-based parser to handle all special characters correctly
+# This avoids shell parsing issues with pipes, quotes, @, and other special
characters
+# and ensures POSIX compliance (no xargs -0, awk, or complex sed needed)
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' ' '
+ # Compile and run JvmConfigParser
+ # Use a temporary directory for compilation to avoid conflicts
+ jvm_parser_dir="${TMPDIR:-/tmp}/mvn-jvm-parser-$$"
Review Comment:
ID reuse -- is ist still a thing on some Unixes? I do not know, but I guess
this should be fine.
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -177,38 +177,25 @@ cd /d "%EXEC_DIR%"
:endDetectBaseDir
+rem Initialize JVM_CONFIG_MAVEN_OPTS to empty to avoid inheriting from
environment
+set JVM_CONFIG_MAVEN_OPTS=
+
if not exist "%MAVEN_PROJECTBASEDIR%\.mvn\jvm.config" goto endReadJvmConfig
-@setlocal EnableExtensions EnableDelayedExpansion
-set JVM_CONFIG_MAVEN_OPTS=
-for /F "usebackq tokens=* delims=" %%a in
("%MAVEN_PROJECTBASEDIR%\.mvn\jvm.config") do (
- set "line=%%a"
-
- rem Skip empty lines and full-line comments
- echo !line! | findstr /b /r /c:"[ ]*#" >nul
- if errorlevel 1 (
- rem Handle end-of-line comments by taking everything before #
- for /f "tokens=1* delims=#" %%i in ("!line!") do set "line=%%i"
-
- rem Trim leading/trailing spaces while preserving spaces in quotes
- set "trimmed=!line!"
- for /f "tokens=* delims= " %%i in ("!trimmed!") do set "trimmed=%%i"
- for /l %%i in (1,1,100) do if "!trimmed:~-1!"==" " set
"trimmed=!trimmed:~0,-1!"
-
- rem Replace MAVEN_PROJECTBASEDIR placeholders
- set "trimmed=!trimmed:${MAVEN_PROJECTBASEDIR}=%MAVEN_PROJECTBASEDIR%!"
- set "trimmed=!trimmed:$MAVEN_PROJECTBASEDIR=%MAVEN_PROJECTBASEDIR%!"
-
- if not "!trimmed!"=="" (
- if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
- set "JVM_CONFIG_MAVEN_OPTS=!trimmed!"
- ) else (
- set "JVM_CONFIG_MAVEN_OPTS=!JVM_CONFIG_MAVEN_OPTS! !trimmed!"
- )
- )
- )
-)
-@endlocal & set JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%
+rem Use Java to parse jvm.config to avoid batch script parsing issues with
special characters
+rem This handles pipes, quotes, @, and other special characters correctly
+rem Use random temp directory to avoid conflicts between different Maven
invocations
+set "JVM_CONFIG_PARSER_DIR=%TEMP%\mvn-jvm-parser-%RANDOM%-%RANDOM%"
+mkdir "%JVM_CONFIG_PARSER_DIR%"
+set "JVM_CONFIG_TEMP=%TEMP%\mvn-jvm-config-%RANDOM%.txt"
+"%JAVACMD:java.exe=javac.exe%" -d "%JVM_CONFIG_PARSER_DIR%"
"%MAVEN_HOME%\bin\JvmConfigParser.java" >nul 2>&1
Review Comment:
Same as on posix/unix: why run `javac`? We are on 17 and can easily run
single-source files using `java`.
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Stream;
+
+/**
+ * Parses .mvn/jvm.config file for Windows batch scripts.
+ * This avoids the complexity of parsing special characters (pipes, quotes,
etc.) in batch scripts.
+ *
+ * Usage: java JvmConfigParser.java <jvm.config-path> <maven-project-basedir>
+ *
+ * Outputs: Single line with space-separated quoted arguments (safe for batch
scripts)
+ */
+public class JvmConfigParser {
+ public static void main(String[] args) {
+ if (args.length != 2) {
+ System.err.println("Usage: java JvmConfigParser.java
<jvm.config-path> <maven-project-basedir>");
+ System.exit(1);
+ }
+
+ Path jvmConfigPath = Paths.get(args[0]);
+ String mavenProjectBasedir = args[1];
+
+ if (!Files.exists(jvmConfigPath)) {
+ // No jvm.config file - output nothing
+ return;
+ }
+
+ try (Stream<String> lines = Files.lines(jvmConfigPath,
StandardCharsets.UTF_8)) {
+ StringBuilder result = new StringBuilder();
+
+ lines.forEach(line -> {
+ // Remove comments
+ int commentIndex = line.indexOf('#');
+ if (commentIndex >= 0) {
+ line = line.substring(0, commentIndex);
+ }
+
+ // Trim whitespace
+ line = line.trim();
+
+ // Skip empty lines
+ if (line.isEmpty()) {
+ return;
+ }
+
+ // Replace MAVEN_PROJECTBASEDIR placeholders
+ line = line.replace("${MAVEN_PROJECTBASEDIR}",
mavenProjectBasedir);
+ line = line.replace("$MAVEN_PROJECTBASEDIR",
mavenProjectBasedir);
+
+ // Parse line into individual arguments (split on spaces,
respecting quotes)
+ List<String> parsed = parseArguments(line);
+
+ // Append each argument quoted
+ for (String arg : parsed) {
+ if (result.length() > 0) {
+ result.append(' ');
+ }
+ result.append('"').append(arg).append('"');
+ }
+ });
+
+ System.out.print(result.toString());
+ System.out.flush(); // Ensure output is flushed before exit
(important on Windows)
Review Comment:
imo the comment is not needed
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Stream;
+
+/**
+ * Parses .mvn/jvm.config file for Windows batch scripts.
+ * This avoids the complexity of parsing special characters (pipes, quotes,
etc.) in batch scripts.
+ *
+ * Usage: java JvmConfigParser.java <jvm.config-path> <maven-project-basedir>
+ *
+ * Outputs: Single line with space-separated quoted arguments (safe for batch
scripts)
+ */
+public class JvmConfigParser {
+ public static void main(String[] args) {
+ if (args.length != 2) {
+ System.err.println("Usage: java JvmConfigParser.java
<jvm.config-path> <maven-project-basedir>");
+ System.exit(1);
+ }
+
+ Path jvmConfigPath = Paths.get(args[0]);
+ String mavenProjectBasedir = args[1];
+
+ if (!Files.exists(jvmConfigPath)) {
+ // No jvm.config file - output nothing
+ return;
+ }
+
+ try (Stream<String> lines = Files.lines(jvmConfigPath,
StandardCharsets.UTF_8)) {
+ StringBuilder result = new StringBuilder();
+
+ lines.forEach(line -> {
+ // Remove comments
+ int commentIndex = line.indexOf('#');
+ if (commentIndex >= 0) {
+ line = line.substring(0, commentIndex);
+ }
+
+ // Trim whitespace
+ line = line.trim();
+
+ // Skip empty lines
+ if (line.isEmpty()) {
+ return;
+ }
+
+ // Replace MAVEN_PROJECTBASEDIR placeholders
+ line = line.replace("${MAVEN_PROJECTBASEDIR}",
mavenProjectBasedir);
+ line = line.replace("$MAVEN_PROJECTBASEDIR",
mavenProjectBasedir);
+
+ // Parse line into individual arguments (split on spaces,
respecting quotes)
+ List<String> parsed = parseArguments(line);
+
+ // Append each argument quoted
+ for (String arg : parsed) {
+ if (result.length() > 0) {
+ result.append(' ');
+ }
+ result.append('"').append(arg).append('"');
+ }
+ });
+
+ System.out.print(result.toString());
+ System.out.flush(); // Ensure output is flushed before exit
(important on Windows)
+ } catch (IOException e) {
+ System.err.println("Error reading jvm.config: " + e.getMessage());
+ System.exit(1);
Review Comment:
This system.exit makes testing a potential IOException problematic
##########
apache-maven/src/assembly/maven/bin/JvmConfigParser.java:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Stream;
+
+/**
+ * Parses .mvn/jvm.config file for Windows batch scripts.
+ * This avoids the complexity of parsing special characters (pipes, quotes,
etc.) in batch scripts.
+ *
+ * Usage: java JvmConfigParser.java <jvm.config-path> <maven-project-basedir>
+ *
+ * Outputs: Single line with space-separated quoted arguments (safe for batch
scripts)
+ */
+public class JvmConfigParser {
+ public static void main(String[] args) {
+ if (args.length != 2) {
+ System.err.println("Usage: java JvmConfigParser.java
<jvm.config-path> <maven-project-basedir>");
+ System.exit(1);
+ }
+
+ Path jvmConfigPath = Paths.get(args[0]);
+ String mavenProjectBasedir = args[1];
+
+ if (!Files.exists(jvmConfigPath)) {
+ // No jvm.config file - output nothing
+ return;
+ }
+
+ try (Stream<String> lines = Files.lines(jvmConfigPath,
StandardCharsets.UTF_8)) {
+ StringBuilder result = new StringBuilder();
+
+ lines.forEach(line -> {
+ // Remove comments
+ int commentIndex = line.indexOf('#');
+ if (commentIndex >= 0) {
+ line = line.substring(0, commentIndex);
+ }
+
+ // Trim whitespace
+ line = line.trim();
+
+ // Skip empty lines
+ if (line.isEmpty()) {
+ return;
+ }
+
+ // Replace MAVEN_PROJECTBASEDIR placeholders
+ line = line.replace("${MAVEN_PROJECTBASEDIR}",
mavenProjectBasedir);
+ line = line.replace("$MAVEN_PROJECTBASEDIR",
mavenProjectBasedir);
+
+ // Parse line into individual arguments (split on spaces,
respecting quotes)
+ List<String> parsed = parseArguments(line);
+
+ // Append each argument quoted
+ for (String arg : parsed) {
+ if (result.length() > 0) {
+ result.append(' ');
+ }
+ result.append('"').append(arg).append('"');
+ }
+ });
Review Comment:
Long lambdas are unreadable, and also impact performance. I would use a
traditional for loop (extracted to a method) here and put the contents into a
new method.
I do not know how much faster it is, but besides performance, lambdas are
also harder to identify in the debugger.
--
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]
