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.

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

Jump to bottom

miri script: build with stable toolchain #3402

Merged
merged 3 commits into from
Mar 24, 2024
Merged

miri script: build with stable toolchain #3402

merged 3 commits into from
Mar 24, 2024

Conversation

RalfJung
Copy link
Member

./miri toolchain sets up a rustup override miri. But then if something goes wrong and the miri toolchain doesn't work, one can't even run ./miri toolchain again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit 5f3c8f8 has been approved by RalfJung

It is now in the queue for this repository.

bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=RalfJung
b351a69 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 5f3c8f8 with merge b351a69...

@RalfJung
Copy link
Member Author

Why would this PR cause that test to fail on Windows...?!?

  $ bash -c "cargo miri run --locked --manifest-path \"D:\\a\\miri\\miri\\bench-cargo-miri\\backtraces\\Cargo.toml\""
  Windows Subsystem for Linux has no installed distributions.
  
  Distributions can be installed by visiting the Microsoft Store:
  
  https://aka.ms/wslstore
  
  Error: command exited with non-zero code `bash -c "cargo miri run --locked --manifest-path \"D:\\a\\miri\\miri\\bench-cargo-miri\\backtraces\\Cargo.toml\""`: 1

We are running our entire CI in bash.^^ But now somehow this one bash call is trying to invoke WSL?!?
Cc @ChrisDenton

@ChrisDenton
Copy link
Member

That could happen if it's picking up the bash in the system directory.

Maybe use $BASH instead?

@bors
Copy link
Contributor

bors commented Mar 24, 2024

💔 Test failed - checks-actions

@RalfJung
Copy link
Member Author

RalfJung commented Mar 24, 2024 •
edited
Loading

I'll try that, thanks. But why does adding +stable change anything? are we getting an old version of Rust where Command behaved differently?

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit dec883b has been approved by RalfJung

It is now in the queue for this repository.

bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=RalfJung
ddfe041 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit dec883b with merge ddfe041...

@ChrisDenton
Copy link
Member

I'll try that, thanks. But why does adding +stable change anything? are we getting an old version of Rust where Command behaved differently?

Command on Windows has some weirdly inconsistent behaviour depending on if Command::env is used or not. The short version is that in Rust's early history, an incomplete attempt was made to emulate Linux behaviour and over time this code rotted further as people refactored the code for modern idioms (lack of testing didn't help). I've improved the code since I started helping to maintain std but actually fixing the behaviour needs a decision on what exactly the new behaviour should be.

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit 951fd76 has been approved by RalfJung

It is now in the queue for this repository.

bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=RalfJung
920eab3 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 951fd76 with merge 920eab3...

@RalfJung
Copy link
Member Author

RalfJung commented Mar 24, 2024 •
edited
Loading

What is even going on?

   + HYPERFINE='/bin/bash -c'
  + ./miri bench
  $ cargo +miri install --locked --path D:\a\miri\miri --force --root C:\Users\runneradmin\.rustup\toolchains\miri
    Installing miri v0.1.0 (D:\a\miri\miri)
      Updating crates.io index
      Finished `release` profile [optimized] target(s) in 0.35s
     Replacing C:\Users\runneradmin\.rustup\toolchains\miri\bin\miri.exe
      Replaced package `miri v0.1.0 (D:\a\miri\miri)` with `miri v0.1.0 (D:\a\miri\miri)` (executable `miri.exe`)
  $ cargo +miri install --locked --path D:\a\miri\miri\cargo-miri --force --root C:\Users\runneradmin\.rustup\toolchains\miri
    Installing cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)
      Updating crates.io index
      Finished `release` profile [optimized] target(s) in 0.31s
     Replacing C:\Users\runneradmin\.rustup\toolchains\miri\bin\cargo-miri.exe
      Replaced package `cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)` with `cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)` (executable `cargo-miri.exe`)
  $ C:/Program Files/Git/usr/bin/bash -c "cargo miri run --locked --manifest-path \"D:\\a\\miri\\miri\\bench-cargo-miri\\backtraces\\Cargo.toml\""
  Error: command not found: `C:/Program`

