Skip to content

compiletest: call cargo-miri directly rather than via 'cargo run' #3318

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

Merged
merged 1 commit into from
Feb 24, 2024
Merged

compiletest: call cargo-miri directly rather than via 'cargo run' #3318

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions tests/compiletest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,15 @@ fn test_config(target: &str, path: &str, mode: Mode, with_dependencies: bool) ->
};

if with_dependencies {
// Set the `cargo-miri` binary, which we expect to be in the same folder as the `miri` binary.
// (It's a separate crate, so we don't get an env var from cargo.)
let mut prog = miri_path();
prog.set_file_name("cargo-miri");
Copy link
Member

Choose a reason for hiding this comment

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

This needs the .exe extension on Windows, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it?

I guess CI will tell...

Copy link
Contributor

@RossSmyth RossSmyth Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

".exe" is in the PATHEXT env var so it should just execute (that's how my miri.bat file works). Though since this is executed in bash on msys2 I'm not sure if that applies or not as I do not use that. But CI seems to be doing something.

Copy link
Member

Choose a reason for hiding this comment

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

I thought PATHEXT only applied to cmd.exe and ShellExecute and not CreateProcess. std::process::Command uses CreateProcessW. The documentation of CreateProcessW doesn't mention whether or not it does apply

Copy link
Member Author

Choose a reason for hiding this comment

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

Windows CI seems to be working so it does seem to add the .exe. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Command, for security reasons, uses its own logic for finding the exe and for compatibility reasons it always assumes .exe was intended if no extension is present.

Copy link
Member Author

@RalfJung RalfJung Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ChrisDenton do you magically show up when someone says the word "Windows" in a Rust repo? ;)

Anyway, thanks for the explanation! :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but only if the proper rituals are observed

config.dependency_builder.program = prog;
let builder_args = ["miri", "run"]; // There is no `cargo miri build` so we just use `cargo miri run`.
config.dependency_builder.args = builder_args.into_iter().map(Into::into).collect();
config.dependencies_crate_manifest_path =
Some(Path::new("test_dependencies").join("Cargo.toml"));
let mut builder_args = vec!["run".into()];
builder_args.extend(flagsplit(&env::var("CARGO_EXTRA_FLAGS").unwrap_or_default()));
builder_args.extend([
"--manifest-path".into(),
"cargo-miri/Cargo.toml".into(),
"--".into(),
"miri".into(),
"run".into(), // There is no `cargo miri build` so we just use `cargo miri run`.
]);
config.dependency_builder.args = builder_args.into_iter().map(Into::into).collect();
// Reset `RUSTFLAGS` to work around <https://github.com/rust-lang/rust/pull/119574#issuecomment-1876878344>.
config.dependency_builder.envs.push(("RUSTFLAGS".into(), None));
}
Expand Down