Copilot commented on code in PR #2431:
URL: https://github.com/apache/groovy/pull/2431#discussion_r3036107372
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -282,6 +286,102 @@ 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);
+ boolean interrupted = false;
+ try {
+ try { tout.join(); } catch (InterruptedException ignore) {
interrupted = true; }
+ try { terr.join(); } catch (InterruptedException ignore) {
interrupted = true; }
+ try { self.waitFor(); } catch (InterruptedException ignore) {
interrupted = true; }
+ closeStreams(self);
+ } finally {
+ if (interrupted) Thread.currentThread().interrupt();
+ }
+ return new ProcessResult(sout.toString(), serr.toString(),
self.exitValue());
+ }
+
+ /**
+ * 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 interrupted = false;
+ try {
+ try {
+ boolean finished = self.waitFor(timeout, unit);
+ if (!finished) {
+ self.destroyForcibly();
+ self.waitFor();
+ }
+ } catch (InterruptedException ignore) {
+ interrupted = true;
+ self.destroyForcibly();
+ }
+ try { tout.join(); } catch (InterruptedException ignore) {
interrupted = true; }
+ try { terr.join(); } catch (InterruptedException ignore) {
interrupted = true; }
+ closeStreams(self);
+ } finally {
+ if (interrupted) Thread.currentThread().interrupt();
+ }
+ return new ProcessResult(sout.toString(), serr.toString(),
self.exitValue());
+ }
Review Comment:
In `waitForResult(Process,long,TimeUnit)`, the `InterruptedException` catch
destroys the process but doesn't `waitFor()` it to actually terminate before
`exitValue()` is read. This can lead to `IllegalThreadStateException` and/or
background process leakage. After `destroyForcibly()`, ensure termination
(e.g., `self.waitFor()` in a `finally`/best-effort block) or rethrow the
interrupt so callers can handle it.
##########
src/test/groovy/groovy/execute/ExecuteTest.groovy:
##########
@@ -167,4 +178,225 @@ final class ExecuteTest {
assert out.toString().contains('hello')
assert process.exitValue() == 0
}
+
+ // --- toProcessBuilder ---
+
+ @Test
+ void testToProcessBuilderFromString() {
+ def pb = cmd.toProcessBuilder()
+ assert pb instanceof ProcessBuilder
+ def process = pb.start()
+ process.waitForProcessOutput()
+ assert process.exitValue() == 0
+ }
+
+ @Test
+ void testToProcessBuilderFromArray() {
+ def pb = cmd.split(' ').toProcessBuilder()
+ assert pb instanceof ProcessBuilder
+ def process = pb.start()
+ process.waitForProcessOutput()
+ assert process.exitValue() == 0
+ }
+
+ @Test
+ void testToProcessBuilderFromList() {
+ def pb = Arrays.asList(cmd.split(' ')).toProcessBuilder()
+ assert pb instanceof ProcessBuilder
+ def process = pb.start()
+ process.waitForProcessOutput()
+ assert process.exitValue() == 0
+ }
+
+ // --- waitForResult ---
+
+ @Test
+ void testWaitForResultFromString() {
+ ProcessResult result = "$echoCmd hello".execute().waitForResult()
+ assert result.ok
+ assert result.exitCode == 0
+ assert result.out.trim().contains('hello')
+ assert result.err != null
Review Comment:
`assert result.err != null` is always true because `waitForResult()` builds
`err` from a `StringBuilder` (never null). Consider asserting something
meaningful (e.g., `result.err.isEmpty()` for success cases) or removing the
assertion to avoid a false sense of coverage.
```suggestion
assert result.err.isEmpty()
```
##########
src/main/java/org/codehaus/groovy/runtime/ProcessGroovyMethods.java:
##########
@@ -754,4 +1024,60 @@ private static String[] stringify(final List orig) {
return result;
}
+ private static File toFile(Object obj) {
+ if (obj instanceof File) return (File) obj;
+ if (obj instanceof Path) return ((Path) obj).toFile();
+ return new File(obj.toString());
+ }
+
+ private static ProcessBuilder configureProcessBuilder(ProcessBuilder pb,
Map<String, Object> options) {
+ if (options == null || options.isEmpty()) return pb;
+
+ Object dir = options.get("dir");
+ if (dir != null) {
+ pb.directory(toFile(dir));
+ }
+
+ Object env = options.get("env");
+ if (env instanceof Map) {
+ Map<String, String> pbEnv = pb.environment();
+ ((Map<?, ?>) env).forEach((k, v) -> pbEnv.put(String.valueOf(k),
String.valueOf(v)));
+ }
+
+ if (Boolean.TRUE.equals(options.get("redirectErrorStream"))) {
+ pb.redirectErrorStream(true);
+ }
+
+ if (Boolean.TRUE.equals(options.get("inheritIO"))) {
+ pb.inheritIO();
+ }
+
+ Object outputFile = options.get("outputFile");
+ if (outputFile != null) {
+ pb.redirectOutput(toFile(outputFile));
+ }
+
+ Object appendOutput = options.get("appendOutput");
+ if (appendOutput != null) {
+
pb.redirectOutput(ProcessBuilder.Redirect.appendTo(toFile(appendOutput)));
+ }
+
+ Object errorFile = options.get("errorFile");
+ if (errorFile != null) {
+ pb.redirectError(toFile(errorFile));
+ }
+
+ Object appendError = options.get("appendError");
+ if (appendError != null) {
+
pb.redirectError(ProcessBuilder.Redirect.appendTo(toFile(appendError)));
+ }
Review Comment:
`configureProcessBuilder` adds support for `inheritIO`, `errorFile`,
`appendOutput`, and `appendError`, but the new/updated tests only cover `dir`,
`env`, `redirectErrorStream`, `outputFile`, and `inputFile`. Adding tests for
the remaining options would help prevent regressions (especially around append
vs overwrite and stderr redirection).
##########
src/spec/test/gdk/WorkingWithIOSpecTest.groovy:
##########
@@ -310,23 +313,23 @@ Fin.''')
void testProcess1() {
if (unixlike) {
// tag::process_list_files[]
- def process = "ls -l".execute() // <1>
- println "Found text ${process.text}" // <2>
+ def process = "ls -l".execute() // <1>
+ println "Files: $process.text" // <2>
Review Comment:
`"Files: $process.text"` doesn't interpolate `process.text` in a GString; it
interpolates `process` and then appends the literal `.text`. Use
`${process.text}` (or store `process.text` in a variable) so the example prints
the command output as intended.
##########
src/spec/test/gdk/WorkingWithIOSpecTest.groovy:
##########
@@ -310,23 +313,23 @@ Fin.''')
void testProcess1() {
if (unixlike) {
// tag::process_list_files[]
- def process = "ls -l".execute() // <1>
- println "Found text ${process.text}" // <2>
+ def process = "ls -l".execute() // <1>
+ println "Files: $process.text" // <2>
// end::process_list_files[]
assert process instanceof Process
}
if (windoz) {
try {
// tag::dir_windows[]
def process = "dir".execute()
- println "${process.text}"
+ println "Files: $process.text"
Review Comment:
`"Files: $process.text"` won't access the `text` property; it produces
something like `Files: <process>.text`. Use `${process.text}` so the Windows
example matches the surrounding documentation intent.
##########
src/spec/test/gdk/WorkingWithIOSpecTest.groovy:
##########
@@ -310,23 +313,23 @@ Fin.''')
void testProcess1() {
if (unixlike) {
// tag::process_list_files[]
- def process = "ls -l".execute() // <1>
- println "Found text ${process.text}" // <2>
+ def process = "ls -l".execute() // <1>
+ println "Files: $process.text" // <2>
// end::process_list_files[]
assert process instanceof Process
}
if (windoz) {
try {
// tag::dir_windows[]
def process = "dir".execute()
- println "${process.text}"
+ println "Files: $process.text"
// end::dir_windows[]
// we do not check that the expected exception is really
thrown,
// because the command succeeds if PATH contains cygwin
- } catch (e) {
+ } catch (ignore) {
// tag::dir_windows_fixed[]
def process = "cmd /c dir".execute()
- println "${process.text}"
+ println "Files: $process.text"
Review Comment:
Same GString interpolation issue as above: `"Files: $process.text"` does not
evaluate `process.text`. Use `${process.text}` so the output shown in the docs
is correct.
--
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]
