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 breaks with cargo-make due to environment variables #3029

Closed
sffc opened this issue Jul 11, 2022 · 30 comments
Closed

rustup breaks with cargo-make due to environment variables #3029

sffc opened this issue Jul 11, 2022 · 30 comments
Labels

Comments

@sffc
Copy link

sffc commented Jul 11, 2022

Problem

It appears that a recent rustup update today caused our GitHub Actions CI to start failing to install a pinned nightly version.

Passing (7hrs ago): https://github.com/unicode-org/icu4x/actions/runs/2650696918

Failing (1hr ago): https://github.com/unicode-org/icu4x/actions/runs/2652484278

GitHub Actions config file: https://github.com/unicode-org/icu4x/blob/main/.github/workflows/build-test.yml

CC @Manishearth

Steps

View the above CI reports. See how the jobs start failing with messages such as:

[cargo-make] INFO - Execute Command: "rustup" "run" "nightly-2022-04-05" "cargo" "wasm-build-release" "--package" "icu_capi_cdylib" "--features" "provider_test,smaller_test"

and

[memory] > error: no such subcommand: `+nightly-2022-04-05`

Possible Solution(s)

No response

Notes

No response

Rustup version

latest

Installed toolchains

nightly-2022-04-05
@Manishearth
Copy link
Member

Another error that we're seeing is:

error: "/home/runner/.rustup/toolchains/1.61-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src --toolchain nightly-2022-04-05-x86_64-unknown-linux-gnu

Which I think is our fault, and I have a fix in #3029 . However it's unclear how the rustup update broke that (maybe the culprit isn't the rustup update)


Apologies for the low-context issue, I figured given that rustup releases are rare it was more valuable for us to report this bug ASAP and work on figuring out more details afterwards. The +nightly thing being broken still seems sus.

@sffc
Copy link
Author

sffc commented Jul 11, 2022

Note that the following error only appears on Windows:

[memory] > error: no such subcommand: `+nightly-2022-04-05`

@Manishearth
Copy link
Member

error: "/home/runner/.rustup/toolchains/1.61-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock" does not exist, unable to build with the standard library, try:
        rustup component add rust-src --toolchain nightly-2022-04-05-x86_64-unknown-linux-gnu

Okay, looking at this further this is really weird. In our "Load cortex target for no_std build" CI task section in this failing job we call:

        rustup component add --toolchain nightly-2022-04-05 rust-src
        rustup target add thumbv7m-none-eabi --toolchain nightly-2022-04-05
        rustup target add thumbv8m.main-none-eabihf --toolchain nightly-2022-04-05
        rustup target add x86_64-unknown-linux-gnu --toolchain nightly-2022-04-05

Looking closely at the error, I see a couple things wrong:

  • We're installing rust-src for the toolchain version, which seems sensible and correct. rustup is asking us to install it for the toolchain and target, which seems excessive (and a breaking change?)
  • It seems like rustup was looking for the file for 1.61-x86_64-unknown-linux-gnu even though that task is running nightly-2022-04-05, and from looking at the error message it clearly wants nightly-2022-04-05, not 1.61. I don't know what's going on here

Both of these things seem to be Rustup bugs. The +nightly-foo failing on Windows seems like another rustup issue but unrelated.

@kinnison
Copy link
Contributor

If you are doing cross-builds from within invocations of cargo are you perhaps not being sure to clean your environment of things like RUSTC RUSTDOC RUSTFLAGS or other environment variables which might affect builds? This might be the equivalent of #3031 if so.

If I'm barking up the wrong tree, please tell me and we can investigate further.

@Kryptos-FR
Copy link

Kryptos-FR commented Jul 12, 2022

Maybe a different issue but since the release of the new rustup version, it fails on Windows CI jobs on GitHub.

Minimal repro

name: Build (Windows)

on:
  workflow_dispatch:
jobs:
  Windows:
    runs-on: windows-2019
    steps:
      - name: Set up rust
        env:
          RUSTUP_USE_CURL: 1
        run: |
          rustup toolchain install nightly
          rustup +nightly target add i686-pc-windows-gnu
          rustup default nightly-i686-pc-windows-gnu

Errors in log

info: syncing channel updates for 'nightly-x86_64-pc-windows-msvc'
info: latest update on 2022-07-12, rust version 1.64.0-nightly (38b72154d 2022-07-11)
info: downloading component 'cargo'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: installing component 'cargo'
info: installing component 'rust-std'
info: installing component 'rustc'
  nightly-x86_64-pc-windows-msvc installed - rustc 1.64.0-nightly (38b72154d 2022-07-11)
