Source the get_* install scripts from one shared shell library by egpbos · Pull Request #864 · FormingWorlds/PROTEUS

egpbos · 2026-09-09T12:30:36Z

Description

The tools/get_*.sh install scripts each carried their own copy of the same
shell logic. Measured on main: portable_realpath in nine scripts, the
dirty-checkout guard in seven, the --force argument split in six, the GitHub
SSH probe in four, and the repo-root derivation in nearly all of them. A fix to
one copy left the rest untouched, which is how a portable_realpath that
rejects a path git clone has yet to create came to need the same four-line
fix in nine places (#862).

tools/_get_common.sh now holds one copy of each helper:

helper	replaces
`portable_realpath`	9 private copies
`proteus_root`, `proteus_tools_dir`	the repo-root derivation in 11 scripts, in two different spellings
`get_parse_args`	the `--force` loop in 6 scripts and the flag-plus-path split in 2
`guard_dirty_checkout`	the dirty-checkout guard in 7 scripts
`github_use_ssh`	the `ssh -T git@github.com` probe in 4 scripts, in 3 different spellings
`github_ssh_url`	the https-to-SSH rewrite in 4 scripts
`resolve_module_pin`	the pin lookup and its emptiness check in 3 scripts

Every script sources it through its own directory:

_get_common="$(dirname "${BASH_SOURCE[0]}")/_get_common.sh"
if [ ! -f "$_get_common" ]; then
    echo "ERROR: $_get_common is missing; use a complete PROTEUS checkout." >&2
    exit 1
fi
source "$_get_common"

Every caller runs these scripts by path (the installation and cluster docs,
install.sh, the setup-proteus composite action, ci-pr-checks.yml, and
proteus install-all through src/proteus/utils/data.py), so
${BASH_SOURCE[0]} locates the library without needing a resolved path first.
A script whose library is absent stops with the missing file named rather than
continuing with undefined helpers.

Variations stay variations, as parameters rather than separate copies:

guard_dirty_checkout takes a git pathspec, so get_socrates.sh keeps
excluding its regenerable make/Mk_cmd from the dirty test.
get_parse_args reports both the --force switch and the optional install
path, in either order, which covers the six flag-only scripts and the two
that also accept a destination.
The SSH probe honours GIT_SSH_COMMAND for every script, not only for
SOCRATES; CI already relies on that for the SOCRATES build and now gets it
everywhere.

The helpers target bash 3.2 (the version macOS ships) and behave the same with
or without set -euo pipefail, because the scripts differ on that and
get_socrates.sh enables it partway through. The two helpers that stop the
install (guard_dirty_checkout, resolve_module_pin) are documented as
plain-command calls, since an exit inside a command substitution would only
leave the subshell.

Three behaviour changes come with the extraction, all intended:

The shared portable_realpath resolves a destination that does not exist
yet, and get_socrates.sh stops when its install path cannot be resolved.
This is the fix from Resolve a get_* install path that does not exist yet #862, applied to the single copy: on main,
bash tools/get_socrates.sh /some/new/path reaches git clone with an empty
work-tree name. That PR can be closed in favour of this one; if it merges
first, the conflict resolves by dropping the nine copies it touched.
get_agni.sh and get_spider.sh derived the checkout root with
cd ... && pwd, which keeps a symlinked path, while the other nine used
realpath, which resolves it. All of them now resolve it, so a checkout
reached through a symlink lands in the same root everywhere instead of
installing AGNI under the logical path and aragog under the physical one.
The checkout guard read both of its probes through git ... | head -1,
whose exit status is git's under set -o pipefail and head's without it.
A checkout with an unborn HEAD or an incomplete .git makes
git log HEAD exit 128, so the same state stopped get_boreas.sh and
get_vulcan.sh with no message at all and was silently deleted by the
other six, staged work included. Both probes now run unpiped, bounded by
git's own -1, and a probe that cannot run is reported with the checkout
kept and --force named. The verdict is the same in either shell, which
is what the library documents.

Two smaller changes in the scripts the extraction rewrites: get_aragog.sh
and get_zalmoxis.sh quote the work path they rm -rf and the root they
cd back to (both were unquoted, and the root is now a resolved path that a
directory name with a space would have split), and get_lovepy.sh stops on
a missing LovePy pin rather than passing an empty url=/rev= to
Pkg.add.

Also in this PR: the SOCRATES cache key in the composite action hashed only
tools/get_socrates.sh, so a change to the shared helpers would have restored
a tree built by the previous version with nothing reporting the mismatch. The
key now hashes the library too.

docs/How-to/development_standards.md gains a short "Module install scripts"
section: what the library provides, the bootstrap a new script starts from, and
the rule that a variation becomes a parameter rather than a second copy. That
replaces the "keep this guard in sync across the get_* scripts" comments the
duplicated blocks carried.

Net across the tools/get_*.sh scripts: 374 lines removed, 170 added. The
library that replaces them is 169 lines, about half of it comment. The rest of
the diff is the tests (864 added, 89 removed) and the docs section.

Validation of changes

Test configuration: macOS 26.6.2 (arm64), bash 3.2.57, which is the only
bash on PATH here, so the bash 3.2 target is what actually ran, and Python
3.12 in the project environment.

Behaviour, before against after. Each tools/get_*.sh ran whole in a
throwaway checkout with git and ssh stubbed on PATH, once with the SSH
probe accepting the key and once refusing it, recording every git call. The
same harness ran against main's scripts. Every clone destination and
transport is identical, with the two destination exceptions above:

get_socrates.sh <path that does not exist>: main reaches
git clone <url> with an empty destination; this branch reaches
git clone <url> <resolved path>.
get_agni.sh: the destination is the symlink-resolved root rather than the
logical one (same directory, different spelling).

PETSC_DIR, PETSC_ARCH and the SPIDER default destination are byte-identical
to main in both the default and the custom-path cases.

Tests, tests/tools/test_install_scripts.py:

The cases source the shipped tools/_get_common.sh instead of copying the
shell under test, so a change to a helper re-runs through all of them. Each
helper case runs under a plain shell and under set -euo pipefail, the
split across the scripts themselves.
New coverage: a destination that does not exist yet; the checkout root taken
from the library's own location rather than the CWD; the --force and
install-path split in either order and with no arguments at all (the edge
case an unguarded "$@" would abort under set -u); the guard's pathspec
exclusion, including that the same modification blocks without the
exclusion; the SSH probe's exit-code mapping (1 accepted, 255 refused, 0
refused) and its GIT_SSH_COMMAND override; that the probe's own banner on
stdout cannot contaminate the answer; the URL rewrite leaving a non-github
and an already-SSH URL alone; a missing module pin stopping the install; a
checkout whose state git cannot report being kept in either shell;
argument parsing leaving a caller's own variables alone; and each script's
clone destination and transport end to end, run from outside the throwaway
checkout so that a destination taken from the working directory cannot
coincide with the expected path.
Two invariants replace the stopgap from Resolve a get_* install path that does not exist yet #862 that pinned the nine copies as
identical text: no get_*.sh carries a private copy of a helper, and every
script that calls one sources the file defining it first. Both read the
helper names out of the library rather than listing them, so a rename
cannot drop a helper out of the scan. The private-copy scan matches every
definition spelling bash accepts (name() {, name () {,
function name {), including a copy that shadows the library in a script
that does source it, and matches the dirty test and the SSH probe as
commands rather than as text, so get_spider.sh naming the probe in its
troubleshooting output is not mistaken for a copy. The source-line
invariant compares line order, because a call above the source line is
the silent no-op it warns about.
Every invariant case was checked against the mutation it exists to catch,
and fails under it: reintroducing a private portable_realpath in three
spellings, including in a script that sources the library; a private
github_use_ssh; the dirty test in its -uno short form; dropping a
source line and moving a call above it; dropping the bootstrap's
missing-file check; making a destination CWD-relative (11 of the 12
whole-script cases fail, the twelfth passing an absolute path); restoring
the guard's piped probe; leaking the parser's loop variable; and dropping
the library from the cache key.

Suite runs:

pytest tests/tools/ -q: 272 passed.
pytest -m "unit and not skip and not slow and not integration" --ignore=tests/examples: 3025 passed, 45 skipped, 9 failed. The same 9 fail
on main in this environment (all in tests/atmos_clim/test_janus.py,
unrelated to shell scripts).
pytest tests/test_doctor.py tests/test_cli.py tests/utils/: 807 passed,
5 skipped. These are the suites covering the callers that invoke the scripts.
ruff check src/ tests/ and ruff format --check src/ tests/: clean.
bash tools/validate_test_structure.sh: complete.
python tools/check_test_quality.py --check: no violations in the touched
file (its pre-existing regressions are in other files).
bash -n on every tools/*.sh: clean.

Observed but not changed here: in get_vulcan.sh, the
floor=$(grep -oE ... | head -1 | sed ...) pipeline runs under
set -euo pipefail, so a pyproject.toml without an fwl-vulcan>= pin kills
the script at that line and the WARNING: could not read fwl-vulcan floor
branch below it is unreachable. That predates this PR and is unrelated to the
shared helpers, so it is left for its own change.

Not verified:

shellcheck is not installed on this host, so the scripts were not linted
with it.
No real clone or build was run: git, ssh, and the compilers were stubbed,
so the SOCRATES, SPIDER and PETSc build steps past the clone are unexercised.
CI exercises the SOCRATES path for real.
GNU realpath on a missing parent was not exercised on a Linux host, only
its documented behaviour. The fix does not depend on which of the two
refuses, since any refusal falls through to python3.
The SSH probe expands GIT_SSH_COMMAND unquoted so that its options reach
ssh as separate words, which is what get_socrates.sh did before. An option
value containing a space therefore does not survive; the probe then exits
non-1 and the script falls back to https, which is safe but silent.
Invoking a get_*.sh through a symlink placed outside tools/ now stops
with the missing-library error, where before it resolved through $0. No
caller in the repo or the docs does that, and the failure is loud rather than
silent.

Checklist

I have followed the contributing guidelines
My code follows the style guidelines of this project
I have performed a self-review of my code
My changes generate no new warnings or errors
I have checked that the tests still pass on my computer
I have updated the docs, as appropriate (the install-script conventions
in docs/How-to/development_standards.md; the installation docs call the
scripts by path, which is unchanged)
I have added tests for these changes, as appropriate
I have checked that all dependencies have been updated, as required (none
changed; python3 was already the fallback inside the helper)

The tools/get_*.sh scripts each carried their own copy of the same shell logic: portable_realpath in nine of them, the dirty-checkout guard in seven, the --force argument split in six, and the GitHub SSH probe in four, plus the repo-root derivation in nearly all of them. A fix to one copy left the others untouched, which is how a portable_realpath that rejects a path git clone has yet to create survived in nine places at once. tools/_get_common.sh now holds one copy of each helper, and every script sources it through its own directory, so a run by path (the docs, install.sh, the CI setup action, and proteus install-all through data.py) finds it without resolving a path first. A script whose library is absent stops with the missing file named instead of continuing with undefined helpers. The shared portable_realpath resolves a destination that does not exist yet, which is what an install path is before the clone, and get_socrates.sh stops when its install path cannot be resolved rather than passing an empty work-tree name to git clone. Variations that differ per script are parameters, not separate copies: guard_dirty_checkout takes a git pathspec so get_socrates.sh keeps excluding its regenerable make/Mk_cmd, get_parse_args reports both the --force switch and the optional install path, and the SSH probe honours GIT_SSH_COMMAND for every caller rather than only for SOCRATES. The helpers target bash 3.2 and behave the same with or without set -euo pipefail, because the scripts differ on that. Destinations are unchanged, with one exception: get_agni.sh and get_spider.sh now derive the checkout root through the same symlink-resolving path as the other scripts, so a checkout reached through a symlink resolves to the same root everywhere.

The cases for the install scripts read tools/_get_common.sh rather than carrying their own copy of the shell under test, so a change to a helper re-runs through them. They pin what a shared helper can silently change: the checkout root derived from the library's own location, the --force and install-path split in either argument order, the refusal to delete a checkout holding local work and the pathspec exclusion get_socrates.sh passes it, the SSH probe exit codes and its GIT_SSH_COMMAND override, the https-to-SSH URL rewrite, and a missing module pin stopping the install. Each runs under a plain shell and under set -euo pipefail, which is the split across the scripts themselves. Two invariants stand behind the rest: no get_*.sh carries a private copy of a helper, and every script that calls one also sources the file defining it. A script that grows a private copy again is outside the reach of these cases, which is what let the same four-line path fix be needed in nine places. Each script also runs whole against a stubbed git and ssh in a throwaway checkout, pinning the destination it resolves and the transport it selects, including a SOCRATES install path that does not exist yet and the https fallback when no key is accepted.

The cache key for the compiled SOCRATES tree hashes the install script, so a reworded build does not restore a tree the previous one produced. The script now sources tools/_get_common.sh, so a change to the SSH probe, the checkout guard, or the pin lookup left the key unchanged and would have restored a stale tree with nothing reporting the mismatch. The key hashes both files, and a case pins that it names every file the build step reads.

The dirty-checkout guard read both probes through `git ... | head -1`, whose status is git's under `set -o pipefail` and head's without it. A checkout with an unborn HEAD or an incomplete .git makes `git log HEAD` exit 128, so the same state stopped get_boreas.sh and get_vulcan.sh with no message at all and was silently deleted by the other six, staged work included. Both probes now run unpiped, bounded by git's own -1, and a probe that cannot run is reported with the checkout kept and --force named. The guard reaches the same verdict in either shell, which is what the library documents. Alongside: get_parse_args declares its loop variable, so it cannot clobber a caller's `arg`, and the aragog and Zalmoxis scripts quote the work path they delete and the root they return to. The cases behind this: the guard runs under both shells rather than only a plain one, and a checkout git cannot report on is pinned as kept in both. The two invariants that stand behind the extraction are sharpened, because both could be defeated by shell they were meant to catch: the private-copy scan reads the function names out of the library and matches every definition spelling bash accepts, including a copy that shadows the library in a script that does source it, and matches the dirty test and the SSH probe as commands rather than as text, so a message that names ssh is not mistaken for a probe; the source-line invariant compares line order, since a call above the source line is the silent no-op it warns about. The whole-script clone cases now run from outside the throwaway checkout, so a destination taken from the working directory can no longer coincide with the expected path.

The helper shares a namespace with every script that sources it, and `arg` is an ordinary name for a script to use, so a leaked loop variable would overwrite the caller's value with the last argument, silently and only when arguments are passed.


 import pytest

+pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]


codecov · 2026-09-09T12:40:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.46%. Comparing base (24a7040) to head (9896ac3).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #864   +/-   ##
=======================================
  Coverage   93.45%   93.46%           
=======================================
  Files         113      113           
  Lines       16797    16797           
  Branches     2992     2992           
=======================================
+ Hits        15698    15699    +1     
+ Misses       1093     1092    -1     
  Partials        6        6

Flag	Coverage Δ
unit-tests	`87.25% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nichollsh

This is great and makes the code much easier to maintain. Thanks, @egpbos, for cleaning these up. I have tested this on my laptop (Fedora 44) and it seems to work well. That is, the scripts download/compile the respective modules as before.

Looks good to merge into main.

Do we want to perform a similar overhaul to the get_* scripts in the submodules?

egpbos added 5 commits September 9, 2026 14:05

egpbos requested a review from a team as a code owner September 9, 2026 12:30

github-code-quality Bot found potential problems Sep 9, 2026

View reviewed changes

Comment thread tests/tools/test_install_scripts.py

import pytest

pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]

nichollsh approved these changes Sep 9, 2026

View reviewed changes

Navigation Menu

Uh oh!

Source the get_* install scripts from one shared shell library - #864

Source the get_* install scripts from one shared shell library#864
egpbos wants to merge 5 commits into
mainfrom
tools/get-script-common-library

egpbos commented Sep 9, 2026

Uh oh!

codecov Bot commented Sep 9, 2026

Uh oh!

nichollsh left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		import pytest

		pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)]

Uh oh!

Conversation

egpbos commented Sep 9, 2026

Description

Validation of changes

Checklist

Uh oh!

codecov Bot commented Sep 9, 2026

Codecov Report

Uh oh!

nichollsh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants