Skip to content

Use miri inside the target directory used by rustc as Miri's target directory #1842

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

Merged
merged 12 commits into from Jul 3, 2021
Merged

Use miri inside the target directory used by rustc as Miri's target directory #1842

merged 12 commits into from Jul 3, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jun 24, 2021

Resolves #1311.

This PR makes Miri use miri inside the rustc target directory as its target directory, by letting cargo-miri get the rustc target directory by calling cargo metadata, append miri to it, and pass it with --target-dir to Cargo.

Getting the rustc target directory accurately requires calling cargo metadata as far as I know, because the target-dir can be set in config files in various places that are hard for cargo-miri to find.

I also considered https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#custom-named-profiles, but it looks like that requires adding cargo-features = ["named-profiles"] to Cargo.toml, which would be tricky for cargo-miri:

$ cargo +nightly-2021-06-23 test --config 'profile.miri.inherits="release"' --profile=miri -Z named-profiles -Z unstable-options
error: config profile `miri` is not valid (defined in `--config cli option`)

Caused by:
  feature `named-profiles` is required

  consider adding `cargo-features = ["named-profiles"]` to the manifest

@RalfJung
Copy link
Member

Getting the rustc target directory accurately requires calling cargo metadata as far as I know, because the target-dir can be set in config files in various places that are hard for cargo-miri to find.

RLS does something here, did you check what it is the do?
This seems to me that code (but the link is old, the code might have changed since then).

@ghost
Copy link
Author

ghost commented Jun 26, 2021

RLS does something here, did you check what it is the do?
This seems to me that code (but the link is old, the code might have changed since then).

It is using Cargo as a git or path dependency. It's not using cargo metadata, but that strategy looks a bit complex to adopt in cargo-miri. Also, it will add 84 new transitive dependencies...

@RalfJung
Copy link
Member

It is using Cargo as a git or path dependency. It's not using cargo metadata, but that strategy looks a bit complex to adopt in cargo-miri. Also, it will add 84 new transitive dependencies...

Yeah I agree your approach works better for Miri. Thanks for checking!

@ghost
Copy link
Author

ghost commented Jun 27, 2021

RLS does something here, did you check what it is the do?
This seems to me that code (but the link is old, the code might have changed since then).

It is using Cargo as a git or path dependency. It's not using cargo metadata, but that strategy looks a bit complex to adopt in cargo-miri. Also, it will add 84 new transitive dependencies...

(Another option is to use Cargo from crates.io. I assume that's not viable, because in the rust-lang/rust repository, it's not allowed to have two versions of cargo (RLS is using the in-tree version), and it's not in PERMITTED_DEPENDENCIES.)

@RalfJung
Copy link
Member

Also cargo is a huge dependency, I much prefer what you did. :)
(In fact we used to use cargo metadata for other stuff in the past. I was happy when we got rid of it but this use makes sense.)

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2021

Looking great, thanks a lot for taking care of this long-standing issue. :)
@bors r+

@bors
Copy link
Contributor

bors commented Jul 3, 2021

📌 Commit e3fca9b has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jul 3, 2021

⌛ Testing commit e3fca9b with merge 6a18683...

@bors
Copy link
Contributor

bors commented Jul 3, 2021

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

@bors bors merged commit 6a18683 into rust-lang:master Jul 3, 2021
@ghost ghost deleted the target-dir branch July 3, 2021 10:09
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2021
update miri

Let's get rust-lang/miri#1842 shipped. :)
Also fixes rust-lang#86863
Cc `@rust-lang/miri` r? `@ghost`
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.

miri test is painful to use with CARGO_TARGET_DIR set
2 participants