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.

Sign up for GitHub

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

Jump to bottom

Fix passing of linker with target-applies-to-host and an implicit target #14206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix passing of linker with target-applies-to-host and an implicit target #14206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4296cce
Add test showing that linker is ignored with target_applies_to_host=f…
gmorenz Jul 5, 2024
fa17b99
Fix `linker` value with `target-applies-to-host` and implicit target …
gmorenz Jul 4, 2024
1842e81
Read `rustflags` from `TargetConfig` in `TargetInfo::new`
gmorenz Jul 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 94 additions & 40 deletions src/cargo/core/compiler/build_context/target_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use crate::core::compiler::apply_env_config;
use crate::core::compiler::{BuildRunner, CompileKind, CompileMode, CompileTarget, CrateType};
use crate::core::{Dependency, Package, Target, TargetKind, Workspace};
use crate::util::context::{GlobalContext, StringList, TargetConfig};
use crate::util::context::{GlobalContext, TargetConfig};
use crate::util::interning::InternedString;
use crate::util::{CargoResult, Rustc};
use anyhow::Context as _;
Expand All @@ -20,10 +20,12 @@ use serde::{Deserialize, Serialize};
use std::cell::RefCell;
use std::collections::hash_map::{Entry, HashMap};
use std::path::{Path, PathBuf};
use std::rc::Rc;
use std::str::{self, FromStr};
use std::sync::Arc;

/// Information about the platform target gleaned from querying rustc.
/// Information about the platform target gleaned from querying rustc and from
/// merging configs.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment didn't really fit with this struct containing rustflags (which predates me working on target-applies-to-host). Putting linker in it makes the mismatch slightly worse, so I thought I'd update the comment.

I'm not entirely thrilled with the phrasing. If someone has a suggestion for better phrasing I'd be happy to take it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe somthing like?

/// Information and compiler flags for the platform target gleaned from querying rustc
/// and from Cargo configurations.

