gnodet commented on code in PR #11102:
URL: https://github.com/apache/maven/pull/11102#discussion_r3369568619
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/CommonsCliOptions.java:
##########
@@ -349,6 +360,14 @@ protected void
prepareOptions(org.apache.commons.cli.Options options) {
.longOpt("help")
.desc("Display help information")
.build());
+ options.addOption(Option.builder()
+ .longOpt(PROCESSES)
+ .desc("List running Maven processes for the current user")
+ .build());
+ options.addOption(Option.builder(PROCESSES_SHORT)
Review Comment:
Two options are registered with the same `longOpt(PROCESSES)` (the long-only
one on line 363 and the short+long one here). Commons CLI does not support
duplicate long option names -- the second `addOption` call will silently
overwrite the first, making the long-only version dead code. You should
register a single option.
Moreover, the comment on line 327 says "No short form: -ps already used" but
then `PROCESSES_SHORT = "p"` is defined and used. I would suggest dropping the
short option entirely and keeping only the `--processes` long form:
```suggestion
options.addOption(Option.builder()
.longOpt(PROCESSES)
.desc("List running Maven processes for the current user
and exit")
.build());
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/ProcessRuns.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.io.Reader;
+import java.io.Writer;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.time.Instant;
+import java.util.Comparator;
+import java.util.Properties;
+
+public final class ProcessRuns {
+
+ private static final String RUNS_SUBDIR = ".m2/.maven/runs";
+ private static final String FILE_PREFIX = "mvn-";
+ private static final String FILE_SUFFIX = ".properties";
+
+ private static final String KEY_PID = "pid";
+ private static final String KEY_VERSION = "version";
+ private static final String KEY_WORKDIR = "workDir";
+ private static final String KEY_EXECROOT = "execRoot";
+ private static final String KEY_STARTED = "started";
+
+ private static Path runsDir() {
+ final String home = System.getProperty("user.home", ".");
+ return Paths.get(home).resolve(RUNS_SUBDIR);
+ }
+
+ private static Path desc(final long pid) {
+ return runsDir().resolve(FILE_PREFIX + pid + FILE_SUFFIX);
+ }
+
+ public static void install(final long pid, final String version, final
Path workDir, final Path execRoot) {
+ try {
+ Files.createDirectories(runsDir());
+ final Properties p = new Properties();
+ p.setProperty(KEY_PID, Long.toString(pid));
+ p.setProperty(KEY_VERSION, version == null ? "-" : version);
+ p.setProperty(
+ KEY_WORKDIR,
+ workDir == null ? "?" :
workDir.toAbsolutePath().toString());
+ p.setProperty(
+ KEY_EXECROOT,
+ execRoot == null ? "?" :
execRoot.toAbsolutePath().toString());
+ p.setProperty(KEY_STARTED, Instant.now().toString());
+ try (Writer w = Files.newBufferedWriter(desc(pid),
StandardCharsets.UTF_8)) {
+ p.store(w, "mvn run");
+ }
+ } catch (final Exception ignored) {
+ // best-effort
+ }
+ }
+
+ public static void uninstall(final long pid) {
+ try {
+ Files.deleteIfExists(desc(pid));
+ } catch (final Exception ignored) {
+ // best-effort
+ }
+ }
+
+ private static final class Run {
+ final long pid;
+ final String version;
+ final String workDir;
+ final String execRoot;
+ final String started;
+
+ Run(final long pid, final String version, final String workDir, final
String execRoot, final String started) {
+ this.pid = pid;
+ this.version = version;
+ this.workDir = workDir;
+ this.execRoot = execRoot;
+ this.started = started;
+ }
+ }
+
+ public static java.util.List<Run> listAlive() {
+ final Path dir = runsDir();
+ if (!Files.isDirectory(dir)) {
+ return java.util.List.of();
+ }
+ final java.util.List<Run> out = new java.util.ArrayList<>();
+ try (DirectoryStream<Path> ds = Files.newDirectoryStream(dir,
FILE_PREFIX + "*" + FILE_SUFFIX)) {
+ for (final Path f : ds) {
+ final Properties p = new Properties();
+ try (Reader r = Files.newBufferedReader(f,
StandardCharsets.UTF_8)) {
+ p.load(r);
+ }
+ final long pid = parseLong(p.getProperty(KEY_PID), -1L);
+ final boolean alive = pid > 0
+ &&
ProcessHandle.of(pid).map(ProcessHandle::isAlive).orElse(false);
+ if (alive) {
+ out.add(new Run(
+ pid,
+ p.getProperty(KEY_VERSION, "-"),
+ p.getProperty(KEY_WORKDIR, "?"),
+ p.getProperty(KEY_EXECROOT, "?"),
+ p.getProperty(KEY_STARTED, "?")));
Review Comment:
`ProcessHandle.isAlive()` only checks whether the PID exists on the system
-- after a reboot or PID recycling, a stale descriptor file could match a
completely different process. Consider comparing the recorded `started`
timestamp against `ProcessHandle.info().startInstant()` to verify that the
process identity matches, not just the PID.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnsh/builtin/BuiltinShellCommandRegistryFactory.java:
##########
@@ -270,6 +273,22 @@ private List<Completer> mvnupCompleter(String name) {
.lookupMap(org.apache.maven.cling.invoker.mvnup.Goal.class)
.keySet())));
}
+
+ /**
+ * mvnsh command: print running Maven processes (delegates to `mvn
--processes`).
+ */
+ private void ps(CommandInput input) {
+ try {
+
shellMavenInvoker.invoke(mavenParser.parseInvocation(ParserRequest.mvn(
+ new String[] {"--processes"},
shellContext.invokerRequest.messageBuilderFactory())
+ .cwd(shellContext.cwd.get())
+ .build()));
+ } catch (InvokerException.ExitException e) {
Review Comment:
The `--processes` handler throws `ExitException(0)` on success, so this
catch block will always log an error message for a successful `ps` command. For
exit code 0, this should not be logged as an error. Consider:
```java
} catch (InvokerException.ExitException e) {
if (e.getExitCode() != 0) {
shellContext.logger.error("ps command exited with exit code " +
e.getExitCode());
}
}
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/CommonsCliOptions.java:
##########
@@ -228,6 +228,13 @@ public Optional<Boolean> help() {
return Optional.empty();
}
+ public Optional<Boolean> processes() {
Review Comment:
The check `commandLine.hasOption(CLIManager.PROCESSES) ||
commandLine.hasOption(CLIManager.PROCESSES_SHORT)` is redundant when both
options share the same `longOpt`. `hasOption` with the long name will match
regardless of whether the user specified the short or long form. If you keep
only the long form (as suggested), this simplifies to just
`commandLine.hasOption(CLIManager.PROCESSES)`.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/MavenInvoker.java:
##########
@@ -613,4 +615,26 @@ protected void logSummary(
logSummary(context, child, references, indent);
}
}
+
+ @Override
+ protected void preCommands(final MavenContext context) throws Exception {
+ super.preCommands(context);
+
+ if (context.options().processes().orElse(false)) {
+ final String out = ProcessRuns.format(ProcessRuns.listAlive());
+ System.out.print(out);
Review Comment:
Using `System.out.print` bypasses the Maven output infrastructure. The
established pattern in `LookupInvoker.helpOrVersionAndMayExit()` is to use
`determineWriter(context)` for early-exit output. This should follow the same
approach.
Additionally, the `--processes` check is conceptually an early-exit flag
like `--help` and `--version`. It would be more appropriate to handle it in
`helpOrVersionAndMayExit()` rather than `preCommands()`, keeping the process
registration separate.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/ProcessRuns.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.io.Reader;
+import java.io.Writer;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.time.Instant;
+import java.util.Comparator;
+import java.util.Properties;
+
+public final class ProcessRuns {
+
+ private static final String RUNS_SUBDIR = ".m2/.maven/runs";
+ private static final String FILE_PREFIX = "mvn-";
+ private static final String FILE_SUFFIX = ".properties";
+
+ private static final String KEY_PID = "pid";
+ private static final String KEY_VERSION = "version";
+ private static final String KEY_WORKDIR = "workDir";
+ private static final String KEY_EXECROOT = "execRoot";
+ private static final String KEY_STARTED = "started";
+
+ private static Path runsDir() {
+ final String home = System.getProperty("user.home", ".");
+ return Paths.get(home).resolve(RUNS_SUBDIR);
+ }
+
+ private static Path desc(final long pid) {
+ return runsDir().resolve(FILE_PREFIX + pid + FILE_SUFFIX);
+ }
+
+ public static void install(final long pid, final String version, final
Path workDir, final Path execRoot) {
Review Comment:
The runs directory is derived from `System.getProperty("user.home")` which
couples this class to global JVM state and makes testing harder (the test has
to mutate `user.home`). Consider accepting the base directory as a parameter to
`install`/`uninstall`/`listAlive`, or at minimum a package-private overload for
testability.
--
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]
