Skip to content

rust: persist toolchain inside env (fixes #3678)#3684

Closed
Carlomus wants to merge 1 commit into
pre-commit:mainfrom
Carlomus:fix/rust-bootstrap
Closed

rust: persist toolchain inside env (fixes #3678)#3684
Carlomus wants to merge 1 commit into
pre-commit:mainfrom
Carlomus:fix/rust-bootstrap

Conversation

@Carlomus
Copy link
Copy Markdown

@Carlomus Carlomus commented May 12, 2026

When pre-commit bootstraps Rust itself, the previous code installed the toolchain into a TemporaryDirectory that was deleted as soon as install_rust_with_toolchain returned. At hook-run time, the rustup proxy could then silently auto-install the requested toolchain into the user's ~/.rustup using their globally configured profile. On CI images that set rustup set profile minimal, this could drop rustfmt and clippy and break those hooks.

This PR keeps the Rust toolchain env-local instead.

Changes:

  • Pin RUSTUP_HOME=envdir/rustup and RUSTUP_TOOLCHAIN=<version> in get_env_patch, so the installed toolchain persists and is found at both install and hook-run time.
  • Bootstrap rustup into envdir/bin/ whenever it's missing there, regardless of system rustup, so the env stays self-contained even if the user uninstalls system rustup later.
  • Scope CARGO_HOME=envdir to the rustup-init bootstrap call only, so cargo still reads the user's ~/.cargo/{config,credentials}.toml and shared registry cache during normal use.
  • Install toolchains with --profile default so rustfmt and clippy ship with the toolchain regardless of the user's globally set rustup profile.
  • Add a Rust-specific health_check that rebuilds envs built by the previous layout, and envs whose installed toolchain no longer matches the configured language_version.
  • Reject install_rust_with_toolchain('system', ...) with ValueError so the helper can't silently degrade to PATH-only env patching.

Tests cover:

  • get_env_patch shape for system, default, and pinned versions
  • bootstrap argv for rustup-init and rustup toolchain install --profile default
  • env-var pinning during rustup calls, covering the original bug class
  • CARGO_HOME scoping to rustup-init only
  • health checks for old-layout envs and wrong-toolchain envs
  • end-to-end bootstrap of a toolchain and running rustfmt through pre-commit

@Carlomus
rust: persist toolchain inside env (pre-commit#3678)
d9fcab9 
- Move RUSTUP_HOME inside envdir so the toolchain installed by
  `rustup toolchain install` survives the function return
  (was a TemporaryDirectory that got rm-rf'd).
- Set RUSTUP_HOME + RUSTUP_TOOLCHAIN in `get_env_patch` so both
  install and hook-run see the env-local toolchain.
- Pin `--profile default` so rustfmt/clippy ship with the toolchain
  regardless of the user's `rustup set profile` config.
- Scope `CARGO_HOME=envdir` to the rustup-init bootstrap only, so
  hooks still see `~/.cargo/{config,credentials}.toml`.
- Add a `health_check` to invalidate envs built by the old layout
  and envs whose installed toolchain no longer matches the configured
  `language_version`.

Fixes pre-commit#3678
@asottile
Copy link
Copy Markdown
Member

asottile commented May 12, 2026

sorry we don't accept ai slop PRs

@asottile asottile closed this May 12, 2026
@Carlomus
Copy link
Copy Markdown
Author

Carlomus commented May 12, 2026 •
edited
Loading

What is your metric for AI slop exactly?
What would you need for this change to be merge-able?

@asottile
Copy link
Copy Markdown
Member

asottile commented May 12, 2026

well for starters I don't think you understand what the llm changed and this doesn't even do the minimal work to fix the "problem" (if there even is one -- the original issue never really triaged as to whether something wasn't working or not)

@Carlomus
Copy link
Copy Markdown
Author

Carlomus commented May 12, 2026 

with tempfile.TemporaryDirectory() as rustup_dir:
        with envcontext((('CARGO_HOME', envdir), ('RUSTUP_HOME', rustup_dir))):

sets RUSTUP_HOME as a temp dir that gets deleted when exiting the context manager.

            cmd_output_b(
                'rustup', 'toolchain', 'install', '--no-self-update',
                toolchain,
            )

Installs the toolchain into RUSTUP_HOME

So when the function is exited, RUSTUP_HOME gets deleted. You still have your proxies in CARGO_HOME/bin, so you can still pick up a version of rust sometimes (and this whole path is only triggeredfor npn-system rust environments, which is rare) so this is normally not caught.

What my PR does is simply decouple RUSTUP_HOME, set by

if version != 'system':
        patches += (
            ('RUSTUP_HOME', os.path.join(target_dir, 'rustup')),
            ('RUSTUP_TOOLCHAIN', _rust_toolchain(version)),
        )

and the temporary dir, which is now only used to download rustup

           with tempfile.TemporaryDirectory() as init_dir:

The rest of the changes (basically just checkhealth) were to make sure I did not affect current users of pre-commit by mistake. If pre-commit thinks that an old env already exists, it double checks the structure of the directories to not have the broken toolchain.

Now, seeing the number of closed PRs, I appreciate that being a maintainer you must get a load of slop PRs, so let me know if you would prefer this PR to be smaller (i.e. fewer test changes or getting rid of the checkhealth and having that as a separate PR), but this is a real bug and I'm just trying to upstream the fix. Otherwise, I can also just not upstream this amd just patch to fix #3678, but that's a bug we are getting and I'm just trying to help

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Carlomus @asottile