Clarify pure-rust-build cc1 wrapping
#1872
Merged
Conversation
This file contains 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
EliahKagan
commented
Mar 3, 2025
Comment on lines
+101
to
+112
| # 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 |
There was a problem hiding this comment.
I'm not sure if it's clearer to present it this way, or to have the command in the subsequent step as it previously was, but just include this comment there. I lean slightly toward thinking this is clearer, considering that this step is defining other scripts anyway. But this file contains only one command, and it must be sourced rather than run. Unlike the scripts, the only reason it exists is so that it can be commented here, in this preparatory step where the comment makes the most sense.
If the comment is moved into the subsequent step, then it would be worded like this:
# Wire 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 occur in this step, because different steps write to different
# pipe objects. (This also needs the shell that runs it to remain running. But that is
# not the cause of the underlying limitation.)
This removes a comment that is obsolete as of GitoxideLabs#1793.
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.
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 GitoxideLabs#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.
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).
EliahKagan
force-pushed
the
run-ci/workflow-readability
branch
from
04806da to
69582a4
Compare
Byron
approved these changes
Mar 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 main change here is to comment and refactor the additions made in #1682, to clarify what is going on. The fb67a78 and 04806da commit messages have further details. There is one part where I'm not sure if the way I've clarified it is the best way; I'll post something below for that so it can be reviewed.
This also includes some other comment improvement/maintenance in CI workflows: b856ad9 removes a comment that is obsolete since #1793, and ab049f6 makes clearer what
shell: bashis for in the various places we use it, since it serves several different purposes.(The failure here is in
test-fixtures-windowsand unrelated to the changes. That job is broken due to #1849, including on main, if rerun. As in #1871, this could be rebased after #1870 is merged, if desired. But unlike #1871, this does not make significant changes that affect other behavior of that job, so rebasing is probably not necessary to be confident that the failure isn't obscuring any separate problems from the changes here.)