fix(setup): make setup.sh work when it is not run interactively - #1374
Open
dholt wants to merge 2 commits into
Open
fix(setup): make setup.sh work when it is not run interactively#1374dholt wants to merge 2 commits into
dholt wants to merge 2 commits into
Conversation
`./scripts/setup.sh` silently does nothing and exits 0 for every
non-interactive caller — CI outside this repo, `ssh host './scripts/setup.sh'`,
pipelines, and AI agents.
The cause is the shebang. `--init-file FILE` consumes the script path as its
option argument, so bash is left with no script operand. An interactive shell
sources it as an init file (the intended trick, which drops you into a shell
with the venv active). A non-interactive shell ignores `--init-file`, has no
operand, reads EOF from stdin, and exits 0 having done nothing.
$ ./scripts/setup.sh </dev/null ; echo "exit=$?"
exit=0 # no packages, no venv, no output at all
$ bash scripts/setup.sh # works — operand form bypasses the shebang
That contrast is also why CI has never caught this: .github/workflows/setup.yml
runs the operand form.
Removing the trick costs little. The script already appends
`source ${VENV_DIR}/bin/activate` to ~/.bashrc and prints the activate command
on its last line, so the venv is still one command away and automatic in the
next shell. The `--init-file` shell was in any case a degraded one — it
*replaces* ~/.bashrc, so it had none of the user's aliases, functions or prompt.
Two adjacent bugs in the same file:
* The root check did `echo` then a bare `exit`, which inherits echo's status —
so refusing to run as root also reported success. Now exits 1, on stderr.
* The unsupported-OS branch printed `${DEPS_RPM[@]}`, an array that has never
been defined anywhere in the tree (`git log -S DEPS_RPM` finds only the commit
that introduced the reference). It now names the real arrays and prints both
the RPM and DEB lists, since the previous text offered EL package names to
everyone including Debian users.
That branch stays non-fatal deliberately. It is an escape hatch: everything
after the `case` is distro-agnostic, and Rocky (`ID=rocky`) and AlmaLinux
(`ID=almalinux`) fall through it today because the case only matches
`rhel*|centos*|ubuntu*` — while README.md claims Rocky 8/9 support. Making it
exit would turn a working path into a hard failure for a supported distro. The
existing `virtualenv` check below still fails loudly when a dependency is
genuinely missing.
Behaviour changes worth calling out for anyone with existing automation:
running as root now fails loudly instead of quietly succeeding, and a
non-interactive `./scripts/setup.sh` now actually does the setup.
Signed-off-by: Doug Holt <dholt@nvidia.com>
Contributor
Author
|
Regression coverage is now part of this PR: the executable path runs on both |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The usual no-argument
./scripts/setup.shinvocation silently does nothing andexits 0 when run non-interactively, including from CI outside this repository,
remote commands, pipelines, and agents.
Why
The old shebang was
#!/bin/bash --init-file.--init-file FILEconsumes thescript path as its option argument, leaving Bash with no script operand.
a shell with the virtual environment activated.
--init-fileis ignored. Bash reads EOF from stdin andexits successfully without running the setup.
The old workflow invoked
bash scripts/setup.sh, which supplied a scriptoperand and bypassed the shebang, so it did not cover the failing path.
Why removing the trick is safe
The interactive convenience is redundant:
~/.bashrc, so theenvironment activates automatically in later shells.
--init-filereplaces~/.bashrc, so the subshell omitted the user's normalaliases, functions, and prompt.
Running
./scripts/setup.shfrom a terminal will no longer leave an activatedsubshell; use the command it prints or open a new shell.
Adjacent fixes
exit, whichinherited
echo's successful status. It now exits 1 and writes to stderr.${DEPS_RPM[@]}array. It now prints the actual RPM and Debian dependency arrays.
The unsupported-OS branch remains non-fatal because later setup is
distribution-independent and the existing
virtualenvcheck still failsloudly when a required dependency is genuinely absent.
Regression coverage
The setup workflow now runs:
./scripts/setup.sh </dev/nullThat exercises the executable/shebang path on both Ubuntu 22.04 and Ubuntu
24.04 instead of bypassing it with
bash scripts/setup.sh.Validation on this head includes:
Follow-up
Add Rocky Linux to the dependency-install case so the distribution already
supported elsewhere in DeepOps receives automatic setup dependencies instead
of the manual fallback message. AlmaLinux and Oracle Linux can be evaluated
separately as Red Hat-compatible candidates rather than being implied as
currently supported.