Skip to content
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.

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

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

Closed
mykmelez opened this issue Jul 12, 2022 · 8 comments
Labels

Comments

@mykmelez
Copy link

mykmelez commented Jul 12, 2022

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 mykmelez added the bug label Jul 12, 2022
@kinnison
Copy link
Contributor

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 #2958 )

@ghost
Copy link

ghost commented Jul 12, 2022

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

@IceTDrinker
Copy link

IceTDrinker commented Jul 12, 2022

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

@nicholasbishop
Copy link
Contributor

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
Copy link
Contributor

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 added a commit to nicholasbishop/uefi-rs that referenced this issue Jul 12, 2022
Starting in rustup 1.25.0, the environment is modified during `cargo`
invocations in a way that breaks setting the channel arg
(e.g. "+nightly") in a subprocess.

Fix by unsetting those variables in the child cargo's env.

See rust-lang/rustup#3031
nicholasbishop added a commit to rust-osdev/uefi-rs that referenced this issue Jul 12, 2022
Starting in rustup 1.25.0, the environment is modified during `cargo`
invocations in a way that breaks setting the channel arg
(e.g. "+nightly") in a subprocess.

Fix by unsetting those variables in the child cargo's env.

See rust-lang/rustup#3031
Enselic added a commit to cargo-public-api/cargo-public-api that referenced this issue Jul 12, 2022
From `rustup 1.25.0` the env var `RUSTDOC` is set to avoid having to
resolve proxy settings more than once. Since we run under the stable
toolchain, it means `RUSTDOC` will point to stable `rustdoc`. This
overrides the toolchain we specify with `+`. Clear `RUSTENV` so we can
run the `+nightly` toolchain.

See rust-lang/rustup#3031 (comment)
Enselic added a commit to cargo-public-api/cargo-public-api that referenced this issue Jul 12, 2022
From `rustup 1.25.0` the env var `RUSTDOC` is set to avoid having to
resolve proxy settings more than once. Since we run under the stable
toolchain, it means `RUSTDOC` will point to stable `rustdoc`. This
overrides the toolchain we specify with `+`. Clear `RUSTENV` so we can
run the `+nightly` toolchain.

See rust-lang/rustup#3031 (comment)
@jyn514
Copy link
Member

jyn514 commented Jul 12, 2022

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

@nicholasbishop
Copy link
Contributor

Thanks, since it sounds like the Windows thing is a separate problem I've filed a new bug for it: #3036

Noah-Kennedy added a commit to tokio-rs/tokio that referenced this issue Jul 13, 2022
This reverts [#4825], which was to address Miri being broken on the latest nightly, possibly due to an issue in rustup.

Rustup issue: rust-lang/rustup#3031

[#4825]: #4825
Noah-Kennedy added a commit to tokio-rs/tokio that referenced this issue Jul 13, 2022
This reverts [#4825], as miri seems to be working again on the latest nightly.

I had originally thought it was this issue with rustup which was to blame, but this seems to be wrong: rust-lang/rustup#3031

[#4825]: #4825
@rami3l
Copy link
Member

rami3l commented Jul 10, 2024

Since this change has been reverted in #3034, let's continue the discussion in #3035.

@rami3l rami3l closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants