Skip to content

Commit

Permalink
Remove prior hack and replace it with a proper solution
Browse files Browse the repository at this point in the history
This is the diff from
#10433 (comment)
applied to the codebase with the previous hack removed.

Thanks a million to @ehuss who has the insight to find fixes like this.
  • Loading branch information
Byron committed Mar 14, 2022
1 parent 0bede73 commit 4e847a2
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 43 deletions.
12 changes: 3 additions & 9 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -464,10 +464,7 @@ fn compute_deps_custom_build(
// All dependencies of this unit should use profiles for custom builds.
// If this is a build script of a proc macro, make sure it uses host
// features.
let script_unit_for = UnitFor::new_host(
unit_for.is_for_host_features(),
unit_for.root_compile_kind(),
);
let script_unit_for = unit_for.for_custom_build();
// When not overridden, then the dependencies to run a build script are:
//
// 1. Compiling the build script itself.
Expand Down Expand Up @@ -782,7 +779,7 @@ fn dep_build_script(
// The profile stored in the Unit is the profile for the thing
// the custom build script is running for.
let profile = state.profiles.get_profile_run_custom_build(&unit.profile);
// UnitFor::new_host is used because we want the `host` flag set
// UnitFor::for_custom_build is used because we want the `host` flag set
// for all of our build dependencies (so they all get
// build-override profiles), including compiling the build.rs
// script itself.
Expand All @@ -807,10 +804,7 @@ fn dep_build_script(
// compiled twice. I believe it is not feasible to only build it
// once because it would break a large number of scripts (they
// would think they have the wrong set of features enabled).
let script_unit_for = UnitFor::new_host(
unit_for.is_for_host_features(),
unit_for.root_compile_kind(),
);
let script_unit_for = unit_for.for_custom_build();
new_unit_dep_with_profile(
state,
unit,
Expand Down
12 changes: 12 additions & 0 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,18 @@ impl UnitFor {
}
}

pub fn for_custom_build(self) -> UnitFor {
UnitFor {
host: true,
host_features: self.host_features,
// Force build scripts to always use `panic=unwind` for now to
// maximally share dependencies with procedural macros.
panic_setting: PanicSetting::AlwaysUnwind,
root_compile_kind: self.root_compile_kind,
artifact_target_for_features: self.artifact_target_for_features,
}
}

/// Set the artifact compile target for use in features using the given `artifact`.
pub(crate) fn with_artifact_features(mut self, artifact: &Artifact) -> UnitFor {
self.artifact_target_for_features = artifact.target().and_then(|t| t.to_compile_target());
Expand Down
44 changes: 15 additions & 29 deletions src/cargo/core/resolver/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -761,29 +761,25 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
) -> Vec<(PackageId, Vec<(&'a Dependency, FeaturesFor)>)> {
// Helper for determining if a platform is activated.
let platform_activated = |dep: &Dependency| -> bool {
// We always care about build-dependencies, and they are always
// Host. If we are computing dependencies "for a build script",
// even normal dependencies are host-only.
if fk == FeaturesFor::HostDep || dep.is_build() {
return self
.target_data
.dep_platform_activated(dep, CompileKind::Host);
}
// We always count platforms as activated if the target stems from an artifact
// dependency's target specification. This triggers in conjunction with
// `[target.'cfg(…)'.dependencies]` manifest sections.
if let FeaturesFor::NormalOrDevOrArtifactTarget(Some(target)) = fk {
if self
.target_data
.dep_platform_activated(dep, CompileKind::Target(target))
{
return true;
match (dep.is_build(), fk) {
(true, _) | (_, FeaturesFor::HostDep) => {
// We always care about build-dependencies, and they are always
// Host. If we are computing dependencies "for a build script",
// even normal dependencies are host-only.
self.target_data
.dep_platform_activated(dep, CompileKind::Host)
}
(_, FeaturesFor::NormalOrDevOrArtifactTarget(None)) => self
.requested_targets
.iter()
.any(|kind| self.target_data.dep_platform_activated(dep, *kind)),
(_, FeaturesFor::NormalOrDevOrArtifactTarget(Some(target))) => self
.target_data
.dep_platform_activated(dep, CompileKind::Target(target)),
}
// Not a build dependency, and not for a build script, so must be Target.
self.requested_targets
.iter()
.any(|kind| self.target_data.dep_platform_activated(dep, *kind))
};
self.resolve
.deps(pkg_id)
Expand Down Expand Up @@ -857,7 +853,7 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
)
});

let mut dep_fks = match artifact_target_keys {
let dep_fks = match artifact_target_keys {
// The artifact is also a library and does specify custom
// targets.
// The library's feature key needs to be used alongside
Expand All @@ -873,16 +869,6 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
// Use the standard feature key without any alteration.
Some((_, None)) | None => vec![lib_fk],
};

// This is more of a hack to fix a particular issue with platform-gated
// dependencies' build scripts, which unfortunately we can't determine
// here any better than checking for a platform and blindly adding the
// feature key that it will later query.
// If it matters, the dependency that actually should add this key
// drops out in line 798.
if dep.platform().is_some() {
dep_fks.push(FeaturesFor::NormalOrDevOrArtifactTarget(None));
}
dep_fks.into_iter().map(move |dep_fk| (dep, dep_fk))
})
.collect::<Vec<_>>();
Expand Down
9 changes: 4 additions & 5 deletions tests/testsuite/artifact_dep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,6 @@ fn feature_resolution_works_for_cfg_target_specification() {
return;
}
let target = cross_compile::alternate();
let target_arch = cross_compile::alternate_arch();
let p = project()
.file(
"Cargo.toml",
Expand Down Expand Up @@ -465,10 +464,10 @@ fn feature_resolution_works_for_cfg_target_specification() {
version = "0.0.1"
authors = []
[target.'cfg(target_arch = "$ARCH")'.dependencies]
[target.'$TARGET'.dependencies]
d2 = { path = "../d2" }
"#
.replace("$ARCH", target_arch),
.replace("$TARGET", target),
)
.file(
"d1/src/main.rs",
Expand All @@ -480,11 +479,11 @@ fn feature_resolution_works_for_cfg_target_specification() {
.file(
"d1/src/lib.rs",
&r#"pub fn f() {
#[cfg(target_arch = "$ARCH")]
#[cfg(target = "$TARGET")]
d2::f();
}
"#
.replace("$ARCH", target_arch),
.replace("$TARGET", target),
)
.file(
"d2/Cargo.toml",
Expand Down

0 comments on commit 4e847a2

Please sign in to comment.