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.

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

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
77 changes: 69 additions & 8 deletions src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,6 +161,11 @@ impl TargetInfo {
requested_kinds: &[CompileKind],
rustc: &Rustc,
kind: CompileKind,
// This config is used for `links_overrides` and `linker`.
//
// In the case of target_applies_to_host=true (default) it may contain
// incorrect rustflags.
target_config: &TargetConfig,
) -> CargoResult<TargetInfo> {
let mut rustflags =
extra_args(gctx, requested_kinds, &rustc.host, None, kind, Flags::Rust)?;
Expand Down Expand Up @@ -321,6 +330,7 @@ impl TargetInfo {
Flags::Rustdoc,
)?
.into(),
linker: target_linker(gctx, &cfg, target_config)?.map(Rc::new),
cfg,
support_split_debuginfo,
support_check_cfg,
Expand Down Expand Up @@ -826,6 +836,42 @@ 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(
Expand Down Expand Up @@ -893,22 +939,28 @@ 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)?
} 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 +972,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 +1024,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
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
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
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
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
6 changes: 1 addition & 5 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1156,11 +1156,7 @@ fn build_base_args(
cmd,
"-C",
"linker=",
build_runner
.compilation
.target_linker(unit.kind)
.as_ref()
.map(|s| s.as_ref()),
unit.linker.as_ref().map(|s| s.as_ref().as_ref()),
);
if incremental {
let dir = build_runner
Expand Down
1 change: 1 addition & 0 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ pub fn generate_std_roots(
target_data.info(*kind).rustflags.clone(),
target_data.info(*kind).rustdocflags.clone(),
target_data.target_config(*kind).links_overrides.clone(),
target_data.info(*kind).linker.clone(),
/*is_std*/ true,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
6 changes: 6 additions & 0 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use std::collections::{BTreeMap, HashSet};
use std::fmt;
use std::hash::{Hash, Hasher};
use std::ops::Deref;
use std::path::PathBuf;
use std::rc::Rc;
use std::sync::Arc;

Expand Down Expand Up @@ -90,6 +91,8 @@ pub struct UnitInner {
/// running its build script and instead use the given output from the
/// config file.
pub links_overrides: Rc<BTreeMap<String, BuildOutput>>,
/// Linker (if any) to pass to `rustc`, if not specified rustc chooses a default.
pub linker: Option<Rc<PathBuf>>,
// if `true`, the dependency is an artifact dependency, requiring special handling when
// calculating output directories, linkage and environment variables provided to builds.
pub artifact: IsArtifact,
Expand Down Expand Up @@ -185,6 +188,7 @@ impl fmt::Debug for Unit {
.field("rustflags", &self.rustflags)
.field("rustdocflags", &self.rustdocflags)
.field("links_overrides", &self.links_overrides)
.field("linker", &self.linker)
.field("artifact", &self.artifact.is_true())
.field(
"artifact_target_for_features",
Expand Down Expand Up @@ -235,6 +239,7 @@ impl UnitInterner {
rustflags: Arc<[String]>,
rustdocflags: Arc<[String]>,
links_overrides: Rc<BTreeMap<String, BuildOutput>>,
linker: Option<Rc<PathBuf>>,
is_std: bool,
dep_hash: u64,
artifact: IsArtifact,
Expand Down Expand Up @@ -271,6 +276,7 @@ impl UnitInterner {
rustflags,
rustdocflags,
links_overrides,
linker,
is_std,
dep_hash,
artifact,
Expand Down
1 change: 1 addition & 0 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,7 @@ fn new_unit_dep_with_profile(
.target_config(kind)
.links_overrides
.clone(),
state.target_data.info(kind).linker.clone(),
state.is_std,
/*dep_hash*/ 0,
artifact.map_or(IsArtifact::No, |_| IsArtifact::Yes),
Expand Down
15 changes: 14 additions & 1 deletion src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,17 @@ pub fn print<'a>(
if index != 0 {
drop_println!(gctx);
}
let target_info = TargetInfo::new(gctx, &build_config.requested_kinds, &rustc, *kind)?;
let target_config = match kind {
CompileKind::Host => gctx.target_cfg_triple(&rustc.host),
CompileKind::Target(target) => gctx.target_cfg_triple(target.short_name()),
}?;
let target_info = TargetInfo::new(
gctx,
&build_config.requested_kinds,
&rustc,
*kind,
&target_config,
)?;
let mut process = rustc.process();
process.args(&target_info.rustflags);
if let Some(args) = target_rustc_args {
Expand Down Expand Up @@ -699,6 +709,7 @@ fn traverse_and_share(
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.clone(),
unit.linker.clone(),
unit.is_std,
unit.dep_hash,
unit.artifact,
Expand Down Expand Up @@ -727,6 +738,7 @@ fn traverse_and_share(
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.clone(),
unit.linker.clone(),
unit.is_std,
new_dep_hash,
unit.artifact,
Expand Down Expand Up @@ -891,6 +903,7 @@ fn override_rustc_crate_types(
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.clone(),
unit.linker.clone(),
unit.is_std,
unit.dep_hash,
unit.artifact,
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_compile/unit_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ impl<'a> UnitGenerator<'a, '_> {
self.target_data.info(kind).rustflags.clone(),
self.target_data.info(kind).rustdocflags.clone(),
self.target_data.target_config(kind).links_overrides.clone(),
self.target_data.info(kind).linker.clone(),
/*is_std*/ false,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
Loading