Auto merge of #10395 - jonhoo:fix-10206, r=alexcrichton

Enable propagating host rustflags to build scripts

### What does this PR try to resolve?

This PR primarily fixes #10206, but in doing so it also slightly modifies the interface for the unstable `target-applies-to-host` feature (#9453), and adds the unstable `target-applies-to-host-kind` flag to mirror `target-applies-to-host` for build scripts and other host artifacts.

The commit messages have more in-depth discussion.

### How should we test and review this PR?

The test case from #10206 now works rather than producing an error. It has also been added a regression test case. A few additional test cases have also been added to handle the expected behavior around rustflags for build scripts with and without `target-applies-to-host-kind` enabled.

### Additional information

1. This changes the interface for `target-applies-to-host` so that it does not need to be specified twice to be used. And it can still be set through configuration files using the `[unstable]` table. However, we may(?) want to pick a stable format for in-file configuration of this setting unless we intend for it to only ever be a command-line flag.
2. It may be that `target-applies-to-host-kind` is never behavior we want users to turn on, and that it should therefore simply be removed and hard-coded as being `false`.
3. It's not entirely clear how `target-applies-to-host-kind` should interact with `-Zmultitarget`. If, for example, `requested_kinds = [HostTarget, SomeOtherTarget]` and `kind.is_host()`, should `RUSTFLAGS` take effect or not? For the time being I've just hard-coded the behavior for single targets, and the answer would be "no".

Author: bors

src/cargo/core/compiler/build_context/target_info.rs

@@ -144,7 +144,7 @@ impl TargetInfo {
             &rustc.host,
             None,
             kind,
-            "RUSTFLAGS",
+            Flags::Rust,
         )?;
         let extra_fingerprint = kind.fingerprint_hash();
         let mut process = rustc.workspace_process();
@@ -241,15 +241,15 @@ impl TargetInfo {
                 &rustc.host,
                 Some(&cfg),
                 kind,
-                "RUSTFLAGS",
+                Flags::Rust,
             )?,
             rustdocflags: env_args(
                 config,
                 requested_kinds,
                 &rustc.host,
                 Some(&cfg),
                 kind,
-                "RUSTDOCFLAGS",
+                Flags::Rustdoc,
             )?,
             cfg,
             supports_split_debuginfo,
@@ -554,89 +554,134 @@ fn output_err_info(cmd: &ProcessBuilder, stdout: &str, stderr: &str) -> String {
     result
 }
 
+#[derive(Debug, Copy, Clone)]
+enum Flags {
+    Rust,
+    Rustdoc,
+}
+
+impl Flags {
+    fn as_key(self) -> &'static str {
+        match self {
+            Flags::Rust => "rustflags",
+            Flags::Rustdoc => "rustdocflags",
+        }
+    }
+
+    fn as_env(self) -> &'static str {
+        match self {
+            Flags::Rust => "RUSTFLAGS",
+            Flags::Rustdoc => "RUSTDOCFLAGS",
+        }
+    }
+}
+
 /// Acquire extra flags to pass to the compiler from various locations.
 ///
 /// The locations are:
 ///
 ///  - 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, _only_ `host.*.rustflags` is respected, and no other configuration
+/// sources, _regardless of the value of `target-applies-to-host`_. This is counterintuitive, but
+/// necessary to retain bacwkards compatibility with older versions of Cargo.
 fn env_args(
     config: &Config,
     requested_kinds: &[CompileKind],
     host_triple: &str,
     target_cfg: Option<&[Cfg]>,
     kind: CompileKind,
-    name: &str,
+    flags: Flags,
 ) -> CargoResult<Vec<String>> {
-    // 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:
-    //
-    // 1) If --target is not specified we just apply RUSTFLAGS to
-    // all builds; they are all going to have the same target.
-    //
-    // 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.
+    let target_applies_to_host = config.target_applies_to_host()?;
+
+    // Host artifacts should not generally pick up rustflags from anywhere except [host].
     //
-    // 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());
+    // The one exception to this is if `target-applies-to-host = true`, which opts into a
+    // particular (inconsistent) past Cargo behavior where host artifacts _do_ pick up rustflags
+    // set elsewhere when `--target` isn't passed.
+    if kind.is_host() {
+        if target_applies_to_host && requested_kinds == [CompileKind::Host] {
+            // This is the past Cargo behavior where we fall back to the same logic as for other
+            // artifacts without --target.
+        } else {
+            // In all other cases, host artifacts just get flags from [host], regardless of
+            // --target. Or, phrased differently, no `--target` behaves the same as `--target
+            // <host>`, and host artifacts are always "special" (they don't pick up `RUSTFLAGS` for
+            // example).
+            return Ok(rustflags_from_host(config, flags, host_triple)?.unwrap_or_else(Vec::new));
+        }
     }
 
+    // All other artifacts pick up the RUSTFLAGS, [target.*], and [build], in that order.
+    // NOTE: It is impossible to have a [host] section and reach this logic with kind.is_host(),
+    // since [host] implies `target-applies-to-host = false`, which always early-returns above.
+
+    if let Some(rustflags) = rustflags_from_env(flags) {
+        Ok(rustflags)
+    } else if let Some(rustflags) =
+        rustflags_from_target(config, host_triple, target_cfg, kind, flags)?
+    {
+        Ok(rustflags)
+    } else if let Some(rustflags) = rustflags_from_build(config, flags)? {
+        Ok(rustflags)
+    } else {
+        Ok(Vec::new())
+    }
+}
+
+fn rustflags_from_env(flags: Flags) -> Option<Vec<String>> {
     // 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 let Ok(a) = env::var(format!("CARGO_ENCODED_{}", flags.as_env())) {
         if a.is_empty() {
-            return Ok(Vec::new());
+            return Some(Vec::new());
         }
-        return Ok(a.split('\x1f').map(str::to_string).collect());
+        return Some(a.split('\x1f').map(str::to_string).collect());
     }
 
     // Then try RUSTFLAGS from the environment
-    if let Ok(a) = env::var(name) {
+    if let Ok(a) = env::var(flags.as_env()) {
         let args = a
             .split(' ')
             .map(str::trim)
             .filter(|s| !s.is_empty())
             .map(str::to_string);
-        return Ok(args.collect());
+        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,
+    flag: Flags,
+) -> CargoResult<Option<Vec<String>>> {
     let mut rustflags = Vec::new();
 
-    let name = name
-        .chars()
-        .flat_map(|c| c.to_lowercase())
-        .collect::<String>();
     // 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);
+    let key = format!("target.{}.{}", target, flag.as_key());
     if let Some(args) = config.get::<Option<StringList>>(&key)? {
         rustflags.extend(args.as_slice().iter().cloned());
     }
@@ -656,22 +701,37 @@ fn env_args(
             });
     }
 
-    if !rustflags.is_empty() {
-        return Ok(rustflags);
+    if rustflags.is_empty() {
+        Ok(None)
+    } else {
+        Ok(Some(rustflags))
     }
+}
 
+fn rustflags_from_host(
+    config: &Config,
+    flag: Flags,
+    host_triple: &str,
+) -> CargoResult<Option<Vec<String>>> {
+    let target_cfg = config.host_cfg_triple(host_triple)?;
+    let list = match flag {
+        Flags::Rust => &target_cfg.rustflags,
+        Flags::Rustdoc => {
+            // host.rustdocflags is not a thing, since it does not make sense
+            return Ok(None);
+        }
+    };
+    Ok(list.as_ref().map(|l| l.val.as_slice().to_vec()))
+}
+
+fn rustflags_from_build(config: &Config, flag: Flags) -> CargoResult<Option<Vec<String>>> {
     // Then the `build.rustflags` value.
     let build = config.build_config()?;
-    let list = if name == "rustflags" {
-        &build.rustflags
-    } else {
-        &build.rustdocflags
+    let list = match flag {
+        Flags::Rust => &build.rustflags,
+        Flags::Rustdoc => &build.rustdocflags,
     };
-    if let Some(list) = list {
-        return Ok(list.as_slice().to_vec());
-    }
-
-    Ok(Vec::new())
+    Ok(list.as_ref().map(|l| l.as_slice().to_vec()))
 }
 
 /// Collection of information about `rustc` and the host and target.

src/doc/src/reference/unstable.md

@@ -509,14 +509,32 @@ 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's behavior for whether the `linker` and `rustflags`
+configuration options from environment variables and `[target]` are
+respected for build scripts, plugins, and other artifacts that are
+_always_ built for the host platform has been somewhat inconsistent.
+When `--target` is _not_ passed, Cargo respects the same `linker` and
+`rustflags` for build scripts as for all other compile artifacts. When
+`--target` _is_ passed, however, Cargo respects `linker` from
+`[target.<host triple>]`, and does not pick up any `rustflags`
+configuration. This dual behavior is confusing, but also makes it
+difficult to correctly configure builds where the host triple and the
+target triple happen to be the same, but artifacts intended to run on
+the build host should still be configured differently.
+
+`-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, 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.<host triple>]`,
+`RUSTFLAGS`, or `[build]` are respected for host artifacts regardless of
+whether `--target` is passed to Cargo. To customize artifacts intended
+to be run on the host, use `[host]` ([`host-config`](#host-config)).
+
+In the future, `target-applies-to-host` may end up defaulting to `false`
+to provide more sane and consistent default behavior.
 
 ```toml
 # config.toml
@@ -536,15 +554,17 @@ 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
 [host]
 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"
 ```