From f23894d93fefaf191e8d665994a5a4b40a340168 Mon Sep 17 00:00:00 2001 From: Jon Gjengset Date: Wed, 23 Feb 2022 16:36:14 -0800 Subject: [PATCH] Make rustflags logic more linear --- .../compiler/build_context/target_info.rs | 261 ++++++++++-------- src/cargo/util/config/target.rs | 28 +- src/doc/src/reference/unstable.md | 9 +- tests/testsuite/build_script.rs | 2 +- tests/testsuite/rustflags.rs | 66 +++++ 5 files changed, 225 insertions(+), 141 deletions(-) diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index b50507671..7f5a0248c 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/src/cargo/core/compiler/build_context/target_info.rs @@ -596,145 +596,172 @@ fn env_args( kind: CompileKind, name: &str, ) -> CargoResult> { - // The `target-applies-to-host` setting is somewhat misleading in name. - // - // What it really does it opt into a _particular_ past Cargo behavior for `[target.]` for - // host artifacts, namely that `linker` and `runner` from `[target.]` are respected, but - // `rustflags` is _not_. - // - // The code (and comments) below are written to first apply `target-applies-to-host` _as if it - // was consistent_, and then adjusts the settings to match the legacy behavior that - // `target-applies-to-host = true` _really_ enables. This was done to (hopefully) make the - // logic flow clearer, and to make future modifications to the rules for when rustflags get - // applied less likely to break the legacy behavior. + // The `target-applies-to-host` setting is somewhat misleading in name. What it really does it + // opt into a _particular_ legacy Cargo behavior for how rustflags are picked up. let target_applies_to_host = config.target_applies_to_host()?; + if target_applies_to_host == true { + // We're operating in "legacy" Cargo mode, where the presence or absence of --target makes + // a difference for which configuration options are picked up. Specifically: + // + // If --target is not passed, all configuration sources are consulted whether we're + // compiling a host artifact or not. + if requested_kinds == [CompileKind::Host] { + // RUSTFLAGS takes priority + if let Some(rustflags) = rustflags_from_env(name) { + Ok(rustflags) - // Include untargeted configuration sources (like `RUSTFLAGS`) if - // - // - 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. - // - // 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 mut include_generic = !kind.is_host() - || requested_kinds == [CompileKind::Host] - || (target_applies_to_host - && requested_kinds == [CompileKind::Target(CompileTarget::new(host_triple)?)]); + // [host] is a new feature, so we have it take priority over [target.] + } else if let Some(rustflags) = rustflags_from_host(config, host_triple)? { + Ok(rustflags) - // Include targeted configuration sources (like `target.*.rustflags`) if - // - // - 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 mut include_for_target = - !kind.is_host() || requested_kinds == [CompileKind::Host] || target_applies_to_host; + // but [target.] _does_ apply to host artifacts + } else if let Some(rustflags) = + rustflags_from_target(config, host_triple, target_cfg, kind, name)? + { + Ok(rustflags) - // 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]; - - // Apply the legacy behavior of target_applies_to_host. - if target_applies_to_host && kind.is_host() && requested_kinds != [CompileKind::Host] { - include_generic = false; - include_for_target = false; - } - - 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()); + // and otherwise we fall back to [build] + } else if let Some(rustflags) = rustflags_from_build(config, name)? { + Ok(rustflags) + } else { + Ok(Vec::new()) + } + + // If --target _is_ passed, then host artifacts traditionally got _no_ rustflags applies. + // Since [host] is a new feature, we can dictate that it is respect even in this mode + // though. + } else if kind.is_host() { + if let Some(rustflags) = rustflags_from_host(config, host_triple)? { + Ok(rustflags) + } else { + Ok(Vec::new()) + } + + // All other artifacts pick up the RUSTFLAGS, [target.*], and [build], in that order. + } else { + if let Some(rustflags) = rustflags_from_env(name) { + Ok(rustflags) + } else if let Some(rustflags) = + rustflags_from_target(config, host_triple, target_cfg, kind, name)? + { + Ok(rustflags) + } else if let Some(rustflags) = rustflags_from_build(config, name)? { + Ok(rustflags) + } else { + Ok(Vec::new()) } - 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()); + // If we're _not_ in legacy mode, then host artifacts just get flags from [host], regardless of + // --target. Or, phrased differently, no `--target` behaves the same as `--target `, and + // host artifacts are always "special" (they don't pick up `RUSTFLAGS` for example). + } else if kind.is_host() { + Ok(rustflags_from_host(config, host_triple)?.unwrap_or_else(Vec::new)) + + // All other artifacts pick up the RUSTFLAGS, [target.*], and [build], in that order. + } else { + if let Some(rustflags) = rustflags_from_env(name) { + Ok(rustflags) + } else if let Some(rustflags) = + rustflags_from_target(config, host_triple, target_cfg, kind, name)? + { + Ok(rustflags) + } else if let Some(rustflags) = rustflags_from_build(config, name)? { + Ok(rustflags) + } else { + Ok(Vec::new()) } } +} +fn rustflags_from_env(name: &str) -> Option> { + // 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 Some(Vec::new()); + } + return Some(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 Some(args.collect()); + } + + // No rustflags to be collected from the environment + None +} + +fn rustflags_from_target( + config: &Config, + host_triple: &str, + target_cfg: Option<&[Cfg]>, + kind: CompileKind, + name: &str, +) -> CargoResult>> { let mut rustflags = Vec::new(); let name = name .chars() .flat_map(|c| c.to_lowercase()) .collect::(); - 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()); - }); - } + + // 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()); + }); } - 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() { + Ok(None) + } else { + Ok(Some(rustflags)) } +} - if !rustflags.is_empty() { - return Ok(rustflags); +fn rustflags_from_host(config: &Config, host_triple: &str) -> CargoResult>> { + let target_cfg = config.host_cfg_triple(host_triple)?; + if let Some(rf) = target_cfg.rustflags { + Ok(Some(rf.val.as_slice().iter().cloned().collect())) + } else { + Ok(None) } +} - 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()) +fn rustflags_from_build(config: &Config, name: &str) -> CargoResult>> { + // Then the `build.rustflags` value. + let build = config.build_config()?; + let list = if name.eq_ignore_ascii_case("rustflags") { + &build.rustflags + } else { + &build.rustdocflags + }; + Ok(list.as_ref().map(|l| l.as_slice().to_vec())) } /// Collection of information about `rustc` and the host and target. diff --git a/src/cargo/util/config/target.rs b/src/cargo/util/config/target.rs index 1174a12cc..21089ce49 100644 --- a/src/cargo/util/config/target.rs +++ b/src/cargo/util/config/target.rs @@ -67,29 +67,19 @@ pub(super) fn load_target_cfgs(config: &Config) -> CargoResult CargoResult { - let default = true; if config.cli_unstable().target_applies_to_host { - 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(false); - } - } - } - return Ok(target_applies_to_host.unwrap_or(default)); + 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) } } else if config.cli_unstable().host_config { - anyhow::bail!("the -Zhost-config flag requires -Ztarget-applies-to-host"); + anyhow::bail!( + "the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set" + ); + } else { + Ok(true) } - Ok(default) } /// 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 0bdd4453b..f79ed6bc2 100644 --- a/src/doc/src/reference/unstable.md +++ b/src/doc/src/reference/unstable.md @@ -521,7 +521,9 @@ When `target-applies-to-host` is unset, or set to `true`, in the configuration file, the existing Cargo behavior is preserved (though see `-Zhost-config`, which changes that default). When it is set to `false`, no options from `[target.]` are respected for host -artifacts. +artifacts. Furthermore, when set to `false`, host artifacts do not pick +up flags from `RUSTFLAGS` or `[build]`, even if `--target` is _not_ +passed to Cargo. In the future, `target-applies-to-host` may end up defaulting to `false` to provide more sane and consistent default behavior. When set to @@ -565,9 +567,8 @@ 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 value of -`target-applies-to-host` to `false`, and will error if -`target-applies-to-host` is set to `true`. +Setting `-Zhost-config` changes the default for `target-applies-to-host` to +`false` from `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 b1e00b7b7..d0ef3a2e7 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 -Ztarget-applies-to-host +error: the -Zhost-config flag requires the -Ztarget-applies-to-host flag to be set ", ) .run(); diff --git a/tests/testsuite/rustflags.rs b/tests/testsuite/rustflags.rs index a271a94d3..9b5441ef0 100644 --- a/tests/testsuite/rustflags.rs +++ b/tests/testsuite/rustflags.rs @@ -1136,6 +1136,72 @@ fn target_rustflags_not_for_build_scripts_with_target() { .run(); } +#[cargo_test] +fn build_rustflags_for_build_scripts() { + let host = rustc_host(); + let p = project() + .file("src/lib.rs", "") + .file( + "build.rs", + r#" + fn main() { } + #[cfg(foo)] + fn main() { } + "#, + ) + .file( + ".cargo/config", + " + [build] + rustflags = [\"--cfg=foo\"] + ", + ) + .build(); + + // With "legacy" behavior, build.rustflags should apply to build scripts without --target + p.cargo("build") + .with_status(101) + .with_stderr_does_not_contain("[..]build_script_build[..]") + .run(); + + // But should _not_ apply _with_ --target + p.cargo("build --target").arg(host).run(); + + // Enabling -Ztarget-applies-to-host should not make a difference without the config setting + p.cargo("build") + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .with_status(101) + .with_stderr_does_not_contain("[..]build_script_build[..]") + .run(); + p.cargo("build --target") + .arg(host) + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .run(); + + // When set to false though, the "proper" behavior where host artifacts _only_ pick up on + // [host] should be applied. + p.change_file( + ".cargo/config", + " + target-applies-to-host = false + + [build] + rustflags = [\"--cfg=foo\"] + ", + ); + p.cargo("build") + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .run(); + p.cargo("build --target") + .arg(host) + .masquerade_as_nightly_cargo() + .arg("-Ztarget-applies-to-host") + .run(); +} + #[cargo_test] fn host_rustflags_for_build_scripts() { let host = rustc_host();