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

Using build.rustflags is awkward right now #3052

Closed
CamJN opened this issue Aug 28, 2016 · 9 comments
Closed

Using build.rustflags is awkward right now #3052

CamJN opened this issue Aug 28, 2016 · 9 comments
Labels
E-easy Experience: Easy

Comments

@CamJN
Copy link

CamJN commented Aug 28, 2016

Based on #2535 and rust-lang/rust#23238 and rust-lang/rust#13841. I thought I could add

[build]
rustflags = ["-C target-cpu=native"]

to my ~/.cargo/config file to pass that flag to my builds. But that breaks everything. Turns out I have to use ["-C", "target-cpu=native"] which is weird but it works, so w/e. However when I run cargo build --verbose I noticed that the -C target-cpu=native flag gets passed to rustc twice in immediate succession. ie: rustc src/main.rs --crate-name min --crate-type bin -g --out-dir /Users/camdennarzt/Developer/Rust/min/target/debug --emit=dep-info,link -L dependency=/Users/camdennarzt/Developer/Rust/min/target/debug -L dependency=/Users/camdennarzt/Developer/Rust/min/target/debug/deps -C target-cpu=native -C target-cpu=native

rustc: 1.11.0
cargo: 0.11.0 (259324c 2016-05-20)
uname -a: Darwin SECUR-T.local 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64

@nagisa
Copy link
Member

nagisa commented Aug 28, 2016

"-C target-cpu=native" is a single argument (i.e. there’s no shell which does argument splitting for you), so what the target binary really receives is not "rustc" "-C" "target-cpu=native" but "rustc" "-C target-cpu=native" which is completely different both behaviour and semantics wise.

Alas, you cannot do such splitting either, because that would preclude people from passing over stuff (i.e. filenames) with spaces in it.

@CamJN
Copy link
Author

CamJN commented Aug 28, 2016

I dunno, I would expect that the command is simply concatenated together and passed to the shell, as that's less work (a huge priority of cargo's) and more flexible. You can pass files with spaces in them the same way as people have done for my entire life, by escaping the space with \.

Either way I think we can agree that passing the flag twice is wrong.

@alexcrichton
Copy link
Member

@CamJN did you happen to also have RUSTFLAGS or CARGO_BUILD_RUSTFLAGS environment variables set at the time? Passing twice definitely seems wrong, and I couldn't reproduce locally unfortunately.

Also we can and should accept configuration like:

[build]
rustflags = "-C target-cpu=native"

99% of the time splitting on spaces is all you want, and the list ([]-syntax) is there if you need to pass an argument with spaces in it.

@CamJN
Copy link
Author

CamJN commented Aug 30, 2016

@alexcrichton nope, both RUSTFLAGS and CARGO_BUILD_RUSTFLAGS are unset.

A repo which exhibits the behavior: https://github.com/CamJN/min
And my ~/.cargo/config file:

[target.x86_64-apple-darwin.openssl-sys]
rustc-link-search = [ "/usr/local/opt/openssl/lib" ]
rustc-link-lib = [ "ssl", "crypto" ]
include = "/usr/local/opt/openssl/include"

[profile.release]
panic = 'abort'

[build]
rustflags = ["-C", "target-cpu=native"]

@alexcrichton
Copy link
Member

Weird! In any case I'm going to tag this as an easy bug as it should be pretty easy to knock out. There's basically a point where we call get_list or something like that for the build.rustflags configuration key, and we just need to also call get_string (or similar) in the error case. The string version would then split on spaces.

@jhbabon
Copy link
Contributor

jhbabon commented Sep 9, 2016

If the config with the rustflags is in the home directory maybe the problem is the same as in this issue #3070

@CamJN
Copy link
Author

CamJN commented Sep 9, 2016

Yup, that's the same issue.

bors added a commit that referenced this issue Dec 2, 2016
Implemented string lookup for `build.rustflags` config key

This addresses the immediate issue described in #3052 .
I am, however, unsure about the current state of the deeper issues mentioned in that issue, but if needed, I can take stab at them as well. :)
@dgellow
Copy link
Contributor

dgellow commented Jan 25, 2017

Is this still an issue? I can see that the PR #3356 has been merged

@alexcrichton
Copy link
Member

Indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Experience: Easy
Projects
None yet
Development

No branches or pull requests

5 participants