Skip to content

Force linting scripts to align rust toolchain with latest nix shell and not latest nightly #1536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DanGould merged 1 commit into from
May 12, 2026
Merged

Force linting scripts to align rust toolchain with latest nix shell and not latest nightly #1536

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,30 +45,30 @@ jobs:
steps:
- name: "Checkout repo"
uses: actions/checkout@v4
- name: "Install nightly toolchain"
uses: dtolnay/rust-toolchain@nightly
with:
components: clippy
- name: "Use cache"
uses: Swatinem/rust-cache@v2
- name: "Install nix"
uses: DeterminateSystems/determinate-nix-action@main
- name: "Use nix cache"
uses: DeterminateSystems/magic-nix-cache-action@main
- name: "Run linting check"
run: cd payjoin-ffi && bash contrib/lint.sh
run: cd payjoin-ffi && nix develop -c ./contrib/lint.sh

Lint:
runs-on: ubuntu-latest
steps:
- name: "Checkout repo"
uses: actions/checkout@v4
- name: "Install nightly toolchain"
uses: dtolnay/rust-toolchain@nightly
with:
components: clippy
- name: "Use cache"
uses: Swatinem/rust-cache@v2
Comment on lines 62 to 63
Copy link
Copy Markdown
Contributor

@nothingmuch nothingmuch May 8, 2026

Choose a reason for hiding this comment

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

Suggested change
- name: "Use cache"
uses: Swatinem/rust-cache@v2

i suspect this depends on having rust tooling installed, please confirm:

  1. does it work without the dtolnay/rust-toolchain bootstrap? if not, is it fine to mix nightly toolchains, rustup one to manage the cache and nix one to actually build and run things?
  2. does it have any effect (iirc clippy will do cargo check, does the output of that get cached? does caching via github actually save time?)

Copy link
Copy Markdown
Collaborator Author

@benalleng benalleng May 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The caching does seem to still work. Here is a run after the workflow changed so no cache.
https://github.com/payjoin/rust-payjoin/actions/runs/25561782784/job/75034980403?pr=1536#step:3:1 followed by a subsequent run with a cache even after the rust toolchain install had been removed https://github.com/payjoin/rust-payjoin/actions/runs/25562206670/job/75036441940?pr=1536#step:3:1

Even with using the dev shell the rust cache seems to be a substantial time saver.

Copy link
Copy Markdown
Contributor

@nothingmuch nothingmuch May 8, 2026

Choose a reason for hiding this comment

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

caching definitely makes sense if it's impactful, quick feedback is always helpful.

if dtolnay/rust-toolchain is still needed for Swatinem/rust-cache then please just add a big scary comment that makes it clear that the lints are running using nix provided rust toolchain and that the additional rustup provided one is there only for the cache action to work

Copy link
Copy Markdown
Contributor

@nothingmuch nothingmuch May 8, 2026

Choose a reason for hiding this comment

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

ugh, github's new UI is such a buggy piece of shit, but in addition to being buggy it's also confusing, i clicked into the diff and thought i was looking at a recent one and concluded that only one copy of the rust-toolchain action was removed for some reason

- name: "Install nix"
uses: DeterminateSystems/determinate-nix-action@main
- name: "Use nix cache"
uses: DeterminateSystems/magic-nix-cache-action@main
- name: "Run code linting"
run: bash contrib/lint.sh
run: nix develop -c ./contrib/lint.sh
- name: "Run documentation linting"
run: RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --document-private-items
run: nix develop -c -- bash -c 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --document-private-items'
Comment on lines -69 to +71
Copy link
Copy Markdown
Contributor

@DanGould DanGould May 9, 2026

Choose a reason for hiding this comment

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

running nix develop multiple times smells to me.

Every time nix develop runs

  • Nix evaluates the flake/derivation from scratch to compute the dev shell's environment
  • It checks the store for all dependencies (even if cached, this involves hash verification)
  • A new shell process is spawned, environment variables are set up, shellHook runs

maxic-nix-cache only eliminates downloads.

Robot suggests the following environment setup step as best practice.

- name: Set up dev environment
    run: nix print-dev-env >> $GITHUB_ENV

but I'm not sure yet what best practice here actually is, but my hunch is that they're doing it right at fedimint. Environment setup and tool running are separate steps that should stay separate to be clean IMO. Non-blocking, but needs to be fixed after.

Copy link
Copy Markdown
Contributor

@DanGould DanGould May 9, 2026

Choose a reason for hiding this comment

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

Correction:

The shim that does this right seems to be nicknovitski/nix-develop: runs nix develop once, writes the env diff to $GITHUB_ENV / $GITHUB_PATH so subsequent steps inherit it. It lets Swatinem/rust-cache key on the nix-provided rustc. It doesn't work if we depend on shellHooks for setup, but we don't.

Still non-blocking. Leaving as concrete pointer for the follow-up (please make an issue when u merge/close)

Copy link
Copy Markdown
Contributor

@DanGould DanGould May 9, 2026

Choose a reason for hiding this comment

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

fedimint does a bunch of complicated stuff which is great for them but loses the multi-toolchain running. They have unique derivations for Ci jobs rather than defining them primarily in YML. And have their own runners that stay warm with their caches. Different thing we don't need yet.

Copy link
Copy Markdown
Contributor

@nothingmuch nothingmuch May 9, 2026

Choose a reason for hiding this comment

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

microsoft donating 1s more of cpu time seems like a fair price to pay for not introducing a github action dependency that isn't really necessary, or rolling our own and debugging string escaping rules in bash in javascript in yaml

nix develop -c is reliable and predictable, and also clarifies to the reader under what environment something is being run on github

if we reach the point where it's more than just a sprinkling here or there and that action is legitimately useful i'd argue things are too tightly coupled to the nightmare that is github actions

Copy link
Copy Markdown
Collaborator Author

@benalleng benalleng May 11, 2026

Choose a reason for hiding this comment

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

Interesting what fedimint does here just running their whole pre-commit hook https://github.com/fedimint/fedimint/blob/c1f97273aa59c1580f03a38530ad104072cdb412/.github/workflows/ci-nix.yml#L62


Coverage:
name: Code coverage
Expand Down
Loading