rustup icon indicating copy to clipboard operation
rustup copied to clipboard

rustup 1.25 sets RUSTC, which overrides custom toolchain specified in sub-Cargo invocation

Open mykmelez opened this issue 3 years ago • 7 comments

Problem

I have a Rust build wrapper (xtask) that invokes Cargo with a custom toolchain +esp and a toolchain-specific target --target xtensa-esp32s3-espidf, so I can use a simple command like cargo xtask build to cross-compile my project with a more complex command like cargo +esp --target xtensa-esp32s3-espidf.

This works with rustup 1.24, but rustup 1.25 defines RUSTC in the environment of the build wrapper, setting it to the default compiler, which causes the sub-Cargo invocation in the build wrapper to ignore the custom toolchain specified in the +esp argument and instead invoke the rustc compiler specified in RUSTC.

And since that compiler doesn't support the toolchain-specific target, the invocation fails:

error: failed to run rustc to learn about target-specific information

Caused by: process didn't exit successfully: /Users/myk/.rustup/toolchains/1.59.0-aarch64-apple-darwin/bin/rustc - --crate-name ___ --print=file-names --target xtensa-esp32s3-espidf --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=cfg (exit status: 1) --- stderr error: Error loading target specification: Could not find specification for target "xtensa-esp32s3-espidf". Run rustc --print target-list for a list of built-in targets

Steps

I don't yet have a set of steps to reproduce, but I'll work on a minimal test case.

Possible Solution(s)

It isn't clear why RUSTC now gets defined in the environment, but if there isn't a good reason for it, then reverting that change would obviously fix the bug. (I'm working around the issue at the moment by undefining RUSTC in the build wrapper before it invokes Cargo.) If there is a good reason for it, then I'm not sure what the solution would be.

Notes

I wonder if #3025 and/or #3029 are related.

Rustup version

Version that works:

>\> rustup --version
>rustup 1.24.0 (2f894d923 2021-04-24)

Version that is broken:

>\> rustup --version
>rustup 1.25.0 (90365aa81 2022-06-15)

In both cases:

>info: This is the version for the rustup toolchain manager, not the rustc compiler.
>info: The currently active `rustc` version is `rustc 1.59.0 (9d1b2106e 2022-02-23)`

Installed toolchains

> rustup show
Default host: aarch64-apple-darwin
rustup home:  /Users/myk/.rustup

installed toolchains
--------------------

stable-aarch64-apple-darwin (default)
nightly-aarch64-apple-darwin
esp
1.56.1-aarch64-apple-darwin
1.57.0-aarch64-apple-darwin
1.59.0-aarch64-apple-darwin
1.61.0-aarch64-apple-darwin

installed targets for active toolchain
--------------------------------------

aarch64-apple-darwin
aarch64-linux-android
armv7-linux-androideabi
i686-linux-android
thumbv7em-none-eabi
x86_64-apple-darwin
x86_64-linux-android

active toolchain
----------------

1.59.0-aarch64-apple-darwin (overridden by '/Users/myk/Projects/project/rust-toolchain.toml')
rustc 1.59.0 (9d1b2106e 2022-02-23)

mykmelez avatar Jul 12 '22 05:07 mykmelez

We set RUSTC in our proxies to improve performance because it means cargo doesn't have to indirect through Rustup again for every compilation step. My gut feeling, if you are wrappering a subsequent invocation of cargo is that you should be being very careful about environment settings, though I can understand that this might have caught you off guard.

I'm tagging in the author of the commit, @jyn514 so that if he's aware of better ways to do this, we can discuss if we want to do a 1.25.1 of Rustup, or if we should improve our documentation so that others don't fall into this trap in the future.

(FYI, the commit in question was merged in https://github.com/rust-lang/rustup/pull/2958 )

kinnison avatar Jul 12 '22 07:07 kinnison

@kinnison how can I restore to the previous behavior? just remove RUSTC env inside the cargo wrapper?

Byte1234 avatar Jul 12 '22 10:07 Byte1234

Workaround in the meantime I found:

In your main/entry point for xtask:

    std::env::remove_var("RUSTC");
    // These two were apparently already present in rustup 1.24.3
    // std::env::remove_var("CARGO"); // This assumes cargo is on PATH
    // std::env::remove_var("RUSTUP_TOOLCHAIN");
    std::env::remove_var("RUSTDOC");

Removes the injected RUSTC at the level of the wrapper and looks to work ok.

Any wrapper around cargo +toolchain will need to have this for now it seems

Edit: this only solves some of the problems, if some of your dependencies use wrappers around cargo, it looks like you may be out of luck in the short term with rustup 1.25

Edit 2: I'm not sure what variables used to be populated, I updated the snippet of code to remove all hardcoded paths linked to rust I could see in a printenv

IceTDrinker avatar Jul 12 '22 11:07 IceTDrinker

Would it be possible to update the 1.25.0 blog post with this info? It's reasonably straightforward to work around with Command::env_remove, but it's a little tricky to figure out that you need to do that in the first place. When I ran into this issue, the blog post was one of the places I checked to see if there were any notes on breaking changes, so a note there might help others :)

nicholasbishop avatar Jul 12 '22 13:07 nicholasbishop

It's actually a bit trickier to fix this on Windows, because there the PATH variable must be edited as well to remove the entry that looks like C:\Users\someuser\.rustup\toolchains\stable-x86_64-pc-windows-msvc\bin.

Here's roughly what I'm using to work around this, in case it's useful to anyone else:

let mut cmd = Command::new("cargo");

cmd.env_remove("RUSTC");
cmd.env_remove("RUSTDOC");

let orig_path = env::var_os("PATH").unwrap_or_default();
let modified_split_paths = env::split_paths(&orig_path).filter(|path| {
    !path
        .components()
        .any(|component| component.as_os_str() == ".rustup")
});
let modified_path = env::join_paths(modified_split_paths).expect("invalid PATH");
cmd.env("PATH", modified_path);

nicholasbishop avatar Jul 12 '22 14:07 nicholasbishop

The additions rustup makes to PATH were not modified between 1.24 and 1.25.0. I am not sure how it's possible for your PATH changes to affect the 1.25.0 regression.

My understanding of the issue is:

  1. You run cargo xtask build
  2. cargo is actually a rustup proxy; it runs your cargo for the default toolchain and also sets RUSTC and RUSTDOC to the binaries in the default toolchain. (This is what changed in 1.25.0)
  3. One of the tests (or anything recursively invoked) runs cargo +toolchain build
  4. rustup runs that new toolchain, but does not set RUSTC (because it's already set in the environment by step 2)
  5. cargo sees that RUSTC is set and runs that toolchain instead of the shim
  6. You get a hard error because the toolchain binary doesn't recognize +toolchain

jyn514 avatar Jul 12 '22 18:07 jyn514

Thanks, since it sounds like the Windows thing is a separate problem I've filed a new bug for it: https://github.com/rust-lang/rustup/issues/3036

nicholasbishop avatar Jul 12 '22 19:07 nicholasbishop