info: checking for self-updates
info: downloading self-update
error: could not copy file from 'C:\Users\runneradmin\.cargo\bin\rustup-init.exe' to 'C:\Users\runneradmin\.cargo\bin\rustup.exe': Access is denied. (os error 5)
ResourceUnavailable: D:\a\_temp\aaf6a12a-e61a-436f-8bde-96f840d0d276.ps1:3
Line |
   3 | rustup +nightly target add i686-pc-windows-gnu
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Program 'rustup.exe' failed to run: An error occurred trying to start process
     | 'C:\Users\runneradmin\.cargo\bin\rustup.exe' with working directory 'D:\a\shards\shards'. Access is
     | denied.At D:\a\_temp\aaf6a12a-e61a-436f-8bde-96f840d0d276.ps1:3 char:1 + rustup +nightly target add
     | i686-pc-windows-gnu + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~.
Error: Process completed with exit code 1.

Workaround

rustup toolchain install --no-self-update nightly

Looks like it is the self update causing havoc.

Note that it only fails on Windows workers. Linux and macOS didn't have that failure.

@kinnison
Copy link
Contributor

That is a problem with Windows locking the file harder. You can add rustup set auto-self-update disable in your CI if you want to prevent this kind of thing.

@Emilgardis
Copy link
Contributor

Emilgardis commented Jul 12, 2022

the windows error is very sporadic, see https://github.com/Emilgardis/rustup-windows-issue/runs/7298752016?check_suite_focus=true

to circumvent, do what @kinnison suggest, example with actions-rs/toolchain (and only doing it on windows since it works well on the unix runners):

- run: rustup set auto-self-update disable
  if: contains(runner.os, 'windows')
  shell: bash
- name: Install Rust toolchain
  uses: actions-rs/toolchain@v1
  with:
    toolchain: stable
    default: true
    profile: minimal

@Manishearth
Copy link
Member

If you are doing cross-builds from within invocations of cargo are you perhaps not being sure to clean your environment of things like RUSTC RUSTDOC RUSTFLAGS or other environment variables which might affect builds? This might be the equivalent of #3031 if so.

Hmm, we're using cargo-make which might be doing that? But cargo make is used a lot by folks, if that's breaking that's probably a problem.

But I don't think cargo-make sets toolchains that way.

@Manishearth
Copy link
Member

unicode-org/icu4x#2170 this confirms it, for whatever reason rustup has decided to use the stable compiler there

@Manishearth
Copy link
Member

Manishearth commented Jul 12, 2022

@kinnison @pietroalbini Given that there are no easy ways to "select" a rustup version at this time, would it be a good idea to roll back the release until we know the scope of this problem and can fix it? As far as ICU4X is concerned we can disable some of these tests if we have to, but I'm worried about the wider impact here, it doesn't seem like we're the only ones with this problem, and it's rather hard to debug since I haven't been able to reproduce it locally (I suspect I will be able to on a fresh system)

@Manishearth
Copy link
Member

Manishearth commented Jul 12, 2022

What we know so far:

#2958 is a part of this

The problem is that when you call cargo make, cargo sets the RUSTC/etc environment variables based on the current toolchain, and then calls cargo-make .... cargo make then calls cargo +nightly build (i.e., rustup run nightly cargo build) or whatever

this ends up with the following invocation: RUSTC=[..]/stable/[..]/rustc cargo +nightly build. Which greatly confuses cargo and leads to very mixed and confusing error messages, since it's trying to use the nightly toolchain, but stable binaries.

Part of the solution here is probably that cargo-make should be clearing rust-relevant environment variables (cc @sagiegurari) since they get passed in by cargo. Especially when cargo-make is doing its own toolchain selection.

But I'm not sure if that's all of it.

@Manishearth
Copy link
Member

from @kinnison

... anything which runs under cargo and expects to invoke cargo on something other than the host toolchain, they should clean up the environment properly before invoking it

@Manishearth
Copy link
Member

Thinking about it more the +nightly-foo issue on windows may also be due to the same kind of bug, if the RUSTC variable ends up with rustc getting called directly instead of te rustup shims

@sagiegurari
Copy link