So here, "$BASH" expands to /bin/bash but then magically that becomes C:/Program Files/Git/usr/bin/bash which causes a problem because of the space. So I use "'$BASH'" instead but now the magic transformation does not happen and it tries to run /bin/bash which does not exist:

  + HYPERFINE=''\''/bin/bash'\'' -c'
  + ./miri bench
  $ cargo +miri install --locked --path D:\a\miri\miri --force --root C:\Users\runneradmin\.rustup\toolchains\miri
    Installing miri v0.1.0 (D:\a\miri\miri)
      Updating crates.io index
      Finished `release` profile [optimized] target(s) in 0.53s
     Replacing C:\Users\runneradmin\.rustup\toolchains\miri\bin\miri.exe
      Replaced package `miri v0.1.0 (D:\a\miri\miri)` with `miri v0.1.0 (D:\a\miri\miri)` (executable `miri.exe`)
  $ cargo +miri install --locked --path D:\a\miri\miri\cargo-miri --force --root C:\Users\runneradmin\.rustup\toolchains\miri
    Installing cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)
      Updating crates.io index
      Finished `release` profile [optimized] target(s) in 0.19s
     Replacing C:\Users\runneradmin\.rustup\toolchains\miri\bin\cargo-miri.exe
      Replaced package `cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)` with `cargo-miri v0.1.0 (D:\a\miri\miri\cargo-miri)` (executable `cargo-miri.exe`)
  $ /bin/bash -c "cargo miri run --locked --manifest-path \"D:\\a\\miri\\miri\\bench-cargo-miri\\backtraces\\Cargo.toml\""
  Error: command not found: `/bin/bash`

@RalfJung RalfJung force-pushed the miri-script branch 3 times, most recently from 5a389cd to 4d1e030 Compare March 24, 2024 13:06
@RalfJung
Copy link
Member Author

Looks like they are using 1.76. Let's see if 1.77 fixes this... after all we've been using miri-script for a while now (but with nightly Rust) and it always worked on Windows.

@bors r+

@bors

This comment was marked as outdated.

Sign in to view
@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit 916ff6e has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 916ff6e with merge 9f142d3...

bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=
9f142d3 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=RalfJung
0d13bab 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 916ff6e with merge 0d13bab...

@RalfJung
Copy link
Member Author

Nope, latest stable is still affected.
We may just have to disable this test on Windows for now...

@RalfJung
Copy link
Member Author

RalfJung commented Mar 24, 2024 •
edited
Loading

Hm, something doesn't make sense. We've been using this for at least 7 months now. Whatever was the nightly back then, has long reached stable.

So this can't be just a difference between Rust stable vs nightly. It has to be that somehow adding the +stable itself is the problem...?!? But either way we are running the binary directly, +stable is only present for the build...

@RalfJung
Copy link
Member Author

Well, hard-coding the path seems to work. 🤷

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit cd776e2 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit cd776e2 with merge dded247...

bors added a commit that referenced this pull request Mar 24, 2024
@bors
Auto merge of #3402 - RalfJung:miri-script, r=RalfJung
dded247 
miri script: build with stable toolchain

`./miri toolchain` sets up a `rustup override miri`. But then if something goes wrong and the `miri` toolchain doesn't work, one can't even run `./miri toolchain` again as building miri-script needs a toolchain...

So let's always use stable to build miri-script, making it override-independent. I assume everyone will have that installed.
@RalfJung
Copy link
Member Author

Yeah something is definitely patching the env var -- but only if it is not surrounded by ' or ", which leads to very bad behavior. What a terrible hack, and I don't even know where it lives.

2024-03-24T14:59:55.2091474Z + HYPERFINE='/bin/bash -c'
2024-03-24T14:59:55.2091893Z + ./miri bench
2024-03-24T14:59:55.2092838Z ##[group]Testing host architecture
2024-03-24T14:59:55.3781009Z [src\commands.rs:399:9] program_name = "C:/Program"
2024-03-24T14:59:55.3781848Z [src\commands.rs:399:9] args = [
2024-03-24T14:59:55.3782357Z     "Files/Git/usr/bin/bash",
2024-03-24T14:59:55.3782915Z     "-c",
2024-03-24T14:59:55.3783142Z ]

@RalfJung
Copy link
Member Author

Seems to be this functionality that reportedly is specific to Git-for-Windows.

What remains completely unclear is why adding the +stable changed anything...

@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Mar 24, 2024

📌 Commit 1de44d0 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 24, 2024

⌛ Testing commit 1de44d0 with merge 6a72dc6...

@bors
Copy link
Contributor

bors commented Mar 24, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 6a72dc6 to master...

@bors bors merged commit 6a72dc6 into rust-lang:master Mar 24, 2024
8 checks passed
@RalfJung RalfJung deleted the miri-script branch March 24, 2024 18:28
@DeSevilla DeSevilla mentioned this pull request Sep 6, 2024
Closed
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RalfJung @bors @ChrisDenton