gnodet-bot commented on code in PR #10906:
URL: https://github.com/apache/maven/pull/10906#discussion_r4258912690
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -125,6 +125,13 @@ public final int invoke(InvokerRequest invokerRequest) {
oldProps.putAll(System.getProperties());
ClassLoader oldCL = Thread.currentThread().getContextClassLoader();
try (C context = createContext(invokerRequest)) {
+ // MSYS2 and Git Bash hand over Windows paths in their POSIX
style, e.g. /c/Users/alice
+ String userHome = System.getProperty("user.home");
+ String nativeUserHome = MsysPaths.toNativePath(userHome);
+ if (!Objects.equals(userHome, nativeUserHome)) {
+ System.setProperty("user.home", nativeUserHome);
+ context.logger.debug("Converted user.home '" + userHome + "'
to '" + nativeUserHome + "'");
Review Comment:
⚠️ **Timing bug: `user.home` is fixed too late**
The conversion of `user.home` happens **after**
`createContext(invokerRequest)` has already run (line 127). By the time we
reach line 129, `context.userDirectory` has already been captured from
`invokerRequest.userHomeDirectory()`, which was set at parse time via
`Paths.get(System.getProperty("user.home"))` in
`BaseParser.getUserHomeDirectory()`.
This means:
- `context.userDirectory` still holds the MSYS-style path (`/c/Users/alice`)
- Settings resolution (uses `context.userDirectory`) → wrong path
- Default local repo path derivation → wrong path
- Extension loading → wrong path
Updating `System.setProperty("user.home", ...)` at this point only affects
future lookups, not the context object already constructed.
**Fix:** Move this conversion **before** `createContext()`, or re-derive
`context.userDirectory` after the fix. The simplest approach: apply it before
line 127, or pass the converted value through `InvokerRequest`.
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -441,6 +461,25 @@ void initialize(CliRequest cliRequest) throws
ExitException {
//
String mavenHome = System.getProperty(Constants.MAVEN_HOME);
+ if (System.getProperty("os.name",
"").toLowerCase(Locale.ROOT).startsWith("windows")) {
+ final String userHome = System.getProperty("user.home");
+ if (userHome != null) {
+ System.setProperty("user.home", msysToWindowsPath(userHome));
+ }
+
+ for (final String key : new String[] {
+ Constants.MAVEN_REPO_LOCAL, // -Dmaven.repo.local
+ "user.settings", // -s / --settings
+ "alternateSettings",
+ "user.toolchains"
+ }) {
+ final String value = System.getProperty(key);
+ if (value != null) {
+ System.setProperty(key, msysToWindowsPath(value));
+ }
+ }
+ }
Review Comment:
ℹ️ **Compat layer adds new behavior with no Maven 3.x equivalent**
`MavenCli` is `@Deprecated` (compat shim for Maven 3 embedders). The new
MSYS path normalization added to `initialize()` is a behavioral change in this
layer — Maven 3's `MavenCli` had no such logic.
This may be intentional (fixing the compat path for Windows users still on
the old entry point), but it warrants a note in the PR description or a comment
clarifying whether parity with Maven 3's behavior is expected here. If Maven 3
didn't do this conversion, embedders relying on the compat layer may get
unexpected behavior (paths silently rewritten).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/MsysPaths.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.
+ */
+package org.apache.maven.cling.invoker;
+
+import java.util.Locale;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Converts the paths MSYS2 and Git Bash hand over in their POSIX style, like
{@code /c/Users/alice}, to native Windows
+ * paths, like {@code C:\Users\alice}.
+ */
+final class MsysPaths {
+
+ /** A drive letter, the only single letter directory right below the root
of such a path. */
+ private static final Pattern DRIVE_PATH =
Pattern.compile("^/([a-zA-Z])/(.*)$");
+
+ private MsysPaths() {}
+
+ /**
+ * Converts the path on Windows, and returns it as it is anywhere else.
+ *
+ * @param path the path, may be {@code null}
+ * @return the native path, or {@code null} if the path is {@code null}
+ */
+ static String toNativePath(String path) {
+ return toNativePath(path, isWindows());
+ }
+
+ static String toNativePath(String path, boolean windows) {
Review Comment:
💡 **Missing Javadoc on the `boolean windows` parameter**
The two-argument overload has no Javadoc at all. The `windows` parameter's
purpose (override OS detection for testing) is non-obvious — add at minimum a
`@param windows` tag explaining its intent:
```suggestion
/**
* Converts the path if {@code windows} is {@code true}, otherwise
returns it unchanged.
* The boolean parameter exists to allow OS-independent testing.
*
* @param path the path, may be {@code null}
* @param windows {@code true} to apply the conversion regardless of the
actual OS
* @return the native path, or {@code null} if the path is {@code null}
*/
static String toNativePath(String path, boolean windows) {
```
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -211,6 +211,26 @@ public class MavenCli {
private static final Pattern NEXT_LINE = Pattern.compile("\r?\n");
+ /** Matches a path starting with a drive letter, like /d/whatever or
/C/... */
+ private static final Pattern MSYS_PATH =
Pattern.compile("^/([a-zA-Z])/(.*)$");
+
+ /**
+ * Converts a POSIX-style Windows path as used by MSYS2 and Git Bash,
+ * e.g. "/c/Users/alice/file.txt", to a native Windows path
"C:\Users\alice\file.txt".
+ * Returns the original string if it is not such a path, a native Windows
path for instance.
+ */
+ static String msysToWindowsPath(final String path) {
+ if (path == null) {
+ return null;
+ }
+ final Matcher matcher = MSYS_PATH.matcher(path);
+ if (matcher.matches()) {
+ return Character.toUpperCase(matcher.group(1).charAt(0)) + ":\\"
+ + matcher.group(2).replace('/', '\\');
+ }
Review Comment:
🔁 **Logic duplicated between `MavenCli.msysToWindowsPath()` and
`MsysPaths.toNativePath()`**
Both methods implement the same regex + backslash-replacement logic
independently, with no shared code between them. `MavenCli` doesn't import or
reference `MsysPaths`.
Since `MavenCli` lives in the compat layer (`compat/maven-embedder`) and
`MsysPaths` lives in the impl layer (`impl/maven-cli`), cross-module reuse may
require a small structural change — but the alternative is maintaining two
copies of the same logic. At minimum, add a comment noting that both must be
kept in sync, or extract to a shared utility.
--
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]
