Skip to content

ci: GitHub Actions workflow improvements - #1697

Open
notpeter wants to merge 6 commits into
mainfrom
gha_hygeine2
Open

ci: GitHub Actions workflow improvements#1697
notpeter wants to merge 6 commits into
mainfrom
gha_hygeine2

Conversation

@notpeter

@notpeter notpeter commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
  • Supersedes ci: GitHub Actions hygeine and MSRV 1.88 #1693
  • Explicitly specify permissions for all GitHub Actions workflows
  • Update Swatinem/rust-cache to v2.9.2 from v2.9.1
  • Update actions/checkout to v7.0.1 (add comment with version)
  • Pin actions/setup-node to a SHA (# v7.0.0)
  • Drop dtolnay/rust-toolchain
  • Add CARGO_INCREMENTAL=0 to match the previous dtolnay/rust-toolchain behavior

Comment thread .github/workflows/rust.yml Outdated
Comment on lines +102 to +104
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
# Deliberately no RUSTUP_TOOLCHAIN here, uses rust-toolchain.toml to pin version.
# trybuild fixtures assert exact rustc diagnostics in committed .stderr files.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep things as they are (I think turning off incremental compilation is important, and the color is a nice bonus as well).

@notpeter notpeter Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I've reverted back to dtolnay/rust-toolchain.

@notpeter notpeter Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here's actually my issue with dtolnay/rust-toolchain -- it actually makes implicit a bunch of things that would be better off to be explicitly defined.

Your comment here was incorrect:

steps:
- uses: actions/checkout@0c366fd6a839edf440554fa01a7085ccba70ac98
# rust-toolchain.toml overrides dtolnay/rust-toolchain@stable, so in some
# sense using it is pointless. But it does a few other useful things such
# as disable incremental compilation, so we use it anyway.
- uses: dtolnay/rust-toolchain@stable

See: yesterday's trybuild job run on main
Screenshot 2026-09-04 at 10 01 24

In fact dtolnay/rust-toolchain never respects rust-toolchain.toml. It requires specifying an explicit toolchain in your workflow, but when you pin to dtolnay/rust-toolchain@stable branch it automatically uses stable. If you attempt to pin to a commit hash in main it shows its true colors and says toolchain is required. job link. source link.

Sadly, the way this implemented also prevents you from pinning to explicit stable commits even if you want that behavior. While the v1 tag moves it points to commits in master (which is pretty common for GitHub Actions). But stable is implemented as a divergent branch. Every time there is a release it is rebased to master and any previous commit becomes orphaned and subject to being garbage collected.

And these behaviors are intentional:

Re: rust-toolchain.toml support
I would prefer not to support it in this action. But someone else could maintain a different action that provides that pair of steps as one action.
dtolnay comment

In a workflow that pins the action using a full-length commit SHA (as opposed to something like @nightly or @1.89.0) it is required that you pick a SHA that is within the history of the master branch. Any commit that is not within the history of master will eventually get garbage-collected and your workflows will fail.
dtolnay/rust-toolchain README.md

To me this sort of complexity is not worth it. CARGO_INCREMENTAL: 0 was the only env var we were getting implicitly from dtolnay/rust-toolchain, RUST_BACKTRACE and CARGO_TERM_COLOR were already being explicitly set:

env:
CARGO_TERM_COLOR: always
RUST_BACKTRACE: 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, okay, thanks -- makes sense. (Though I think my comment agrees with what you said about precedence?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants