-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use target.rustflags
instead of build.rustflags
#57
Conversation
a3fd32b
to
8dbe52d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the new tests failed on my system (see accompanying comment), but otherwise the PR works as designed. Thanks a lot!
@@ -296,6 +296,35 @@ rustflags = ["-Ctarget-cpu=native"] | |||
"#, | |||
); | |||
|
|||
let output = project.cmd(&["build", "--", "-v"]).run()?; | |||
println!("{}", output.stderr()); | |||
assert!(!output.stderr().contains("-Ctarget-cpu=native")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very minor note, but this assumes the tests run on a system that hasn't set -Ctarget-cpu=native
on a matching target.rustflags
... which, of course, mine has, causing the test to fail.
Suggestion:
diff --git a/tests/integration/pgo.rs b/tests/integration/pgo.rs
index a60e19828f27..3a33ee5f8ca0 100644
--- a/tests/integration/pgo.rs
+++ b/tests/integration/pgo.rs
@@ -292,13 +292,13 @@ fn test_override_build_rustflags_from_config() -> anyhow::Result<()> {
".cargo/config.toml",
r#"
[build]
-rustflags = ["-Ctarget-cpu=native"]
+rustflags = ["-Ctarget-cpu=imaginary"]
"#,
);
let output = project.cmd(&["build", "--", "-v"]).run()?;
println!("{}", output.stderr());
- assert!(!output.stderr().contains("-Ctarget-cpu=native"));
+ assert!(!output.stderr().contains("-Ctarget-cpu=imaginary"));
assert!(output.stderr().contains("-Cprofile-generate"));
output.assert_ok();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work, the test would fail because cargo does not know the imaginary
target. It would be nice to disallow Cargo to load external config files, but I haven't found any way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, i tested it (both with and without my local ~/.cargo/config.toml
), and it works. The imaginary
target is removed before it's sent to rustc (and AFAIK, and the test seems to confirm, cargo doesn't check it, only rustc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it failed with -Ctarget-cpu=foo
on my machine, by Rust saying that it doesn't know the foo
target (as expected).
Aah, I see now, I replaced it everywhere, it should have only been done for this single test. Sorry.
That's a good idea to add a CLI flag for allowing swapping between build/target rustflags! It can be implemented later though, I'd like to merge this as is, to let people try it. |
Fixes: #56