Apply RUSTFLAGS arguments to rustc builds

[brson]

Cargo will use RUSTFLAGS for building everything that is not a build script
or plugin. It does not apply to these targets because they may be for
a different platform that 'normal' builds.

Closes #2112

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[brson]

r? @alexcrichton

[alexcrichton]

Cargo may want to start tracking at a finer grain of why dependencies are being compiled the way they are. For example if a plugin has a dependency then the dependency will be compiled for the host architecture, but it'll have RUSTFLAGS applied to it even when the plugin doesn't. I would personally expect that the entire subgraphs of dependencies only used by plugins wouldn't get RUSTFLAGS treatment.

Also, could this have a few acceptance tests as well? e.g. fails-to-compile unless RUSTFLAGS is parsed properly.

[brson]

I'll try to figure out how to propagate the plugin-ness to dependencies.

re tests there is no error case in cargo's parsing of RUSTFLAGS. It only splits on space. Are there other tests you would like to see?

[brson]

I've got a patch that makes plugin deps not use RUSTFLAGS, but have discovered the build scripts have this same problem.

[alexcrichton]

It may actually be easier to just check kind to see whether the custom flags apply. Custom flags would apply to Kind::Target unconditionally and then it would also apply to Kind::Host if host == target.

That does mean that plugins and build scripts will get custom flags for a vanilla cargo build, but that's probably ok?

[Zoxc]

RUSTFLAGS should allow passing command line arguments which include spaces. I want to use this to pass --sysroot to rustc.

[alexcrichton]

One other test I thought of recently to add is to ensure that changing RUSTFLAGS will trigger recompliations.

[brson]

@alexcrichton I updated the patch to make the logic much simpler. What I've got is:

1. If --target is not specified, RUSTFLAGS applies to all builds.
2. If --target is specified, RUSTFLAGS only applies to builds
   with the Kind::Target target kind, which indicates build units
   derived from the requested --target.

This doesn't seem to capture your "it would also apply to Kind::Host if host == target" suggestion, but still all my test cases pass. Can you think of a test case that requires your suggested rule?

I'll look at the recompilation and parsing matters now.

[alexcrichton]

Looks good to me! The recompilation will probably come about by modifying this structure .

[brson]

@alexcrichton I've added a patch to recompile when RUSTFLAGS changes.

@Zoxc I'm not sure how to make RUSTFLAGS support spaces. I looked for precedent with CFLAGS and couldn't find it. Simple escaping with \ would make passing windows paths pretty terrible - everyone would need to know to escape RUSTFLAGS all the time.

[alexcrichton]

Could this use the methods on the context instead? That way if you use RUSTFLAGS with --target you won't recompile any build scripts or plugins (as they're not affected by this env var anyway)

[alexcrichton]

This is fine to leave as empty because running the build script itself (what this fingerprint is for) doesn't actually depend on RUSTFLAGS. Compilation of the build script should be covered by the fingerprint above I believe

[alexcrichton]

Could you also add a test for if RUSTFLAGS changes that a rebuild happens? Also if RUSTFLAGS doesn't change a rebuild doesn't spuriously happen

[Zoxc]

@brson Maybe Pascal style escaping with " is better? or use another escape charater than \

[eddyb]

I recommend shell-like quoting (e.g. bash "..." and '...'), with no $ or other such special cases.

If no semantics attached to \ are desired, then you can have both "..." and '...', with the former obeying the same simple rules as the latter.
You can then escape a'b by using "a'b" and x"y by using 'x"y', and it's still a proper subset of bash quote handling.

[bors]

☔️ The latest upstream changes (presumably #2406) made this pull request unmergeable. Please resolve the merge conflicts.

[brson]

I've added another commit that also draws rustflags from the config file according to the following scheme:

build.rustflags is treated exactly like RUSTFLAGS.

target.rustflags only applies when --target is not specified.

target.$triple.rustflags only applies when `--target is
specified and the target matches.

RUSTFLAGS takes precedent, then build.rustflags, then target.

All are lists, so argument lists with spaces work. None of these allow extra flags to be passed to build scripts and plugins when --target is specified. We probably shouldn't land all these options.

Edit: it's important to note that because the config options are lists, there aren't corresponding CARGO_* env vars, only RUSTFLAGS.

[alexcrichton]

This may want a filter_map for "not empty" for environment variables like " -l foo --sysroot bar"

[alexcrichton]

Out of curiosity, was this updated required to get the patch to land? Or was this just a routine updated?

[brson]

I don't recall.

[alexcrichton]

Looks like some tests on travis are failing, but they probably just need a if !::is_nightly() { return } gate as they're using unstable features.

I'm curious what the difference between build.rustflags and target.rustflags are, they seem roughly equivalent right? Otherwise target.$triple.rustflags seems somewhat reasonable to me, but I'd also be fine starting out with just build.rustflags and RUSTFLAGS.

[brson]

I'm curious what the difference between build.rustflags and target.rustflags are, they seem roughly equivalent right

They are almost the same. The difference is that build.rustflags applies regardless of whether --target is specified whereas target.rustflags only applies when --target is not specified.

[brson]

I've removed the target.* options and filtered out empty strings.

[brson]

I did have to cargo update again today to get around a build error.

[brson]

This was the build error

/home/brian/.cargo/registry/src/github.com-88ac128001ac3a9a/libgit2-sys-0.4.0/lib.rs:539:9: 539:30 error: unresolved import `git_status_show_t::*`. Not a modu
le `git_status_show_t` [E0432]
/home/brian/.cargo/registry/src/github.com-88ac128001ac3a9a/libgit2-sys-0.4.0/lib.rs:539 pub use git_status_show_t::*;
                                                                                                 ^~~~~~~~~~~~~~~~~~~~~
/home/brian/.cargo/registry/src/github.com-88ac128001ac3a9a/libgit2-sys-0.4.0/lib.rs:539:9: 539:30 help: run `rustc --explain E0432` to see a detailed explana
tion

[alexcrichton]

@bors: r+ 2f01868

I am excite!

[bors]

⌛️ Testing commit 2f01868 with merge 9208212...

[alexcrichton]

@bors: retry force clean

[bors]

⌛️ Testing commit 2f01868 with merge 8c60ada...

[bors]

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64