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

[cargo-miri] Don't skip rlib crates #1709

Merged
merged 3 commits into from Feb 14, 2021
Merged

[cargo-miri] Don't skip rlib crates #1709

merged 3 commits into from Feb 14, 2021

Conversation

ghost
Copy link

@ghost ghost commented Feb 14, 2021

Fixes #1691.

@@ -3,5 +3,5 @@
/// assert!(cargo_miri_test::make_true());
/// ```
pub fn make_true() -> bool {
true
rlib_dep::use_me()
Copy link
Author

Choose a reason for hiding this comment

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

Confirmed that current nightly cargo-miri fails here:

## Running `cargo miri` tests
A libstd for Miri is now available in `/my/home/directory/.cache/miri/HOST`.
Testing `cargo miri run` (no isolation)...
--- BEGIN stdout ---
--- END stdout ---
--- BEGIN stderr ---
error: extern location for rlib_dep does not exist: /my/home/directory/miri/test-cargo-miri/target/x86_64-unknown-linux-gnu/debug/deps/librlib_dep-095fd0061894c932.rmeta
 --> src/lib.rs:6:5
  |
6 |     rlib_dep::use_me()
  |     ^^^^^^^^

error: aborting due to previous error

error: could not compile `cargo-miri-test`

To learn more, run the command again with --verbose.
--- END stderr ---

TEST FAIL: exit code was 101

Comment on lines -601 to +605
match get_arg_flag_value("--crate-type").as_deref() {
Some("rlib") | Some("cdylib") => {
if verbose {
eprint!("[cargo-miri rustc] (rlib/cdylib skipped)");
}
return;
if get_arg_flag_value("--crate-type").as_deref() == Some("cdylib") {
if verbose {
eprint!("[cargo-miri rustc] (cdylib skipped)");
}
_ => {},
return;
Copy link
Author

@ghost ghost Feb 14, 2021

Choose a reason for hiding this comment

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

I found that skipping the crate if get_arg_flag_value("--crate-type") is something is probably wrong, because Cargo passes multiple --crate-type to the compiler.

If I add

issue_1567::use_the_dependency();

to test-cargo-miri/src/lib.rs, the test will fail:

## Running `cargo miri` tests
A libstd for Miri is now available in `/my/home/directory/.cache/miri/HOST`.
Testing `cargo miri run` (no isolation)...
--- BEGIN stdout ---
--- END stdout ---
--- BEGIN stderr ---
error: extern location for issue_1567 does not exist: /my/home/directory/miri/test-cargo-miri/target/x86_64-unknown-linux-gnu/debug/deps/libissue_1567.rmeta
 --> src/lib.rs:6:5
  |
6 |     issue_1567::use_the_dependency();
  |     ^^^^^^^^^^

error: aborting due to previous error

error: could not compile `cargo-miri-test`

To learn more, run the command again with --verbose.
--- END stderr ---

TEST FAIL: exit code was 101

I think that is because Cargo passes both --crate-type cdylib and --crate-type rlib to the compiler and only invokes the compiler once. If cargo-miri skips the crate here, the rlib won't be produced.

Maybe just dropping --crate-type cdylib and continuing to compile with only --crate-type rlib (or skip the compilation if there's no more --crate-type) makes more sense.

(EDIT: Hit #1705 when experimenting with that... Trying to fix that together...)

Copy link
Member

Choose a reason for hiding this comment

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

(EDIT: Hit #1705 when experimenting with that... Trying to fix that together...)

So I suggest we tackle that in a future MR. Good catch though, this should be added to the test suite then (together with a crate-type = ["lib", "staticlib", "cdylib"] crate that triggers this behavior).

@RalfJung
Copy link
Member

Thanks for the PR. :)

@RalfJung
Copy link
Member

Looking good, thanks for your help in working out the kinks of cargo-miri!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 14, 2021

📌 Commit 0737de8 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Feb 14, 2021

⌛ Testing commit 0737de8 with merge c3f7069...

@bors
Copy link
Contributor

bors commented Feb 14, 2021

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

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.

Cannot compile a crate with a crate-type = ["rlib"] dependency
2 participants