Skip to content

Fix passing of links-overrides with target-applies-to-host and an implicit target #14205

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

Merged
merged 2 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 1 addition & 11 deletions src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@
//! * [`TargetInfo::rustc_outputs`] to get a list of supported file types.

use crate::core::compiler::apply_env_config;
use crate::core::compiler::{
BuildOutput, BuildRunner, CompileKind, CompileMode, CompileTarget, CrateType,
};
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::interning::InternedString;
Expand Down Expand Up @@ -1038,14 +1036,6 @@ impl<'gctx> RustcTargetData<'gctx> {
CompileKind::Target(s) => &self.target_config[&s],
}
}

/// If a build script is overridden, this returns the `BuildOutput` to use.
///
/// `lib_name` is the `links` library name and `kind` is whether it is for
/// Host or Target.
pub fn script_override(&self, lib_name: &str, kind: CompileKind) -> Option<&BuildOutput> {
self.target_config(kind).links_overrides.get(lib_name)
}
}

/// Structure used to deal with Rustdoc fingerprinting
Expand Down
10 changes: 3 additions & 7 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ const OLD_CARGO_WARNING_SYNTAX: &str = "cargo:warning=";
/// [the doc]: https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargo-warning
const NEW_CARGO_WARNING_SYNTAX: &str = "cargo::warning=";
/// Contains the parsed output of a custom build script.
#[derive(Clone, Debug, Hash, Default)]
#[derive(Clone, Debug, Hash, Default, PartialEq, Eq, PartialOrd, Ord)]
Copy link
Member

Choose a reason for hiding this comment

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

Also realized that there is an implication that these newly added fields would affect the order of any sorted units list. I don't think that would cause real issues though.

Copy link
Contributor Author

@gmorenz gmorenz Jul 16, 2024

Choose a reason for hiding this comment

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

True... if this is a significant concern we could move the new fields to the end.

Ultimately I don't think I have the context to decide if it is. I think it is almost certainly publicly exposed given comments like this one, and where these fields currently are in the struct is probably high enough to change the order in the cases described in this comment. I wouldn't have thought that the ordering of units was something that was guaranteed to be stable between cargo versions.

I'll hold off on updating the rustflags Arc to Rc until we've decided if we need to move it to the bottom since that might as well be one PR.

