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

miri-script: use --remap-path-prefix to print errors relative to the right root #3798

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 9, 2024

Inspired by rust-lang/rust-clippy#13232, this makes it so that when cargo-miri fails to build, ./miri check will print errors with paths like cargo-miri/src/setup.rs. That means we can get rid of the miri-symlink-hacks and instead tell RA to just always invoke the ./miri clippy script just once, in the root.

This means that we can no longer share a target dir between cargo-miri and miri as the RUSTFLAGS are different to crates that are shared in the dependency tree need to be built twice with two different flags. miri-script hence now has to set the MIRI environment variable to tell the cargo miri setup invocation where to find Miri.

I also made it so that errors in miri-script itself are properly shown in RA, for which the ./miri shell wrapper needs to set the right flags.

@RalfJung

This comment was marked as resolved.

miri Show resolved Hide resolved
@RalfJung RalfJung force-pushed the miri-script-remap-path-prefix branch 2 times, most recently from 19de1a7 to 9e21767 Compare August 10, 2024 09:48
@RalfJung
Copy link
Member Author

All right, this seems to be working now.

@rust-lang/miri what do you think?
Also note that when this lands, you'll have to update your RA config.

@RalfJung
Copy link
Member Author

Oh, looking at the Miri thing again we could also use a totally different approach... namely, to set this as a per-package variable in the profile, which will also avoid applying it to dependencies.

@RalfJung RalfJung force-pushed the miri-script-remap-path-prefix branch 2 times, most recently from d8e7771 to 116d2de Compare August 10, 2024 10:47
@RalfJung RalfJung force-pushed the miri-script-remap-path-prefix branch from 45ec272 to d3fb2b8 Compare August 10, 2024 10:59
Comment on lines +38 to +40
- name: Install nightly toolchain
run: rustup toolchain install nightly --profile minimal
shell: bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we now need this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose more than anything I'm confused that we didn't need this before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now need this because miri-script needs nightly because it picks up the .config/cargo.toml which uses nightly features.

Previously we didn't need this because miri-script was built with stable, and then it used rustup-toolchain-install-master to install the toolchain used for Miri.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.

@saethlin
Copy link
Member

@rust-lang/miri what do you think?

No objections. I trip over the path prefixes more in the rust-lang/rust tree than this repo, but still a nice change.

@RalfJung
Copy link
Member Author

Okay let's land this then. :)
@bors r=saethlin

@bors
Copy link
Collaborator

bors commented Aug 12, 2024

📌 Commit b9ebd52 has been approved by saethlin

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 12, 2024

⌛ Testing commit b9ebd52 with merge c73f658...

@bors
Copy link
Collaborator

bors commented Aug 12, 2024

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing c73f658 to master...

@bors bors merged commit c73f658 into rust-lang:master Aug 12, 2024
8 checks passed
@RalfJung RalfJung deleted the miri-script-remap-path-prefix branch August 13, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants