Repository navigation
fix(docker): give the builder command a tty in a terminal - #453
Conversation
|
Thanks @JorickPepin! The colors are back indeed, but giving
The root cause predates this PR: #426 moved the policy of the
Patch on top of your branch ( diff --git a/.castor/docker.php b/.castor/docker.php
index e0106bc..f9acfb7 100644
--- a/.castor/docker.php
+++ b/.castor/docker.php
@@ -255,6 +255,12 @@ function builder(#[AsArgsAfterOptionEnd] array $params = []): int
{
$context = context()->withEnvironment($_ENV + $_SERVER);
+ // Behave like the command run by hand: a TTY in a terminal (colors,
+ // prompts), the forwarded stdin otherwise (CI, pipes, redirections)
+ $context = $context->supportsInteraction()
+ ? $context->toInteractive()
+ : $context->withInput(STDIN)->withAllowFailure();
+
return (int) docker_compose_run($params, $context)->getExitCode();
}
@@ -520,16 +526,6 @@ function docker_compose(array $subCommand, ?Context $c = null, array $profiles =
return run($command, context: $c, callback: $callback);
}
-// Keep the TTY in a terminal so tools keep their colors, drop it when piped (CI, `castor postgres < dump.sql`)
-function command_context(Context $c): Context
-{
- if (!$c->quiet && stream_isatty(STDIN) && stream_isatty(STDOUT)) {
- return $c->withTty()->withAllowFailure();
- }
-
- return $c->withTty(false)->withPty(false)->withInput(STDIN)->withAllowFailure();
-}
-
/**
* @param list<string> $params
*/
@@ -572,7 +568,6 @@ function docker_compose_run(
$params = ['bash'];
$c = $c->toInteractive();
} else {
- $c = command_context($c);
$params = array_map(escapeshellarg(...), $params);
}
@@ -602,7 +597,6 @@ function docker_compose_exec(
$params = ['bash'];
$context = $context->toInteractive();
} else {
- $context = command_context($context);
$params = array_map(escapeshellarg(...), $params);
}
diff --git a/.castor/postgres.php b/.castor/postgres.php
index 60fc2e6..528a1bb 100644
--- a/.castor/postgres.php
+++ b/.castor/postgres.php
@@ -40,5 +40,5 @@ function client(#[AsArgsAfterOptionEnd] array $params = []): int
$params = ['-c', implode(' ', $params)];
}
- return (int) docker_compose_exec([...$command, ...$params], service: 'postgres')->getExitCode();
+ return (int) docker_compose_exec([...$command, ...$params], context()->withInput(STDIN)->withAllowFailure(), service: 'postgres')->getExitCode();
}
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index c2eb379..de5cdf1 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -122,8 +122,14 @@ jobs:
set -e
set -o pipefail
- # A terminal gets a TTY (for colors), a pipe does not
- script -qec "castor builder -- php -r 'exit(stream_isatty(STDOUT) ? 0 : 1);'" /dev/null
+ # In a terminal (emulated by script), the command gets a TTY (for colors).
+ # CI is unset, as Castor never considers it as interactive
+ env -u CI script -qec "castor builder -- php -r 'exit(stream_isatty(STDOUT) ? 0 : 1);'" /dev/null
+ # Even in a terminal, a pipe on stdout or stdin reaches the container
+ env -u CI script -qec "castor builder -- php -r 'echo stream_isatty(STDOUT) ? \"tty\" : \"pipe\";' | cat" /dev/null | grep -w pipe
+ env -u CI script -qec "echo hello | castor builder -- cat" /dev/null | grep -w hello
+
+ # Without a terminal
castor builder -- php -r 'exit(stream_isatty(STDOUT) ? 1 : 0);' | cat
echo hello | castor builder -- cat | grep -x helloChecked locally in a pseudo-terminal:
Two limits remain for |
2d8092a to
ba40973
Compare
|
Thanks a lot @lyrixx for the testing and the patch! I applied it as is, squashed everything into a single commit and updated the PR description. |
…ss with the standard streams of the current process (lyrixx) This PR was merged into the 8.2 branch. Discussion ---------- [Process] Add `Process::inheritStdio()` to run a process with the standard streams of the current process | Q | A | ------------- | --- | Branch? | 8.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #44821 | License | MIT `Process` can read the output through pipes, run the command in a PTY, or connect it to `/dev/tty` (TTY mode). None of these does what a shell script does: let the command use the standard input, output and error of the current process. The TTY mode is the closest, but it opens `/dev/tty` instead of using the current streams, so: - `php script.php 2> err.log` is ignored: the command keeps writing its errors to the terminal; - `php script.php > out.log` makes `setTty(true)` throw, as `STDOUT` is not a TTY; - `echo foo | php script.php` does not reach the command, which reads the terminal instead; - it fails without a controlling terminal (`setsid`, some CI runners, a PHP script started by another tool in a PTY): `proc_open(/dev/tty): Failed to open stream: No such device or address`. This PR adds `Process::inheritStdio()`: the child inherits the standard input, output and error of the current process: ```php $process = new Process(['vendor/bin/phpunit']); $process->inheritStdio(); $process->run(); // colors in a terminal, plain output when redirected, exit code available $process = new Process(['psql', '-v', 'ON_ERROR_STOP=1']); $process->inheritStdio(); $process->setInput($dump); // stdin is fed by the input, stdout and stderr are inherited $process->run(); ``` Behavior: - the output is not captured: `getOutput()` returns an empty string and callbacks are not called, like in TTY mode. `getIterator()` and `waitUntil()` throw a `LogicException`, as there is no output to read; - when an input is set, it is written to the standard input instead of inheriting it; - it cannot be combined with the TTY mode or an idle timeout (`LogicException`, like `disableOutput()` and the idle timeout); it takes precedence over the PTY mode, like the TTY mode does; - it cannot be called while the process is running (`RuntimeException`, like `disableOutput()`); - the descriptors are fresh `php://stdin`, `php://stdout` and `php://stderr` streams, not the `STDIN`/`STDOUT`/`STDERR` constants. `proc_open()` seeks the constants back to the offset PHP last saw for them, so a child would overwrite output redirected to a file, or read an input file again. This also works outside the CLI SAPI, where the constants don't exist; - on Windows, the temporary files used to read the output are not created, and the streams are passed to `proc_open()` as well (I could not test it on Windows: let's see what the CI says); - no signal is handled: the child runs in the process group of the current process, so Ctrl+C reaches both. A caller that wants to survive it can install a no-op handler with `pcntl_signal()`: handlers are reset in the child on exec, so the child is still interrupted and `wait()` throws a `ProcessSignaledException`. `SIG_IGN` must not be used for that, as an ignored signal stays ignored in the child. Motivation: in [Castor](https://github.com/jolicode/castor), commands proxied to a container (`docker compose run`) need to behave like they were typed in the shell: TTY detection by the command itself, redirections, pipes, exit code. Doing it with the TTY/PTY/pipes modes ends up in heuristics that each break another case (see jolicode/docker-starter#426, jolicode/docker-starter#453 and jolicode/castor#907). #40037 reached the same conclusion: passing the current streams to `proc_open()` is what makes it work. cc `@joelwurtz` Commits ------- b5b8398 [Process] Add `Process::inheritStdio()` to run a process with the standard streams of the current process
Since docker_compose_run() no longer forces a run without TTY (jolicode/docker-starter#453), "castor qa:security-audit" asks whether to trust this plugin (required by open-telemetry/sdk), and hangs when Castor has a PTY but no terminal. It is declared as disabled: Composer already skipped it, and the application builds its OpenTelemetry meter provider itself.
Since #453, docker_compose_run() and docker_compose_exec() keep the PTY of the context. Without a terminal (CI, pipes, AI agents...), the command gets a terminal nobody types in, and a prompt waits forever (e.g. composer audit asking to trust a plugin). The default context now gives a PTY only when stdin is a terminal, outside of a CI. A CTRL+C followed by a CTRL+D in the shell of `castor builder` no longer stops the task as interrupted: the shell exits with the code of its last command, 130, which Castor 1.9 takes for a CTRL+C. The terminal tests move from the workflow to `castor test:terminal`, and the tests of docker-starter to the `test` namespace. Castor 1.9 is required, for its detection of non-interactive environments.
Changes
docker_compose_run()anddocker_compose_exec()no longer change the context: internal tasks (install, qa, start…) get Castor's default context back, as before Refactor how we use run() docker command #426 (colors, Ctrl-C, output capture, a failing step stops the task)castor builder -- …gets a TTY in a terminal, and the forwarded stdin otherwise (CI, pipes, redirections), so the fix from Really fix the builder command when no TTY is available #407 still holdscastor postgres -- …stays non-interactive, with stdin forwardedcastor builder, with and without a terminalHow to test