From 4f41727106933b9d0e7427da5af24db98725ef01 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Wed, 16 Feb 2022 15:35:53 -0800 Subject: [PATCH] Enable propagating rustflags to build scripts This patch implements more complete logic for applying rustflags to build scripts and other host artifacts. In the default configuration, it only makes build scripts (and plugins and whatnot) pick up on `rustflags` from `[host]`, which fixes #10206 but otherwise preserves existing behavior in all its inconsistent glory. The same is the case if `target-applies-to-host` is explicitly set to `false`. When `target-applies-to-host` is explicitly set to `true`, rustflags will start to be applied in the same way that `linker` and `runner` are today -- namely they'll be applied from `[target.]` and from `RUSTFLAGS`/`build.rustflags` if `--target ` is given. --- .../compiler/build_context/target_info.rs | 183 +++++++++++------- src/cargo/util/config/mod.rs | 2 +- src/cargo/util/config/target.rs | 29 ++- src/doc/src/reference/unstable.md | 43 ++-- tests/testsuite/build_script.rs | 9 +- tests/testsuite/rustflags.rs | 148 +++++++++++++- 6 files changed, 310 insertions(+), 104 deletions(-) diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index 505910d5fa2..c7c4535a921 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -573,17 +573,23 @@ fn output_err_info(cmd: &ProcessBuilder, stdout: &str, stderr: &str) -> String { /// - the `CARGO_ENCODED_RUSTFLAGS` environment variable /// - the `RUSTFLAGS` environment variable /// -/// then if this was not found +/// then if none of those were found /// /// - `target.*.rustflags` from the config (.cargo/config) /// - `target.cfg(..).rustflags` from the config +/// - `host.*.rustflags` from the config if compiling a host artifact or without `--target` /// -/// then if neither of these were found +/// then if none of those were found /// /// - `build.rustflags` from the config /// -/// Note that if a `target` is specified, no args will be passed to host code (plugins, build -/// scripts, ...), even if it is the same as the target. +/// The behavior differs slightly when cross-compiling (or, specifically, when `--target` is +/// provided) for artifacts that are always built for the host (plugins, build scripts, ...). +/// For those artifacts, the behavior depends on `target-applies-to-host`. In the default +/// configuration (where `target-applies-to-host` is unset), and if `target-applies-to-host = +/// false`, host artifacts _only_ respect `host.*.rustflags`, and no other configuration sources. +/// If `target-applies-to-host = true`, host artifacts also respect `target.`, and if +/// ` == ` `RUSTFLAGS` and `CARGO_ENCODED_RUSTFLAGS`. fn env_args( config: &Config, requested_kinds: &[CompileKind], @@ -592,49 +598,67 @@ fn env_args( kind: CompileKind, name: &str, ) -> CargoResult> { - // We *want* to apply RUSTFLAGS only to builds for the - // requested target architecture, and not to things like build - // scripts and plugins, which may be for an entirely different - // architecture. Cargo's present architecture makes it quite - // hard to only apply flags to things that are not build - // scripts and plugins though, so we do something more hacky - // instead to avoid applying the same RUSTFLAGS to multiple targets - // arches: + let target_applies_to_host = config.target_applies_to_host()?; + + // Include untargeted configuration sources (like `RUSTFLAGS`) if // - // 1) If --target is not specified we just apply RUSTFLAGS to - // all builds; they are all going to have the same target. + // - we're compiling artifacts for the target platform; or + // - we're not cross-compiling; or + // - we're compiling host artifacts, the requested target matches the host, and the user has + // requested that the host pick up target configurations. // - // 2) If --target *is* specified then we only apply RUSTFLAGS - // to compilation units with the Target kind, which indicates - // it was chosen by the --target flag. + // The rationale for the third condition is that `RUSTFLAGS` is intended to affect compilation + // for the target, and `target-applies-to-host` makes it so that the host is affected by its + // target's config, so if `--target ` then `RUSTFLAGS` should also apply to the + // host. + let include_generic = !kind.is_host() + || requested_kinds == [CompileKind::Host] + || (target_applies_to_host == Some(true) + && requested_kinds == [CompileKind::Target(CompileTarget::new(host_triple)?)]); + // Include targeted configuration sources (like `target.*.rustflags`) if // - // This means that, e.g., even if the specified --target is the - // same as the host, build scripts in plugins won't get - // RUSTFLAGS. - if requested_kinds != [CompileKind::Host] && kind.is_host() { - // This is probably a build script or plugin and we're - // compiling with --target. In this scenario there are - // no rustflags we can apply. - return Ok(Vec::new()); - } - - // First try CARGO_ENCODED_RUSTFLAGS from the environment. - // Prefer this over RUSTFLAGS since it's less prone to encoding errors. - if let Ok(a) = env::var(format!("CARGO_ENCODED_{}", name)) { - if a.is_empty() { - return Ok(Vec::new()); + // - we're compiling artifacts for the target platform; or + // - we're not cross-compiling; or + // - we're compiling host artifacts and the user has requested that the host pick up target + // configurations. + // + // The middle condition here may seem counter-intuitive. If `target-applies-to-host-kind` is + // disabled, then `target.*.rustflags` should arguably not apply to host artifacts. However, + // this is likely surprising to users who set `target..rustflags` and run `cargo + // build` (with no `--target`). So, we respect `target.`.rustflags` when + // `--target` _isn't_ supplied, which arguably matches the mental model established by + // respecting `RUSTFLAGS` when `--target` isn't supplied. + let include_for_target = !kind.is_host() + || requested_kinds == [CompileKind::Host] + || target_applies_to_host == Some(true); + // Include host-based configuration sources (like `host.*.rustflags`) if + // + // - we're compiling host artifacts; or + // - we're not cross-compiling + // + // Note that we do _not_ read `host.*.rustflags` just because the host's target is the same as + // the requested target, as that's the whole point of the `host` section in the first place. + let include_for_host = kind.is_host() || requested_kinds == [CompileKind::Host]; + + if include_generic { + // First try CARGO_ENCODED_RUSTFLAGS from the environment. + // Prefer this over RUSTFLAGS since it's less prone to encoding errors. + if let Ok(a) = env::var(format!("CARGO_ENCODED_{}", name)) { + if a.is_empty() { + return Ok(Vec::new()); + } + return Ok(a.split('\x1f').map(str::to_string).collect()); } - return Ok(a.split('\x1f').map(str::to_string).collect()); - } - // Then try RUSTFLAGS from the environment - if let Ok(a) = env::var(name) { - let args = a - .split(' ') - .map(str::trim) - .filter(|s| !s.is_empty()) - .map(str::to_string); - return Ok(args.collect()); + // Then try RUSTFLAGS from the environment + if let Ok(a) = env::var(name) { + let args = a + .split(' ') + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(str::to_string); + return Ok(args.collect()); + } } let mut rustflags = Vec::new(); @@ -643,44 +667,55 @@ fn env_args( .chars() .flat_map(|c| c.to_lowercase()) .collect::(); - // Then the target.*.rustflags value... - let target = match &kind { - CompileKind::Host => host_triple, - CompileKind::Target(target) => target.short_name(), - }; - let key = format!("target.{}.{}", target, name); - if let Some(args) = config.get::>(&key)? { - rustflags.extend(args.as_slice().iter().cloned()); + if include_for_target { + // Then the target.*.rustflags value... + let target = match &kind { + CompileKind::Host => host_triple, + CompileKind::Target(target) => target.short_name(), + }; + let key = format!("target.{}.{}", target, name); + if let Some(args) = config.get::>(&key)? { + rustflags.extend(args.as_slice().iter().cloned()); + } + // ...including target.'cfg(...)'.rustflags + if let Some(target_cfg) = target_cfg { + config + .target_cfgs()? + .iter() + .filter_map(|(key, cfg)| { + cfg.rustflags + .as_ref() + .map(|rustflags| (key, &rustflags.val)) + }) + .filter(|(key, _rustflags)| CfgExpr::matches_key(key, target_cfg)) + .for_each(|(_key, cfg_rustflags)| { + rustflags.extend(cfg_rustflags.as_slice().iter().cloned()); + }); + } } - // ...including target.'cfg(...)'.rustflags - if let Some(target_cfg) = target_cfg { - config - .target_cfgs()? - .iter() - .filter_map(|(key, cfg)| { - cfg.rustflags - .as_ref() - .map(|rustflags| (key, &rustflags.val)) - }) - .filter(|(key, _rustflags)| CfgExpr::matches_key(key, target_cfg)) - .for_each(|(_key, cfg_rustflags)| { - rustflags.extend(cfg_rustflags.as_slice().iter().cloned()); - }); + + if include_for_host { + let target_cfg = config.host_cfg_triple(host_triple)?; + if let Some(rf) = target_cfg.rustflags { + rustflags.extend(rf.val.as_slice().iter().cloned()); + } } if !rustflags.is_empty() { return Ok(rustflags); } - // Then the `build.rustflags` value. - let build = config.build_config()?; - let list = if name == "rustflags" { - &build.rustflags - } else { - &build.rustdocflags - }; - if let Some(list) = list { - return Ok(list.as_slice().to_vec()); + if include_generic { + // Then the `build.rustflags` value. + let build = config.build_config()?; + let list = if name == "rustflags" { + &build.rustflags + } else { + &build.rustdocflags + }; + if let Some(list) = list { + return Ok(list.as_slice().to_vec()); + } } Ok(Vec::new()) @@ -718,7 +753,7 @@ impl<'cfg> RustcTargetData<'cfg> { let mut target_info = HashMap::new(); let target_applies_to_host = config.target_applies_to_host()?; let host_info = TargetInfo::new(config, requested_kinds, &rustc, CompileKind::Host)?; - let host_config = if target_applies_to_host { + let host_config = if target_applies_to_host != Some(false) { config.target_cfg_triple(&rustc.host)? } else { config.host_cfg_triple(&rustc.host)? diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 0f183cdf563..83604e896c1 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -1551,7 +1551,7 @@ impl Config { } /// Returns true if the `[target]` table should be applied to host targets. - pub fn target_applies_to_host(&self) -> CargoResult { + pub fn target_applies_to_host(&self) -> CargoResult> { target::get_target_applies_to_host(self) } diff --git a/src/cargo/util/config/target.rs b/src/cargo/util/config/target.rs index 21089ce491b..407f120e5d1 100644 --- a/src/cargo/util/config/target.rs +++ b/src/cargo/util/config/target.rs @@ -66,20 +66,29 @@ pub(super) fn load_target_cfgs(config: &Config) -> CargoResult CargoResult { +pub(super) fn get_target_applies_to_host(config: &Config) -> CargoResult> { if config.cli_unstable().target_applies_to_host { - if let Ok(target_applies_to_host) = config.get::("target-applies-to-host") { - Ok(target_applies_to_host) - } else { - Ok(!config.cli_unstable().host_config) + if let Ok(target_applies_to_host) = config.get::>("target-applies-to-host") { + if config.cli_unstable().host_config { + match target_applies_to_host { + Some(true) => { + anyhow::bail!( + "the -Zhost-config flag requires target-applies-to-host = false" + ); + } + Some(false) => {} + None => { + // -Zhost-config automatically disables target-applies-to-host + return Ok(Some(false)); + } + } + } + return Ok(target_applies_to_host); } } else if config.cli_unstable().host_config { - anyhow::bail!( - "the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set" - ); - } else { - Ok(true) + anyhow::bail!("the -Zhost-config flag requires -Ztarget-applies-to-host"); } + Ok(None) } /// Loads a single `[host]` table for the given triple. diff --git a/src/doc/src/reference/unstable.md b/src/doc/src/reference/unstable.md index 7ca9d7b9f6b..9a19bc698f9 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -508,14 +508,30 @@ CLI paths are relative to the current working directory. * Original Pull Request: [#9322](https://github.com/rust-lang/cargo/pull/9322) * Tracking Issue: [#9453](https://github.com/rust-lang/cargo/issues/9453) -The `target-applies-to-host` key in a config file can be used set the desired -behavior for passing target config flags to build scripts. - -It requires the `-Ztarget-applies-to-host` command-line option. - -The current default for `target-applies-to-host` is `true`, which will be -changed to `false` in the future, if `-Zhost-config` is used the new `false` -default will be set for `target-applies-to-host`. +Historically, Cargo has respected the `linker` and `runner` options (but +_not_ `rustflags`) of `[target.]` configuration sections +when building and running build scripts, plugins, and other artifacts +that are _always_ built for the host platform. +`-Ztarget-applies-to-host` enables the top-level +`target-applies-to-host` setting in Cargo configuration files which +allows users to opt into different (and more consistent) behavior for +these properties. + +When `target-applies-to-host` is unset in the configuration file, the +existing Cargo behavior is preserved (though see `-Zhost-config`, which +changes that default). When it is set to `true`, _all_ options from +`[target.]` are respected for host artifacts. When it is +set to `false`, _no_ options from `[target.]` are respected +for host artifacts. + +In the specific case of `rustflags`, this settings also affects whether +`--target ` picks up the same configuration as if +`--target` was not supplied in the first place. + +In the future, `target-applies-to-host` may end up defaulting to `false` +to provide more sane and consistent default behavior. When set to +`false`, `host-config` can be used to customize the behavior for host +artifacts. ```toml # config.toml @@ -535,8 +551,9 @@ such as build scripts that must run on the host system instead of the target system when cross compiling. It supports both generic and host arch specific tables. Matching host arch tables take precedence over generic host tables. -It requires the `-Zhost-config` and `-Ztarget-applies-to-host` command-line -options to be set. +It requires the `-Zhost-config` and `-Ztarget-applies-to-host` +command-line options to be set, and that `target-applies-to-host = +false` is set in the Cargo configuration file. ```toml # config.toml @@ -544,6 +561,7 @@ options to be set. linker = "/path/to/host/linker" [host.x86_64-unknown-linux-gnu] linker = "/path/to/host/arch/linker" +rustflags = ["-Clink-arg=--verbose"] [target.x86_64-unknown-linux-gnu] linker = "/path/to/target/linker" ``` @@ -552,8 +570,9 @@ The generic `host` table above will be entirely ignored when building on a `x86_64-unknown-linux-gnu` host as the `host.x86_64-unknown-linux-gnu` table takes precedence. -Setting `-Zhost-config` changes the default for `target-applies-to-host` to -`false` from `true`. +Setting `-Zhost-config` changes the default value of +`target-applies-to-host` to `false`, and will error if +`target-applies-to-host` is set to `true`. ```console cargo +nightly -Ztarget-applies-to-host -Zhost-config build --target x86_64-unknown-linux-gnu diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index 95449e3aa98..f57c120588e 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -469,7 +469,7 @@ fn custom_build_invalid_host_config_feature_flag() { .with_status(101) .with_stderr_contains( "\ -error: the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set +error: the -Zhost-config flag requires -Ztarget-applies-to-host ", ) .run(); @@ -483,7 +483,6 @@ fn custom_build_env_var_rustc_linker_host_target_with_bad_host_config() { ".cargo/config", &format!( r#" - target-applies-to-host = true [host] linker = "/path/to/host/linker" [target.{}] @@ -496,7 +495,7 @@ fn custom_build_env_var_rustc_linker_host_target_with_bad_host_config() { .file("src/lib.rs", "") .build(); - // build.rs should fail due to bad target linker being set + // build.rs should fail due to bad host linker being set p.cargo("build -Z target-applies-to-host -Z host-config --verbose --target") .arg(&target) .masquerade_as_nightly_cargo() @@ -504,8 +503,8 @@ fn custom_build_env_var_rustc_linker_host_target_with_bad_host_config() { .with_stderr_contains( "\ [COMPILING] foo v0.0.1 ([CWD]) -[RUNNING] `rustc --crate-name build_script_build build.rs [..]--crate-type bin [..]-C linker=[..]/path/to/target/linker [..]` -[ERROR] linker `[..]/path/to/target/linker` not found +[RUNNING] `rustc --crate-name build_script_build build.rs [..]--crate-type bin [..]-C linker=[..]/path/to/host/linker [..]` +[ERROR] linker `[..]/path/to/host/linker` not found " ) .run(); diff --git a/tests/testsuite/rustflags.rs b/tests/testsuite/rustflags.rs index 163f410a1dd..d0ee0ac931d 100644 --- a/tests/testsuite/rustflags.rs +++ b/tests/testsuite/rustflags.rs @@ -272,6 +272,47 @@ fn env_rustflags_build_script_with_target() { .run(); } +#[cargo_test] +fn env_rustflags_build_script_with_target_and_applies_to_host_kind() { + // RUSTFLAGS *should* be passed to rustc for build scripts when --target is specified as the + // host triple and target-applies-to-host-kind is enabled. + // In this test if --cfg foo is not passed the build will fail. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + build = "build.rs" + "#, + ) + .file("src/lib.rs", "") + .file( + "build.rs", + r#" + fn main() { } + #[cfg(not(foo))] + fn main() { } + "#, + ) + .file( + ".cargo/config.toml", + r#" + target-applies-to-host = true + "#, + ) + .build(); + + let host = rustc_host(); + p.cargo("build --target") + .masquerade_as_nightly_cargo() + .arg(host) + .arg("-Ztarget-applies-to-host") + .env("RUSTFLAGS", "--cfg foo") + .run(); +} + #[cargo_test] fn env_rustflags_build_script_dep_with_target() { // RUSTFLAGS should not be passed to rustc for build scripts @@ -664,7 +705,7 @@ fn build_rustflags_normal_source_with_target() { let host = &rustc_host(); - // Use RUSTFLAGS to pass an argument that will generate an error + // Use build.rustflags to pass an argument that will generate an error p.cargo("build --lib --target") .arg(host) .with_status(101) @@ -690,6 +731,25 @@ fn build_rustflags_normal_source_with_target() { .with_status(101) .with_stderr_contains("[..]bogus[..]") .run(); + + // With -Ztarget-applies-to-host build.rustflags should apply to build.rs since target == host + p.change_file("build.rs", "fn main() {}"); + p.change_file( + ".cargo/config", + r#" + target-applies-to-host = true + + [build] + rustflags = ["-Clink-arg=this is a bogus argument"] + "#, + ); + p.cargo("build --lib --target") + .arg(host) + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .with_status(101) + .with_stderr_contains("[..]build_script_build[..]") + .run(); } #[cargo_test] @@ -993,6 +1053,89 @@ fn target_rustflags_normal_source() { .run(); } +#[cargo_test] +fn target_rustflags_also_for_build_scripts() { + let p = project() + .file("src/lib.rs", "") + .file( + "build.rs", + r#" + fn main() { } + #[cfg(not(foo))] + fn main() { } + "#, + ) + .file( + ".cargo/config", + &format!( + " + [target.{}] + rustflags = [\"--cfg=foo\"] + ", + rustc_host() + ), + ) + .build(); + + p.cargo("build").run(); +} + +#[cargo_test] +fn target_rustflags_not_for_build_scripts_with_target() { + let host = rustc_host(); + let p = project() + .file("src/lib.rs", "") + .file( + "build.rs", + r#" + fn main() { } + #[cfg(foo)] + fn main() { } + "#, + ) + .file( + ".cargo/config", + &format!( + " + [target.{}] + rustflags = [\"--cfg=foo\"] + ", + host + ), + ) + .build(); + + p.cargo("build --target").arg(host).run(); + + // Enabling -Ztarget-applies-to-host should not make a difference without the config setting + p.cargo("build --target") + .arg(host) + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .run(); + + // But if we also set the setting, then the rustflags from `target.` should apply + p.change_file( + ".cargo/config", + &format!( + " + target-applies-to-host = true + + [target.{}] + rustflags = [\"--cfg=foo\"] + ", + host + ), + ); + p.cargo("build --target") + .arg(host) + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .with_status(101) + .with_stderr_contains("[..]the name `main` is defined multiple times[..]") + .run(); +} + // target.{}.rustflags takes precedence over build.rustflags #[cargo_test] fn target_rustflags_precedence() { @@ -1442,12 +1585,13 @@ fn host_config_rustflags_with_target() { let p = project() .file("src/lib.rs", "") .file("build.rs.rs", "fn main() { assert!(cfg!(foo)); }") + .file(".cargo/config.toml", "target-applies-to-host = false") .build(); p.cargo("build") .masquerade_as_nightly_cargo() .arg("-Zhost-config") - .arg("-Ztarget-applies-to-host=no") + .arg("-Ztarget-applies-to-host") .arg("-Zunstable-options") .arg("--config") .arg("host.rustflags=[\"--cfg=foo\"]")