Copilot commented on code in PR #2431:
URL: https://github.com/apache/groovy/pull/2431#discussion_r3034778268
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -282,6 +285,87 @@ public static void waitForProcessOutput(Process self,
OutputStream output, Outpu
}
}
+ /**
+ * Executes the process and waits for it to complete, capturing
+ * the standard output, standard error, and exit code into a
+ * {@link ProcessResult}.
+ *
+ * <pre>
+ * def result = "ls -la".execute().waitForResult()
+ * if (result.ok) println result.out
+ * else System.err.println result.err
+ * </pre>
+ *
+ * @param self a Process
+ * @return a ProcessResult containing stdout, stderr, and exit code
+ * @throws InterruptedException if the current thread is interrupted.
+ * @since 6.0.0
+ */
+ public static ProcessResult waitForResult(Process self) throws
InterruptedException {
+ StringBuilder sout = new StringBuilder();
+ StringBuilder serr = new StringBuilder();
+ Thread tout = consumeProcessOutputStream(self, sout);
+ Thread terr = consumeProcessErrorStream(self, serr);
+ tout.join();
+ terr.join();
+ int exitCode = self.waitFor();
+ closeStreams(self);
+ return new ProcessResult(sout.toString(), serr.toString(), exitCode);
+ }
+
+ /**
+ * Executes the process and waits for it to complete within the given
timeout,
+ * capturing the standard output, standard error, and exit code into a
+ * {@link ProcessResult}. If the process does not complete within the
timeout,
+ * it is forcibly destroyed.
+ *
+ * <pre>
+ * def result = "cmd".execute().waitForResult(30, TimeUnit.SECONDS)
+ * </pre>
+ *
+ * @param self a Process
+ * @param timeout the maximum time to wait
+ * @param unit the time unit of the timeout argument
+ * @return a ProcessResult containing stdout, stderr, and exit code
+ * @throws InterruptedException if the current thread is interrupted.
+ * @since 6.0.0
+ */
+ public static ProcessResult waitForResult(Process self, long timeout,
TimeUnit unit) throws InterruptedException {
+ StringBuilder sout = new StringBuilder();
+ StringBuilder serr = new StringBuilder();
+ Thread tout = consumeProcessOutputStream(self, sout);
+ Thread terr = consumeProcessErrorStream(self, serr);
+ boolean finished = self.waitFor(timeout, unit);
+ if (!finished) {
+ self.destroyForcibly();
+ self.waitFor();
+ }
+ tout.join();
+ terr.join();
+ closeStreams(self);
+ return new ProcessResult(sout.toString(), serr.toString(),
self.exitValue());
Review Comment:
`waitForResult(Process,long,TimeUnit)` has the same interruption/cleanup
concern as the non-timeout variant: on `InterruptedException`, streams may
remain open and the interrupt status is cleared. Aligning this method with
`waitForProcessOutput`'s try/finally pattern (always closing streams and
restoring the interrupt flag) would make it safer, especially when the process
is force-destroyed on timeout.
```suggestion
boolean interrupted = false;
try {
boolean finished = self.waitFor(timeout, unit);
if (!finished) {
self.destroyForcibly();
self.waitFor();
}
tout.join();
terr.join();
return new ProcessResult(sout.toString(), serr.toString(),
self.exitValue());
} catch (InterruptedException e) {
interrupted = true;
throw e;
} finally {
closeStreams(self);
if (interrupted) {
Thread.currentThread().interrupt();
}
}
```
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -282,6 +285,87 @@ public static void waitForProcessOutput(Process self,
OutputStream output, Outpu
}
}
+ /**
+ * Executes the process and waits for it to complete, capturing
+ * the standard output, standard error, and exit code into a
+ * {@link ProcessResult}.
+ *
+ * <pre>
+ * def result = "ls -la".execute().waitForResult()
+ * if (result.ok) println result.out
+ * else System.err.println result.err
+ * </pre>
+ *
+ * @param self a Process
+ * @return a ProcessResult containing stdout, stderr, and exit code
+ * @throws InterruptedException if the current thread is interrupted.
+ * @since 6.0.0
+ */
+ public static ProcessResult waitForResult(Process self) throws
InterruptedException {
+ StringBuilder sout = new StringBuilder();
+ StringBuilder serr = new StringBuilder();
+ Thread tout = consumeProcessOutputStream(self, sout);
+ Thread terr = consumeProcessErrorStream(self, serr);
+ tout.join();
+ terr.join();
+ int exitCode = self.waitFor();
+ closeStreams(self);
+ return new ProcessResult(sout.toString(), serr.toString(), exitCode);
+ }
Review Comment:
`waitForResult(Process)` doesn't follow the interrupt/cleanup pattern used
by `waitForProcessOutput`: if `join()`/`waitFor()` throws
`InterruptedException`, the process streams won't be closed and the thread's
interrupt status will be cleared. Consider wrapping the joins/wait in a
try/finally that always calls `closeStreams(self)` and restores the interrupt
flag before rethrowing (or uses an `interrupted` boolean like
`waitForProcessOutput`).
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -433,6 +517,42 @@ public static Process or(final Process left, final Process
right) throws IOExcep
return pipeTo(left, right);
}
+ /**
+ * Creates a native OS pipeline from a list of commands, using
+ * {@link ProcessBuilder#startPipeline(List)}. Each element in the list
+ * can be a String (tokenized on whitespace), a List of Strings (used
as-is),
+ * a String array, or a {@link ProcessBuilder} for full control.
Review Comment:
Javadoc for `pipeline(List)` says list commands are "used as-is", but
`toProcessBuilder(List)` calls `toString()` on each element (via `stringify`).
Either adjust the Javadoc to reflect the `toString()` conversion or change the
implementation to require/retain `List<String>` without coercion.
```suggestion
* can be a String (tokenized on whitespace), a List whose elements are
* converted to Strings using {@code toString()}, a String array, or a
* {@link ProcessBuilder} for full control.
```
##########
src/spec/doc/_working-with-io.adoc:
##########
@@ -204,8 +204,48 @@
include::../test/gdk/WorkingWithIOSpecTest.groovy[tags=object_in_out,indent=0]
The previous section described how easy it was to deal with files, readers or
streams in Groovy. However in domains
like system administration or devops it is often required to communicate with
external processes.
-Groovy provides a simple way to execute command line processes. Simply
-write the command line as a string and call the `execute()` method.
+Groovy provides two flavors for executing external processes. The *simple*
style calls `execute()`
+directly on a command string:
+
+[source,groovy]
+----
+"javac src/main/java/*.java".execute()
+----
+
+For more sophisticated workloads, the *named parameter* style configures the
process with options like working directory,
+environment, and stream redirection:
+
+[source,groovy]
+----
+def result = "javac src/main/java/*.java".execute(
+ dir: new File('/myproject'),
+ redirectErrorStream: true
+).waitForResult(2, TimeUnit.MINUTES)
+
Review Comment:
The inline examples use `TimeUnit.MINUTES` but the snippet doesn't import
`java.util.concurrent.TimeUnit` (or qualify it). To keep the documentation
example self-contained (and to avoid failures if snippets are
compiled/validated), add the import or use
`java.util.concurrent.TimeUnit.MINUTES`.
##########
src/spec/doc/_working-with-io.adoc:
##########
@@ -204,8 +204,48 @@
include::../test/gdk/WorkingWithIOSpecTest.groovy[tags=object_in_out,indent=0]
The previous section described how easy it was to deal with files, readers or
streams in Groovy. However in domains
like system administration or devops it is often required to communicate with
external processes.
-Groovy provides a simple way to execute command line processes. Simply
-write the command line as a string and call the `execute()` method.
+Groovy provides two flavors for executing external processes. The *simple*
style calls `execute()`
+directly on a command string:
+
+[source,groovy]
+----
+"javac src/main/java/*.java".execute()
+----
+
+For more sophisticated workloads, the *named parameter* style configures the
process with options like working directory,
+environment, and stream redirection:
+
+[source,groovy]
+----
+def result = "javac src/main/java/*.java".execute(
+ dir: new File('/myproject'),
+ redirectErrorStream: true
+).waitForResult(2, TimeUnit.MINUTES)
+
+if (!result.ok) System.err.println result.out
+----
+
+The *ProcessBuilder* style uses `toProcessBuilder()` and the fluent
`ProcessBuilder` API
+for full control:
+
+[source,groovy]
+----
+def result = "javac src/main/java/*.java".toProcessBuilder()
+ .directory(new File('/myproject'))
+ .redirectErrorStream(true)
+ .start()
+ .waitForResult(2, TimeUnit.MINUTES)
+
Review Comment:
Same issue in the `toProcessBuilder()` example: it references
`TimeUnit.MINUTES` without importing/qualifying `TimeUnit`, so the snippet
isn't self-contained.
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -735,6 +898,96 @@ public static Process execute(final List commands, final
List envp, final File d
return Runtime.getRuntime().exec(stringify(commands), stringify(envp),
dir);
}
+ /**
+ * Executes the command specified by <code>self</code> with options
provided
+ * as named parameters. Supported options:
+ * <ul>
+ * <li><b>dir</b>: (File or String) the working directory</li>
+ * <li><b>env</b>: (Map) environment variables to add (merged with
inherited environment)</li>
+ * <li><b>redirectErrorStream</b>: (boolean) merge stderr into
stdout</li>
+ * <li><b>inheritIO</b>: (boolean) inherit the parent process I/O
streams</li>
+ * <li><b>outputFile</b>: (File) redirect stdout to a file</li>
+ * <li><b>errorFile</b>: (File) redirect stderr to a file</li>
+ * <li><b>inputFile</b>: (File) redirect stdin from a file</li>
+ * <li><b>appendOutput</b>: (File) append stdout to a file</li>
+ * <li><b>appendError</b>: (File) append stderr to a file</li>
+ * </ul>
+ *
+ * <pre>
+ * "make test".execute(dir: new File("/project"), env: [CI: "true"])
+ * "cmd".execute(redirectErrorStream: true)
+ * </pre>
+ *
+ * @param self a command line String
+ * @param options a Map of options to configure the process
+ * @return the Process which has just started
+ * @throws IOException if an IOException occurs.
+ * @since 6.0.0
+ */
+ public static Process execute(final String self, @NamedParams({
+ @NamedParam(value = "dir", type = File.class),
+ @NamedParam(value = "env", type = Map.class),
+ @NamedParam(value = "redirectErrorStream", type = Boolean.class),
+ @NamedParam(value = "inheritIO", type = Boolean.class),
+ @NamedParam(value = "outputFile", type = File.class),
+ @NamedParam(value = "errorFile", type = File.class),
+ @NamedParam(value = "inputFile", type = File.class),
+ @NamedParam(value = "appendOutput", type = File.class),
+ @NamedParam(value = "appendError", type = File.class)
+ }) Map<String, Object> options) throws IOException {
+ return configureProcessBuilder(toProcessBuilder(self),
options).start();
+ }
Review Comment:
The named-parameter contract for `dir` is documented as "File or String",
and the implementation accepts any non-null value via `toString()`, but the
`@NamedParam` type is `File.class`. This makes IDE/type-checking hints
misleading; consider widening the annotation type (e.g.,
`Object`/`CharSequence`) or documenting that only `File` is supported.
##########
src/spec/doc/_working-with-io.adoc:
##########
@@ -204,8 +204,48 @@
include::../test/gdk/WorkingWithIOSpecTest.groovy[tags=object_in_out,indent=0]
The previous section described how easy it was to deal with files, readers or
streams in Groovy. However in domains
like system administration or devops it is often required to communicate with
external processes.
-Groovy provides a simple way to execute command line processes. Simply
-write the command line as a string and call the `execute()` method.
+Groovy provides two flavors for executing external processes. The *simple*
style calls `execute()`
Review Comment:
This section says Groovy provides "two flavors" for executing external
processes, but it then introduces three (*simple*, *named parameter*, and
*ProcessBuilder*) styles. Please reconcile the wording (e.g., "three styles" or
merge descriptions) to avoid confusing readers.
```suggestion
Groovy provides several styles for executing external processes. The
*simple* style calls `execute()`
```
--
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]