///
/// [`RustcTargetData`] keeps several of these, one for the host and the others
/// for other specified targets. If no target is specified, it uses a clone from
Expand Down Expand Up @@ -54,6 +56,8 @@ pub struct TargetInfo {
pub rustflags: Arc<[String]>,
/// Extra flags to pass to `rustdoc`, see [`extra_args`].
pub rustdocflags: Arc<[String]>,
gmorenz marked this conversation as resolved.
Show resolved Hide resolved
/// Linker to use. If the value is `None` it is left up to rustc.
pub linker: Option<Rc<PathBuf>>,
/// Whether or not rustc (stably) supports the `--check-cfg` flag.
///
/// Can be removed once the minimum supported rustc version of Cargo is
Expand Down Expand Up @@ -157,9 +161,16 @@ impl TargetInfo {
requested_kinds: &[CompileKind],
rustc: &Rustc,
kind: CompileKind,
target_config: &TargetConfig,
) -> CargoResult<TargetInfo> {
let mut rustflags =
extra_args(gctx, requested_kinds, &rustc.host, None, kind, Flags::Rust)?;
let mut rustflags = extra_args(
gctx,
requested_kinds,
None,
target_config,
kind,
Flags::Rust,
)?;
let mut turn = 0;
loop {
let extra_fingerprint = kind.fingerprint_hash();
Expand Down Expand Up @@ -281,8 +292,8 @@ impl TargetInfo {
let new_flags = extra_args(
gctx,
requested_kinds,
&rustc.host,
Some(&cfg),
target_config,
kind,
Flags::Rust,
)?;
Expand Down Expand Up @@ -315,12 +326,13 @@ impl TargetInfo {
rustdocflags: extra_args(
gctx,
requested_kinds,
&rustc.host,
Some(&cfg),
target_config,
kind,
Flags::Rustdoc,
)?
.into(),
linker: target_linker(gctx, &cfg, target_config)?.map(Rc::new),
cfg,
support_split_debuginfo,
support_check_cfg,
Expand Down Expand Up @@ -668,13 +680,6 @@ enum Flags {
}

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",
Expand Down Expand Up @@ -711,8 +716,8 @@ impl Flags {
fn extra_args(
gctx: &GlobalContext,
requested_kinds: &[CompileKind],
host_triple: &str,
target_cfg: Option<&[Cfg]>,
target_config: &TargetConfig,
kind: CompileKind,
flags: Flags,
) -> CargoResult<Vec<String>> {
Expand All @@ -732,7 +737,7 @@ fn extra_args(
// --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(gctx, flags, host_triple)?.unwrap_or_else(Vec::new));
return Ok(rustflags_from_host(target_config, flags)?.unwrap_or_else(Vec::new));
}
}

Expand All @@ -742,9 +747,7 @@ fn extra_args(

if let Some(rustflags) = rustflags_from_env(gctx, flags) {
Ok(rustflags)
} else if let Some(rustflags) =
rustflags_from_target(gctx, host_triple, target_cfg, kind, flags)?
{
} else if let Some(rustflags) = rustflags_from_target(gctx, target_cfg, target_config, flags)? {
Ok(rustflags)
} else if let Some(rustflags) = rustflags_from_build(gctx, flags)? {
Ok(rustflags)
Expand Down Expand Up @@ -783,21 +786,18 @@ fn rustflags_from_env(gctx: &GlobalContext, flags: Flags) -> Option<Vec<String>>
/// See [`extra_args`] for more.
fn rustflags_from_target(
gctx: &GlobalContext,
host_triple: &str,
target_cfg: Option<&[Cfg]>,
kind: CompileKind,
target_config: &TargetConfig,
flag: Flags,
) -> CargoResult<Option<Vec<String>>> {
let mut rustflags = Vec::new();

// Then the target.*.rustflags value...
let target = match &kind {
CompileKind::Host => host_triple,
CompileKind::Target(target) => target.short_name(),
let config_flags = match flag {
Flags::Rust => &target_config.rustflags,
Flags::Rustdoc => &target_config.rustdocflags,
};
let key = format!("target.{}.{}", target, flag.as_key());
if let Some(args) = gctx.get::<Option<StringList>>(&key)? {
rustflags.extend(args.as_slice().iter().cloned());
if let Some(args) = config_flags {
rustflags.extend(args.val.as_slice().iter().cloned());
}
// ...including target.'cfg(...)'.rustflags
if let Some(target_cfg) = target_cfg {
Expand Down Expand Up @@ -826,16 +826,50 @@ fn rustflags_from_target(
}
}

/// Gets the user-specified linker for a particular host or target from the configuration.
fn target_linker(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR the target_linker is called when gleaning target info. Before this, it was called during Compilation::new in compile_ws. This may fail some commands like cargo metadata, cargo tree or cargo -Zunstable-options rustc --print due to bad configuration files.

For example, cargo metadata failed with this configs but not before.

[target.'cfg(target_arch = "x86_64")']
linker = "foo"

[target.'cfg(target_os = "linux")']
linker = "foo"

This might not be ideal because those commands don't need target infos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I entirely missed that this was a problem.

I don't see a good solution to this, I'll come back with a fresh set of eyes tomorrow and see if that changes.

Right now my take is it's either this (bad), don't fix this issue at all (also bad), or doing something involving not so clean code to defer actually returning the error (also bad). But I think there's a small chance I can find a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, the current approach is not too bad for normal users.

However, I know some enterprise users have weird multi-level configuration setup. And may move projects among different directories. This is also bad because you cannot opt out directory-probing for configuration files (which Cargo may provide an escape patch?).

gctx: &GlobalContext,
target_cfg: &[Cfg],
target_config: &TargetConfig,
) -> CargoResult<Option<PathBuf>> {
// Try host.linker and target.{}.linker.
if let Some(path) = target_config
.linker
.as_ref()
.map(|l| l.val.clone().resolve_program(gctx))
{
return Ok(Some(path));
}

// Try target.'cfg(...)'.linker.
let mut cfgs = gctx
.target_cfgs()?
.iter()
.filter_map(|(key, cfg)| cfg.linker.as_ref().map(|linker| (key, linker)))
.filter(|(key, _linker)| CfgExpr::matches_key(key, target_cfg));
let matching_linker = cfgs.next();
if let Some((key, linker)) = cfgs.next() {
anyhow::bail!(
"several matching instances of `target.'cfg(..)'.linker` in configurations\n\
first match `{}` located in {}\n\
second match `{}` located in {}",
matching_linker.unwrap().0,
matching_linker.unwrap().1.definition,
key,
linker.definition
);
}
Ok(matching_linker.map(|(_k, linker)| linker.val.clone().resolve_program(gctx)))
}

/// Gets compiler flags from `[host]` section in the config.
/// See [`extra_args`] for more.
fn rustflags_from_host(
gctx: &GlobalContext,
target_config: &TargetConfig,
flag: Flags,
host_triple: &str,
) -> CargoResult<Option<Vec<String>>> {
let target_cfg = gctx.host_cfg_triple(host_triple)?;
let list = match flag {
Flags::Rust => &target_cfg.rustflags,
Flags::Rust => &target_config.rustflags,
Flags::Rustdoc => {
// host.rustdocflags is not a thing, since it does not make sense
return Ok(None);
Expand Down Expand Up @@ -893,22 +927,33 @@ impl<'gctx> RustcTargetData<'gctx> {
let mut target_info = HashMap::new();
let target_applies_to_host = gctx.target_applies_to_host()?;
let host_target = CompileTarget::new(&rustc.host)?;
let host_info = TargetInfo::new(gctx, requested_kinds, &rustc, CompileKind::Host)?;

// This config is used for link overrides and choosing a linker.
let host_config = if target_applies_to_host {
gctx.target_cfg_triple(&rustc.host)?
let mut config = gctx.target_cfg_triple(&rustc.host)?;
if requested_kinds != [CompileKind::Host] {
// When an explicit target flag is passed rustflags are ignored for host artifacts.
config.rustflags = None;
config.rustdocflags = None;
}
Comment on lines +933 to +937
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮‍💨 seems like a bad code smell to me, but I understand it is for maintaining the dual bad behavior that linker is respected but rustflags is not, right?

Copy link
Contributor Author

@gmorenz gmorenz Jul 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly.

And I 100% agree with bad code smell, but this feels less bad to me than the pre-existing reality that host_config is just storing incorrect rustflags in this case. It's one of the things that took me awhile to understand the first time I started working on this code (and why I added that comment on line 943 that I now get to delete).

A more ambitious refactoring option might be to try and push all this logic into something like gctx.host_cfg_triple. I remember trying that when I wrote this and rejecting it, but I don't remember why I rejected it and I'd be happy to give it another shot if you think that it might be cleaner.

config
} else {
gctx.host_cfg_triple(&rustc.host)?
};
let host_info = TargetInfo::new(
gctx,
requested_kinds,
&rustc,
CompileKind::Host,
&host_config,
)?;

// This is a hack. The unit_dependency graph builder "pretends" that
// `CompileKind::Host` is `CompileKind::Target(host)` if the
// `--target` flag is not specified. Since the unit_dependency code
// needs access to the target config data, create a copy so that it
// can be found. See `rebuild_unit_graph_shared` for why this is done.
if requested_kinds.iter().any(CompileKind::is_host) {
target_config.insert(host_target, gctx.target_cfg_triple(&rustc.host)?);
let host_target_config = gctx.target_cfg_triple(&rustc.host)?;

// If target_applies_to_host is true, the host_info is the target info,
// otherwise we need to build target info for the target.
Expand All @@ -920,9 +965,12 @@ impl<'gctx> RustcTargetData<'gctx> {
requested_kinds,
&rustc,
CompileKind::Target(host_target),
&host_target_config,
)?;
target_info.insert(host_target, host_target_info);
}

target_config.insert(host_target, host_target_config);
};

let mut res = RustcTargetData {
Expand Down Expand Up @@ -969,14 +1017,20 @@ impl<'gctx> RustcTargetData<'gctx> {
/// Insert `kind` into our `target_info` and `target_config` members if it isn't present yet.
pub fn merge_compile_kind(&mut self, kind: CompileKind) -> CargoResult<()> {
if let CompileKind::Target(target) = kind {
if !self.target_config.contains_key(&target) {
self.target_config
.insert(target, self.gctx.target_cfg_triple(target.short_name())?);
}
let target_config: &TargetConfig = match self.target_config.entry(target) {
Entry::Occupied(o) => o.into_mut(),
Entry::Vacant(v) => v.insert(self.gctx.target_cfg_triple(target.short_name())?),
};
if !self.target_info.contains_key(&target) {
self.target_info.insert(
target,
TargetInfo::new(self.gctx, &self.requested_kinds, &self.rustc, kind)?,
TargetInfo::new(
self.gctx,
&self.requested_kinds,
&self.rustc,
kind,
target_config,
)?,
);
}
}
Expand Down
1 change: 0 additions & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
unit: unit.clone(),
args,
unstable_opts,
linker: self.compilation.target_linker(unit.kind).clone(),
script_meta,
env: artifact::get_env(&self, self.unit_deps(unit))?,
});
Expand Down
52 changes: 0 additions & 52 deletions src/cargo/core/compiler/compilation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ pub struct Doctest {
pub args: Vec<OsString>,
/// Whether or not -Zunstable-options is needed.
pub unstable_opts: bool,
/// The -Clinker value to use.
pub linker: Option<PathBuf>,
/// The script metadata, if this unit's package has a build script.
///
/// This is used for indexing [`Compilation::extra_env`].
Expand Down Expand Up @@ -120,8 +118,6 @@ pub struct Compilation<'gctx> {
primary_rustc_process: Option<ProcessBuilder>,

target_runners: HashMap<CompileKind, Option<(PathBuf, Vec<String>)>>,
/// The linker to use for each host or target.
target_linkers: HashMap<CompileKind, Option<PathBuf>>,
}

impl<'gctx> Compilation<'gctx> {
Expand Down Expand Up @@ -162,13 +158,6 @@ impl<'gctx> Compilation<'gctx> {
.chain(Some(&CompileKind::Host))
.map(|kind| Ok((*kind, target_runner(bcx, *kind)?)))
.collect::<CargoResult<HashMap<_, _>>>()?,
target_linkers: bcx
.build_config
.requested_kinds
.iter()
.chain(Some(&CompileKind::Host))
.map(|kind| Ok((*kind, target_linker(bcx, *kind)?)))
.collect::<CargoResult<HashMap<_, _>>>()?,
})
}

Expand Down Expand Up @@ -240,11 +229,6 @@ impl<'gctx> Compilation<'gctx> {
self.target_runners.get(&kind).and_then(|x| x.as_ref())
}

/// Gets the user-specified linker for a particular host or target.
pub fn target_linker(&self, kind: CompileKind) -> Option<PathBuf> {
self.target_linkers.get(&kind).and_then(|x| x.clone())
}

/// Returns a [`ProcessBuilder`] appropriate for running a process for the
/// target platform. This is typically used for `cargo run` and `cargo
/// test`.
Expand Down Expand Up @@ -484,39 +468,3 @@ fn target_runner(
)
}))
}

/// Gets the user-specified linker for a particular host or target from the configuration.
fn target_linker(bcx: &BuildContext<'_, '_>, kind: CompileKind) -> CargoResult<Option<PathBuf>> {
// Try host.linker and target.{}.linker.
if let Some(path) = bcx
.target_data
.target_config(kind)
.linker
.as_ref()
.map(|l| l.val.clone().resolve_program(bcx.gctx))
{
return Ok(Some(path));
}

// Try target.'cfg(...)'.linker.
let target_cfg = bcx.target_data.info(kind).cfg();
let mut cfgs = bcx
.gctx
.target_cfgs()?
.iter()
.filter_map(|(key, cfg)| cfg.linker.as_ref().map(|linker| (key, linker)))
.filter(|(key, _linker)| CfgExpr::matches_key(key, target_cfg));
let matching_linker = cfgs.next();
if let Some((key, linker)) = cfgs.next() {
anyhow::bail!(
"several matching instances of `target.'cfg(..)'.linker` in configurations\n\
first match `{}` located in {}\n\
second match `{}` located in {}",
matching_linker.unwrap().0,
matching_linker.unwrap().1.definition,
key,
linker.definition
);
}
Ok(matching_linker.map(|(_k, linker)| linker.val.clone().resolve_program(bcx.gctx)))
}
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/custom_build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,8 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
cmd.env(&var, value);
}

if let Some(linker) = &build_runner.compilation.target_linker(unit.kind) {
cmd.env("RUSTC_LINKER", linker);
if let Some(linker) = &unit.linker {
cmd.env("RUSTC_LINKER", linker.as_ref());
}

if let Some(links) = unit.pkg.manifest().links() {
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/fingerprint/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,8 +1454,8 @@ fn calculate_normal(
let m = unit.pkg.manifest().metadata();
let metadata = util::hash_u64((&m.authors, &m.description, &m.homepage, &m.repository));
let mut config = StableHasher::new();
if let Some(linker) = build_runner.compilation.target_linker(unit.kind) {
linker.hash(&mut config);
if let Some(linker) = &unit.linker {
linker.as_ref().hash(&mut config);
}
if unit.mode.is_doc() && build_runner.bcx.gctx.cli_unstable().rustdoc_map {
if let Ok(map) = build_runner.bcx.gctx.doc_extern_map() {
Expand Down
Loading
Loading