pub struct BuildOutput {
/// Paths to pass to rustc with the `-L` flag.
pub library_paths: Vec<PathBuf>,
Expand Down Expand Up @@ -160,7 +160,7 @@ pub struct BuildDeps {
/// See the [build script documentation][1] for more.
///
/// [1]: https://doc.rust-lang.org/nightly/cargo/reference/build-scripts.html#cargorustc-link-argflag
#[derive(Clone, Hash, Debug, PartialEq, Eq)]
#[derive(Clone, Hash, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum LinkArgTarget {
/// Represents `cargo::rustc-link-arg=FLAG`.
All,
Expand Down Expand Up @@ -1168,11 +1168,7 @@ pub fn build_map(build_runner: &mut BuildRunner<'_, '_>) -> CargoResult<()> {
// If there is a build script override, pre-fill the build output.
if unit.mode.is_run_custom_build() {
if let Some(links) = unit.pkg.manifest().links() {
if let Some(output) = build_runner
.bcx
.target_data
.script_override(links, unit.kind)
{
if let Some(output) = unit.links_overrides.get(links) {
let metadata = build_runner.get_run_build_script_metadata(unit);
build_runner.build_script_outputs.lock().unwrap().insert(
unit.pkg.package_id(),
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 @@ -219,6 +219,7 @@ pub fn generate_std_roots(
features.clone(),
target_data.info(*kind).rustflags.clone(),
target_data.info(*kind).rustdocflags.clone(),
target_data.target_config(*kind).links_overrides.clone(),
/*is_std*/ true,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
13 changes: 12 additions & 1 deletion src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ use crate::util::hex::short_hash;
use crate::util::interning::InternedString;
use crate::util::GlobalContext;
use std::cell::RefCell;
use std::collections::HashSet;
use std::collections::{BTreeMap, HashSet};
use std::fmt;
use std::hash::{Hash, Hasher};
use std::ops::Deref;
use std::rc::Rc;
use std::sync::Arc;

use super::BuildOutput;

/// All information needed to define a unit.
///
/// A unit is an object that has enough information so that cargo knows how to build it.
Expand Down Expand Up @@ -82,6 +84,12 @@ pub struct UnitInner {
/// [`BuildContext::extra_args_for`]: crate::core::compiler::build_context::BuildContext::extra_args_for
/// [`TargetInfo.rustdocflags`]: crate::core::compiler::build_context::TargetInfo::rustdocflags
pub rustdocflags: Arc<[String]>,
/// Build script override for the given library name.
///
/// Any package with a `links` value for the given library name will skip
/// running its build script and instead use the given output from the
/// config file.
pub links_overrides: Rc<BTreeMap<String, BuildOutput>>,
// 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 @@ -176,6 +184,7 @@ impl fmt::Debug for Unit {
.field("features", &self.features)
.field("rustflags", &self.rustflags)
.field("rustdocflags", &self.rustdocflags)
.field("links_overrides", &self.links_overrides)
.field("artifact", &self.artifact.is_true())
.field(
"artifact_target_for_features",
Expand Down Expand Up @@ -225,6 +234,7 @@ impl UnitInterner {
features: Vec<InternedString>,
rustflags: Arc<[String]>,
rustdocflags: Arc<[String]>,
links_overrides: Rc<BTreeMap<String, BuildOutput>>,
is_std: bool,
dep_hash: u64,
artifact: IsArtifact,
Expand Down Expand Up @@ -260,6 +270,7 @@ impl UnitInterner {
features,
rustflags,
rustdocflags,
links_overrides,
is_std,
dep_hash,
artifact,
Expand Down
11 changes: 6 additions & 5 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,11 +457,7 @@ fn compute_deps_custom_build(
state: &State<'_, '_>,
) -> CargoResult<Vec<UnitDep>> {
if let Some(links) = unit.pkg.manifest().links() {
if state
.target_data
.script_override(links, unit.kind)
.is_some()
{
if unit.links_overrides.get(links).is_some() {
// Overridden build scripts don't have any dependencies.
return Ok(Vec::new());
}
Expand Down Expand Up @@ -861,6 +857,11 @@ fn new_unit_dep_with_profile(
features,
state.target_data.info(kind).rustflags.clone(),
state.target_data.info(kind).rustdocflags.clone(),
state
.target_data
.target_config(kind)
.links_overrides
.clone(),
state.is_std,
/*dep_hash*/ 0,
artifact.map_or(IsArtifact::No, |_| IsArtifact::Yes),
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,7 @@ fn traverse_and_share(
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.clone(),
unit.is_std,
unit.dep_hash,
unit.artifact,
Expand Down Expand Up @@ -725,6 +726,7 @@ fn traverse_and_share(
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.clone(),
unit.is_std,
new_dep_hash,
unit.artifact,
Expand Down Expand Up @@ -888,6 +890,7 @@ fn override_rustc_crate_types(
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.links_overrides.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 @@ -173,6 +173,7 @@ impl<'a> UnitGenerator<'a, '_> {
features.clone(),
self.target_data.info(kind).rustflags.clone(),
self.target_data.info(kind).rustdocflags.clone(),
self.target_data.target_config(kind).links_overrides.clone(),
/*is_std*/ false,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/util/context/target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::util::CargoResult;
use serde::Deserialize;
use std::collections::{BTreeMap, HashMap};
use std::path::PathBuf;
use std::rc::Rc;

/// Config definition of a `[target.'cfg(…)']` table.
///
Expand Down Expand Up @@ -36,7 +37,7 @@ pub struct TargetConfig {
/// Any package with a `links` value for the given library name will skip
/// running its build script and instead use the given output from the
/// config file.
pub links_overrides: BTreeMap<String, BuildOutput>,
pub links_overrides: Rc<BTreeMap<String, BuildOutput>>,
}

/// Loads all of the `target.'cfg()'` tables.
Expand Down Expand Up @@ -128,7 +129,7 @@ fn load_config_table(gctx: &GlobalContext, prefix: &str) -> CargoResult<TargetCo
rustflags,
rustdocflags,
linker,
links_overrides,
links_overrides: Rc::new(links_overrides),
})
}

Expand Down
38 changes: 38 additions & 0 deletions tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5777,3 +5777,41 @@ hello
"#]])
.run();
}

#[cargo_test]
fn links_overrides_with_target_applies_to_host() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "mylib-sys"
edition = "2021"
version = "0.0.1"
authors = []
links = "mylib"
"#,
)
.file("src/lib.rs", "")
.file("build.rs", "bad file")
.build();

p.cargo("build -v")
.masquerade_as_nightly_cargo(&["target-applies-to-host"])
.args(&[
"-Ztarget-applies-to-host",
"--config",
"target-applies-to-host=false",
])
.args(&[
"--config",
&format!(r#"target.{}.mylib.rustc-link-search=["foo"]"#, rustc_host()),
])
.with_stderr_data(str![[r#"
[COMPILING] mylib-sys v0.0.1 ([ROOT]/foo)
[RUNNING] `rustc --crate-name mylib_sys [..] -L foo`
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
.run();
}