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

Pass rustflags to artifacts built with implicit targets when using target-applies-to-host #13900

Merged
merged 3 commits into from
Jul 4, 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
26 changes: 0 additions & 26 deletions src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,32 +134,6 @@ impl<'a, 'gctx> BuildContext<'a, 'gctx> {
self.build_config.jobs
}

/// Extra compiler flags to pass to `rustc` for a given unit.
///
/// Although it depends on the caller, in the current Cargo implementation,
/// these flags take precedence over those from [`BuildContext::extra_args_for`].
///
/// As of now, these flags come from environment variables and configurations.
/// See [`TargetInfo.rustflags`] for more on how Cargo collects them.
///
/// [`TargetInfo.rustflags`]: TargetInfo::rustflags
pub fn rustflags_args(&self, unit: &Unit) -> &[String] {
&self.target_data.info(unit.kind).rustflags
}

/// Extra compiler flags to pass to `rustdoc` for a given unit.
///
/// Although it depends on the caller, in the current Cargo implementation,
/// these flags take precedence over those from [`BuildContext::extra_args_for`].
///
/// As of now, these flags come from environment variables and configurations.
/// See [`TargetInfo.rustdocflags`] for more on how Cargo collects them.
///
/// [`TargetInfo.rustdocflags`]: TargetInfo::rustdocflags
pub fn rustdocflags_args(&self, unit: &Unit) -> &[String] {
&self.target_data.info(unit.kind).rustdocflags
}

/// Extra compiler args for either `rustc` or `rustdoc`.
///
/// As of now, these flags come from the trailing args of either
Expand Down
36 changes: 28 additions & 8 deletions src/cargo/core/compiler/build_context/target_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use std::cell::RefCell;
use std::collections::hash_map::{Entry, HashMap};
use std::path::{Path, PathBuf};
use std::str::{self, FromStr};
use std::sync::Arc;

/// Information about the platform target gleaned from querying rustc.
///
Expand Down Expand Up @@ -52,9 +53,9 @@ pub struct TargetInfo {
/// target libraries.
pub sysroot_target_libdir: PathBuf,
/// Extra flags to pass to `rustc`, see [`extra_args`].
pub rustflags: Vec<String>,
pub rustflags: Arc<[String]>,
/// Extra flags to pass to `rustdoc`, see [`extra_args`].
pub rustdocflags: Vec<String>,
pub rustdocflags: Arc<[String]>,
/// 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 @@ -312,15 +313,16 @@ impl TargetInfo {
crate_types: RefCell::new(map),
sysroot,
sysroot_target_libdir,
rustflags,
rustflags: rustflags.into(),
rustdocflags: extra_args(
gctx,
requested_kinds,
&rustc.host,
Some(&cfg),
kind,
Flags::Rustdoc,
)?,
)?
.into(),
cfg,
support_split_debuginfo,
support_check_cfg,
Expand Down Expand Up @@ -867,7 +869,10 @@ pub struct RustcTargetData<'gctx> {

/// Build information for the "host", which is information about when
/// `rustc` is invoked without a `--target` flag. This is used for
/// procedural macros, build scripts, etc.
/// selecting a linker, and applying link overrides.
///
/// The configuration read into this depends on whether or not
/// `target-applies-to-host=true`.
host_config: TargetConfig,
/// Information about the host platform.
host_info: TargetInfo,
Expand All @@ -889,7 +894,10 @@ impl<'gctx> RustcTargetData<'gctx> {
let mut target_config = HashMap::new();
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 {
Expand All @@ -902,9 +910,21 @@ impl<'gctx> RustcTargetData<'gctx> {
// 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) {
let ct = CompileTarget::new(&rustc.host)?;
target_info.insert(ct, host_info.clone());
target_config.insert(ct, gctx.target_cfg_triple(&rustc.host)?);
target_config.insert(host_target, gctx.target_cfg_triple(&rustc.host)?);
Copy link
Member

Choose a reason for hiding this comment

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

This looks correct.

Would you mind sharing why we need the entire infra change (Arc and move out rustflags from targetinfo), instead of just the change here?

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 question is basically what "How it is implemented" tries to answer in the original comment on this PR. To take another stab at explaining it here:

The CompileKind for artifact Unit starts out as CompileKind::Target(host_target) (assuming we're not cross compiling). The CompileKind for host Unit starts out as CompileKind::Host

This change sets up target_info so that lookups for CompileKind::Target(host_target) get the correct rustflags (as well as units with CompileKind::Host, which need different rustflags). Which is great provided we lookup artifact Unit's with their original CompileKind.

rebuild_unit_graph_shared changes artifact Units with CompileKind::Target(host_target) to have CompileKind::Host. It does this so that we can deduplicate identical otherwise identical units and only compile them once*.

The current infrastructure attempts to look up rustflags via kind after rebuild_unit_graph_shared. Thus even though we have correctly set up the target_info for CompileKind::Target(host_target) we're going to look up the flags in host_info, which are wrong. The infrastructure change is to push this lookup forwards to before we rebuild the unit with a different kind. It is also necessary because we deduplicate Units based on their hash, so even if we could somehow lookup the correct rustflags later, there wouldn't necessarily be a unique set of them (in the case where host and target artifacts have different rustflags, and a shared dependency).

* To draw your attention to the table at the top. This is why no --target flag, target_applies_to_host=false has "Without rustflags, built with 5 invocations" rather than 6. A dependency shared between the host artifacts and the target artifacts only has to be built once because it is built the exact same way for both of them.


However this question caused me to double check that my understanding was correct by... trying it. I believe it is, however it turns out my test is broken and will (incorrectly) pass (fail to compile) because passing an invalid rustflag will cause TargetInfo::new(host_target) to fail because that command calls rustc with rustflags to query file-names/sysroot/... even though we never build a target with those rustflags.

I'll figure out how to write a version of that test without the false-pass on the partial change and push it later tonight.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I like the idea that Unit holds the exact rustflags it is going to invoke. It opens a new door that we might be able to use this information to create the acutal “build plans” for external systems.

There's only one nit left and I'll merge this :)


// If target_applies_to_host is true, the host_info is the target info,
// otherwise we need to build target info for the target.
if target_applies_to_host {
target_info.insert(host_target, host_info.clone());
} else {
let host_target_info = TargetInfo::new(
gctx,
requested_kinds,
&rustc,
CompileKind::Target(host_target),
)?;
target_info.insert(host_target, host_target_info);
}
};

let mut res = RustcTargetData {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
}
}
}
args.extend(self.bcx.rustdocflags_args(unit).iter().map(Into::into));
args.extend(unit.rustdocflags.iter().map(Into::into));

