From b8b4f7c8d28cb7ce1c09295197544a5f670c7e21 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 2 Mar 2025 14:24:03 -0500 Subject: [PATCH 1/4] Remove an obsolete comment in `test-fixtures-windows` This removes a comment that is obsolete as of #1793. --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index db1e2399cc9..2f8dc16679f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -226,7 +226,6 @@ jobs: - name: Compare expected and actual failures run: | # Fail on any differences, even unexpectedly passing tests, so they can be investigated. - # (If the job is made blocking for PRs, it may make sense to make this less stringent.) git --no-pager diff --no-index --exit-code --unified=1000000 --color=always -- ` etc/test-fixtures-windows-expected-failures-see-issue-1358.txt actual-failures.txt From ab4776966c49844f96341dfb0ab9862456b6c8be Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 2 Mar 2025 18:09:04 -0500 Subject: [PATCH 2/4] Clarify the various purposes of `shell: bash` We set `shell: bash` explicitly in several places, for three different reasons. At least two of the reasons (those in `ci.yml`) are not not obvious without explanation. This clarifies the existing comments that explained this, and adds such comments where absent. --- .github/workflows/ci.yml | 6 +++--- .github/workflows/release.yml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f8dc16679f..a5b25c288e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,7 +38,7 @@ jobs: ) apt-get update apt-get install --no-install-recommends -y -- "${prerequisites[@]}" - shell: bash + shell: bash # This step needs `bash`, and the default in container jobs is `sh`. - name: Verify that we are in an environment with limited dev tools run: | set -x @@ -264,7 +264,7 @@ jobs: dpkg --add-architecture ${{ matrix.runner-arch }} apt-get update apt-get install --no-install-recommends -y -- "${prerequisites[@]}" - shell: bash + shell: bash # This step needs `bash`, and the default in container jobs is `sh`. - uses: actions/checkout@v4 - name: Install Rust via Rustup run: | @@ -427,7 +427,7 @@ jobs: defaults: run: - shell: bash # Without specifying this, we don't get `-o pipefail`. + shell: bash # Without this, the shell here is `bash` but without `-o pipefail`. steps: - name: Find this workflow diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b76dd8c5f26..8de670112b9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,7 +17,7 @@ permissions: defaults: run: - shell: bash + shell: bash # Use `bash` even in the Windows jobs. jobs: # Create a draft release, initially with no binary assets attached. From 01c7b0e9102281d702463374867709be81d0f2e9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 2 Mar 2025 17:12:46 -0500 Subject: [PATCH 3/4] Clarify `pure-rust-build` cc1 wrapping This adds comments, and also refactors, to make these steps of the `pure-rust-build` CI job more readable, and to make clearer what the wrapping does and how it works: - "Wrap cc1 (and cc1plus if present) to record calls" - "Build max-pure with limited dev tools and log cc1" The logic clarified here was introduced in #1682. The clarification is mainly through these two changes: - Document each of the scripts the steps create and use with an explanatory comment above the "stanza" of code that creates it. - Extract the command to create the `~/display` symlink to such a script, which also has such a comment to explain the effect of writing to that symlink, why it is needed, and why it has to be set in the same GitHub Actions step as the related `cargo` command. Two other less significant clarifcations are made, which arguably are not refactorings in that they could change the behavior (for the better) in some hypothetical situations, but the goal is clarity rather than a behavioral change: - In the scripts that had more than one non-shebang line, take `-e` out of the shebangs and use a `set -e` commmand. This makes no difference in how the scripts are used, since they are always executed directoy. But may make them easier to read, as readers need not check that they are only run in this way to verify their understanding of what they do. - Set `noclobber` in the step that uses `>` to create the script files in `/usr/local`, so that if they somehow clash with files already there, we get an error rather than proceeding and maybe having them called in unantiicpated ways. The likelihood this would happen on a GHA runner is very low, so the real impact of this change is to make immediately clear to readers that the scripts and their names do not have a pre-defined meaning but are instead simply helpers for these GHA steps. --- .github/workflows/ci.yml | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a5b25c288e3..475afb10a19 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,16 +70,29 @@ jobs: ! grep -qP '(?/usr/local/bin/wrapper1 <<'EOF' - #!/bin/sh -e + #!/bin/sh + set -e printf '%s\n' "$0 $*" | flock /run/lock/wrapper1.fbd136bd-9b1b-448d-84a9-e18be53ae63c.lock \ tee -a -- /var/log/wrapper1.log ~/display >/dev/null # We'll link ~/display later. exec "$0.orig" "$@" EOF + # Define the script that performs the wrapping. This script shall be run once for each + # executable to be wrapped, renaming it with a `.orig` suffix and replacing it with a + # symlink to the wrapper script, defined above. cat >/usr/local/bin/wrap1 <<'EOF' - #!/bin/sh -e + #!/bin/sh + set -e dir="$(dirname -- "$1")" base="$(basename -- "$1")" cd -- "$dir" @@ -87,14 +100,26 @@ jobs: ln -s -- /usr/local/bin/wrapper1 "$base" EOF - chmod +x /usr/local/bin/wrap1 /usr/local/bin/wrapper1 + # Define a script that wires up the `~/display` symlink that `wrapper1` uses to report + # calls as GitHub Actions step output (in addition to writing them to a log file). This + # is needed because stdout and stderr are both redirected elsewhere when the wrapper + # actually runs, and `/dev/tty` is not usable. This script must be run in the same step + # as the `cargo` command that causes wrapped executables to be run, because different + # steps write to different pipe objects. (As implemented, this also needs the calling + # shell to remain running, but that is not the cause of the underlying limitation.) + cat >/usr/local/bin/set-display <<'EOF' + #!/bin/sh + ln -s -- "/proc/$$/fd/1" ~/display + EOF + + chmod +x /usr/local/bin/wrapper1 /usr/local/bin/wrap1 /usr/local/bin/set-display mkdir /run/lock/wrapper1.fbd136bd-9b1b-448d-84a9-e18be53ae63c.lock find /usr/lib/gcc \( -name cc1 -o -name cc1plus \) \ -print -exec /usr/local/bin/wrap1 {} \; - name: Build max-pure with limited dev tools and log cc1 run: | - ln -s -- "/proc/$$/fd/1" ~/display # Bypass `cc1` redirection. + /usr/local/bin/set-display cargo install --debug --locked --no-default-features --features max-pure --path . - name: Show logged C and C++ compilations (should be none) run: | From 69582a4ccf69ae2a000a4be9a6a99061f3878911 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 2 Mar 2025 18:28:06 -0500 Subject: [PATCH 4/4] The set-display script needs to be sourced, not run This removes the executable bit and shebang, names it `.sh` as is traditional for a file being sourced, and sources it. The reason it needs to be sourced is that it is using a symlink that goes through an entry specific to the current proces under `/proc`. This is done because the symlink is one of the symlinks in the process filesystem that has special semantics: it refers to stdout for the process, even when stdout is not a file that could otherwise be accessed with a path on disk. (We need that because the stdout and stderr streams in a GitHub Actions step go to an unnamed pipe object.) But if it is run as a script then the new shell instance that runs the script is the current process, which then goes away, breaking the symlink (or causing it go to the wrong place if another process is created that reuses the old PID). --- .github/workflows/ci.yml | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 475afb10a19..6bec8a3f615 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -100,26 +100,25 @@ jobs: ln -s -- /usr/local/bin/wrapper1 "$base" EOF - # Define a script that wires up the `~/display` symlink that `wrapper1` uses to report - # calls as GitHub Actions step output (in addition to writing them to a log file). This - # is needed because stdout and stderr are both redirected elsewhere when the wrapper - # actually runs, and `/dev/tty` is not usable. This script must be run in the same step - # as the `cargo` command that causes wrapped executables to be run, because different - # steps write to different pipe objects. (As implemented, this also needs the calling - # shell to remain running, but that is not the cause of the underlying limitation.) - cat >/usr/local/bin/set-display <<'EOF' - #!/bin/sh + # Define a helper file that, when sourced, wires up the `~/display` symlink `wrapper1` + # uses to report calls as GitHub Actions step output (in addition to writing them to a + # log file). This is needed because stdout and stderr are both redirected elsewhere when + # the wrapper actually runs, and `/dev/tty` is not usable. This must be sourced in the + # same step as the `cargo` command that causes wrapped executables to be run, because + # different steps write to different pipe objects. (This also needs the shell that + # sourced it to remain running. But that is not the cause of the underlying limitation.) + cat >/usr/local/bin/set-display.sh <<'EOF' ln -s -- "/proc/$$/fd/1" ~/display EOF - chmod +x /usr/local/bin/wrapper1 /usr/local/bin/wrap1 /usr/local/bin/set-display + chmod +x /usr/local/bin/wrapper1 /usr/local/bin/wrap1 mkdir /run/lock/wrapper1.fbd136bd-9b1b-448d-84a9-e18be53ae63c.lock find /usr/lib/gcc \( -name cc1 -o -name cc1plus \) \ -print -exec /usr/local/bin/wrap1 {} \; - name: Build max-pure with limited dev tools and log cc1 run: | - /usr/local/bin/set-display + . /usr/local/bin/set-display.sh cargo install --debug --locked --no-default-features --features max-pure --path . - name: Show logged C and C++ compilations (should be none) run: |