@sffc @Manishearth from what i remember, cargo-make does not set RUSTC env var.
maybe i don't remember correctly.

however, cargo-make does set the CARGO env var before calling commands that require a specific toolchain.
its actually related to sagiegurari/cargo-make#658 which was reported by @sffc 😄

if a change is needed feel free to open an issue and specify whats wrong with the behavior so i'll know what needs fixing.

@Manishearth
Copy link
Member

@sagiegurari no, the problem is that when you run cargo make, what happens is that cargo is run (under the default toolchain), which then calls cargo-make with the environment variables set, and cargo-make doesn't unset them before calling things further down

@Manishearth
Copy link
Member

That may not be the only problem however. cargo-make setting CARGO when needed also seems correct (maybe it should be setting more things?)

@sagiegurari
Copy link

question, rustup manages the rust installations. doesn't it override any existing env var to satisfy the command line request?

i could unset stuff but later cargo will set more env vars which in turn mess up rustup again and its all internal rust implementation.

so it feels like something that rustup needs to manage maybe? does it make sense?

unsetting those 3 env vars is easy and can be worked around now in the makefile env block of any project using the unset capability.
so that could be used to test it solves the issue and if so i got no problem pushing it to the master.
but, it might break again in the future if cargo changes something.

@Manishearth
Copy link
Member

@sagiegurari I don't think that's the problem. This is in part cargo behavior, and how cargo calls custom subcommands.

In general I think the following can be taken as an authoritative statement on what the rustup team's intent is here:

from @kinnison

... anything which runs under cargo and expects to invoke cargo on something other than the host toolchain, they should clean up the environment properly before invoking it

So I don't think "things changing later" is a real problem if the team commits to (and documents) this.

unsetting those 3 env vars is easy and can be worked around now in the makefile env block of any project using the unset capability.

Going to try this!

@Manishearth
Copy link
Member

I suspect on cargo-make's side, the answer is that it should unset RUSTC and RUSTDOC, and it should either unset CARGO or set it to a toolchain, if specified.

@simbleau
Copy link

I think this is relevant #3025

As far as CI goes, trying to update the toolchain on nightly is broken at least (Ubuntu 22.04).

I think this can help narrow the problem:
simbleau/website@bac36b2

This caused my pipeline to pass.

And locally, I couldn't install the newest nightly until I did a rustup uninstall and reinstalled nightly, it worked.

Somewhere, somehow, someone pushed a bad version, and the only solution is to uninstall and reinstall.

@Manishearth
Copy link
Member

(note that rustup updates are done via rustup self update)

@kinnison
Copy link
Contributor

We need, at some point, to start to resolve this problem in a way which permits us to give the performance boost we were trying to achieve; without breaking your CI

@Manishearth
Copy link
Member

I mean, we're also happy to make changes on our end, if cargo-make can be updated to handle this and stuff it's also fine.

Such CI tools are popular but I don't think there are actually all too many of them.

@Emilgardis
Copy link
Contributor

if a cargo subcommand is currently doing from it's own process cargo +<toolchain> <command> is the way to resolve when it's fixed: rustup run <toolchain> cargo <command> instead?

@sagiegurari
Copy link

if cargo-make can be updated to handle this and stuff it's also fine

@Manishearth I can unset these 3 env vars by default at the start of cargo-make. that's easy.
that means cargo setting them would be ignored in a way.

@Manishearth
Copy link
Member

@kinnison btw, mind changing this issue title to something like "new rustup version breaks with error: no such subcommand: +nightly-2022-04-05`" (unfortunately I don't have edit access to this repo anymore)

@sffc sffc changed the title rustup update broke our CI rustup breaks with cargo-make due to environment variables Aug 29, 2022
@sffc
Copy link
Author

sffc commented Aug 29, 2022

(title edited)

@rami3l
Copy link
Member

rami3l commented Apr 25, 2024

A gentle ping: cargo-make has made sagiegurari/cargo-make#1060 now, does this issue still exist since?

@wmmc88
Copy link

wmmc88 commented Apr 25, 2024

From the cargo-make side, this issue was worked around and resolved, but im unsure if rustup want to leave this open to address @kinnison comments:

#3029 (comment)

@rami3l
Copy link
Member

rami3l commented Jul 10, 2024

From the cargo-make side, this issue was worked around and resolved, but im unsure if rustup want to leave this open to address @kinnison comments:

#3029 (comment)

Thanks! 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

9 participants