use super::MessageFormat;
let format = match self.bcx.build_config.message_format {
Expand Down
5 changes: 1 addition & 4 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
cmd.env("RUSTC_WORKSPACE_WRAPPER", wrapper);
}
}
cmd.env(
"CARGO_ENCODED_RUSTFLAGS",
bcx.rustflags_args(unit).join("\x1f"),
);
cmd.env("CARGO_ENCODED_RUSTFLAGS", unit.rustflags.join("\x1f"));
cmd.env_remove("RUSTFLAGS");

if build_runner.bcx.ws.gctx().extra_verbose() {
Expand Down
8 changes: 4 additions & 4 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1415,9 +1415,9 @@ fn calculate_normal(
// hashed to take up less space on disk as we just need to know when things
// change.
let extra_flags = if unit.mode.is_doc() || unit.mode.is_doc_scrape() {
build_runner.bcx.rustdocflags_args(unit)
&unit.rustdocflags
} else {
build_runner.bcx.rustflags_args(unit)
&unit.rustflags
}
.to_vec();

Expand Down Expand Up @@ -1512,7 +1512,7 @@ fn calculate_run_custom_build(
An I/O error happened. Please make sure you can access the file.

By default, if your project contains a build script, cargo scans all files in
it to determine whether a rebuild is needed. If you don't expect to access the
it to determine whether a rebuild is needed. If you don't expect to access the
file, specify `rerun-if-changed` in your build script.
See https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed for more information.";
pkg_fingerprint(build_runner.bcx, &unit.pkg).map_err(|err| {
Expand Down Expand Up @@ -1542,7 +1542,7 @@ See https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-change
.collect::<CargoResult<Vec<_>>>()?
};

let rustflags = build_runner.bcx.rustflags_args(unit).to_vec();
let rustflags = unit.rustflags.to_vec();

Ok(Fingerprint {
local: Mutex::new(local),
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ fn prepare_rustc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResult
base.inherit_jobserver(&build_runner.jobserver);
build_deps_args(&mut base, build_runner, unit)?;
add_cap_lints(build_runner.bcx, unit, &mut base);
base.args(build_runner.bcx.rustflags_args(unit));
base.args(&unit.rustflags);
if build_runner.bcx.gctx.cli_unstable().binary_dep_depinfo {
base.arg("-Z").arg("binary-dep-depinfo");
}
Expand Down Expand Up @@ -780,7 +780,7 @@ fn prepare_rustdoc(build_runner: &BuildRunner<'_, '_>, unit: &Unit) -> CargoResu

rustdoc::add_output_format(build_runner, unit, &mut rustdoc)?;

rustdoc.args(bcx.rustdocflags_args(unit));
rustdoc.args(&unit.rustdocflags);

if !crate_version_flag_already_present(&rustdoc) {
append_crate_version_flag(unit, &mut rustdoc);
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/compiler/standard_lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ pub fn generate_std_roots(
package_set: &PackageSet<'_>,
interner: &UnitInterner,
profiles: &Profiles,
target_data: &RustcTargetData<'_>,
) -> CargoResult<HashMap<CompileKind, Vec<Unit>>> {
// Generate the root Units for the standard library.
let std_ids = crates
Expand Down Expand Up @@ -216,6 +217,8 @@ pub fn generate_std_roots(
*kind,
mode,
features.clone(),
target_data.info(*kind).rustflags.clone(),
target_data.info(*kind).rustdocflags.clone(),
/*is_std*/ true,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
28 changes: 28 additions & 0 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::fmt;
use std::hash::{Hash, Hasher};
use std::ops::Deref;
use std::rc::Rc;
use std::sync::Arc;

/// All information needed to define a unit.
///
Expand Down Expand Up @@ -59,6 +60,28 @@ pub struct UnitInner {
/// The `cfg` features to enable for this unit.
/// This must be sorted.
pub features: Vec<InternedString>,
/// Extra compiler flags to pass to `rustc` for a given unit.
///
/// Although it depends on the caller, in the current Cargo implementation,
/// these flags take precedence over those from [`BuildContext::extra_args_for`].
///
/// As of now, these flags come from environment variables and configurations.
/// See [`TargetInfo.rustflags`] for more on how Cargo collects them.
///
/// [`BuildContext::extra_args_for`]: crate::core::compiler::build_context::BuildContext::extra_args_for
/// [`TargetInfo.rustflags`]: crate::core::compiler::build_context::TargetInfo::rustflags
pub rustflags: Arc<[String]>,
/// Extra compiler flags to pass to `rustdoc` for a given unit.
///
/// Although it depends on the caller, in the current Cargo implementation,
/// these flags take precedence over those from [`BuildContext::extra_args_for`].
///
/// As of now, these flags come from environment variables and configurations.
/// See [`TargetInfo.rustdocflags`] for more on how Cargo collects them.
///
/// [`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]>,
// 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 @@ -151,6 +174,7 @@ impl fmt::Debug for Unit {
.field("kind", &self.kind)
.field("mode", &self.mode)
.field("features", &self.features)
.field("rustflags", &self.rustflags)
.field("artifact", &self.artifact.is_true())
.field(
"artifact_target_for_features",
Expand Down Expand Up @@ -198,6 +222,8 @@ impl UnitInterner {
kind: CompileKind,
mode: CompileMode,
features: Vec<InternedString>,
rustflags: Arc<[String]>,
rustdocflags: Arc<[String]>,
is_std: bool,
dep_hash: u64,
artifact: IsArtifact,
Expand Down Expand Up @@ -231,6 +257,8 @@ impl UnitInterner {
kind,
mode,
features,
rustflags,
rustdocflags,
is_std,
dep_hash,
artifact,
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,8 @@ fn new_unit_dep_with_profile(
kind,
mode,
features,
state.target_data.info(kind).rustflags.clone(),
state.target_data.info(kind).rustdocflags.clone(),
state.is_std,
/*dep_hash*/ 0,
artifact.map_or(IsArtifact::No, |_| IsArtifact::Yes),
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ pub fn create_bcx<'a, 'gctx>(
let generator = UnitGenerator {
ws,
packages: &to_builds,
target_data: &target_data,
filter,
requested_kinds: &build_config.requested_kinds,
explicit_host_kind,
Expand Down Expand Up @@ -399,6 +400,7 @@ pub fn create_bcx<'a, 'gctx>(
&pkg_set,
interner,
&profiles,
&target_data,
)?
} else {
Default::default()
Expand Down Expand Up @@ -694,6 +696,8 @@ fn traverse_and_share(
to_host.unwrap(),
unit.mode,
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.is_std,
unit.dep_hash,
unit.artifact,
Expand All @@ -719,6 +723,8 @@ fn traverse_and_share(
canonical_kind,
unit.mode,
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.is_std,
new_dep_hash,
unit.artifact,
Expand Down Expand Up @@ -880,6 +886,8 @@ fn override_rustc_crate_types(
unit.kind,
unit.mode,
unit.features.clone(),
unit.rustflags.clone(),
unit.rustdocflags.clone(),
unit.is_std,
unit.dep_hash,
unit.artifact,
Expand Down
8 changes: 6 additions & 2 deletions src/cargo/ops/cargo_compile/unit_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use std::fmt::Write;

use crate::core::compiler::rustdoc::RustdocScrapeExamples;
use crate::core::compiler::unit_dependencies::IsArtifact;
use crate::core::compiler::UnitInterner;
use crate::core::compiler::{CompileKind, CompileMode, Unit};
use crate::core::compiler::{RustcTargetData, UnitInterner};
use crate::core::dependency::DepKind;
use crate::core::profiles::{Profiles, UnitFor};
use crate::core::resolver::features::{self, FeaturesFor};
Expand Down Expand Up @@ -47,6 +47,7 @@ struct Proposal<'a> {
pub(super) struct UnitGenerator<'a, 'gctx> {
pub ws: &'a Workspace<'gctx>,
pub packages: &'a [&'a Package],
pub target_data: &'a RustcTargetData<'gctx>,
pub filter: &'a CompileFilter,
pub requested_kinds: &'a [CompileKind],
pub explicit_host_kind: CompileKind,
Expand Down Expand Up @@ -162,13 +163,16 @@ impl<'a> UnitGenerator<'a, '_> {
unit_for,
kind,
);
let kind = kind.for_target(target);
self.interner.intern(
pkg,
target,
profile,
kind.for_target(target),
kind,
target_mode,
features.clone(),
self.target_data.info(kind).rustflags.clone(),
self.target_data.info(kind).rustdocflags.clone(),
/*is_std*/ false,
/*dep_hash*/ 0,
IsArtifact::No,
Expand Down
Loading