Skip to content

Commit

Permalink
Add fake dependency roots for all transitive proc-macros
Browse files Browse the repository at this point in the history
See the extensive comment for details, but tl;dr: when cross-compiling,
just doing top-level resolves of direct dependencies can miss resolving
platform-specific features for proc-macros which are only used on
certain platforms.
  • Loading branch information
illicitonion committed Jul 30, 2024
1 parent d1e84e3 commit 3cc0ab7
Show file tree
Hide file tree
Showing 9 changed files with 12,374 additions and 20 deletions.
2 changes: 1 addition & 1 deletion crate_universe/src/cli/splice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub fn splice(opt: SpliceOptions) -> Result<()> {

// Splice together the manifest
let manifest_path = splicer
.splice_workspace(&opt.cargo)
.splice_workspace(&Cargo::new(opt.cargo.clone()))
.context("Failed to splice workspace")?;

let cargo = Cargo::new(opt.cargo);
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/cli/vendor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ pub fn vendor(opt: VendorOptions) -> Result<()> {

// Splice together the manifest
let manifest_path = splicer
.splice_workspace(&opt.cargo)
.splice_workspace(&Cargo::new(opt.cargo.clone()))
.context("Failed to splice workspace")?;

let cargo = Cargo::new(opt.cargo);
Expand Down
2 changes: 1 addition & 1 deletion crate_universe/src/splicing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl WorkspaceMetadata {
workspace_metaata.tree_metadata = resolver_data;
workspace_metaata.inject_into(&mut manifest)?;

write_root_manifest(output_manifest_path, manifest)?;
write_root_manifest(output_manifest_path, manifest, cargo)?;

Ok(())
}
Expand Down
88 changes: 74 additions & 14 deletions crate_universe/src/splicing/splicer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use cargo_toml::{Dependency, Manifest};
use normpath::PathExt;

use crate::config::CrateId;
use crate::splicing::{SplicedManifest, SplicingManifest};
use crate::splicing::{Cargo, SplicedManifest, SplicingManifest};
use crate::utils::starlark::Label;

use super::{read_manifest, DirectPackageManifest, WorkspaceMetadata};
Expand Down Expand Up @@ -45,7 +45,7 @@ impl<'a> SplicerKind<'a> {
pub(crate) fn new(
manifests: &'a BTreeMap<PathBuf, Manifest>,
splicing_manifest: &'a SplicingManifest,
cargo: &Path,
cargo_bin: &Cargo,
) -> Result<Self> {
// First check for any workspaces in the provided manifests
let workspace_owned: BTreeMap<&PathBuf, &Manifest> = manifests
Expand Down Expand Up @@ -75,8 +75,8 @@ impl<'a> SplicerKind<'a> {
if workspace_roots.is_empty() {
let sorted_manifests: BTreeSet<_> = manifests.keys().collect();
for manifest_path in sorted_manifests {
let metadata_result = MetadataCommand::new()
.cargo_path(cargo)
let metadata_result = cargo_bin
.metadata_command()?
.current_dir(manifest_path.parent().unwrap())
.manifest_path(manifest_path)
.no_deps()
Expand Down Expand Up @@ -186,22 +186,22 @@ impl<'a> SplicerKind<'a> {

/// Performs splicing based on the current variant.
#[tracing::instrument(skip_all)]
pub(crate) fn splice(&self, workspace_dir: &Path) -> Result<SplicedManifest> {
pub(crate) fn splice(&self, workspace_dir: &Path, cargo: &Cargo) -> Result<SplicedManifest> {
match self {
SplicerKind::Workspace {
path,
manifest,
splicing_manifest,
} => Self::splice_workspace(workspace_dir, path, manifest, splicing_manifest),
} => Self::splice_workspace(workspace_dir, path, manifest, splicing_manifest, cargo),
SplicerKind::Package {
path,
manifest,
splicing_manifest,
} => Self::splice_package(workspace_dir, path, manifest, splicing_manifest),
} => Self::splice_package(workspace_dir, path, manifest, splicing_manifest, cargo),
SplicerKind::MultiPackage {
manifests,
splicing_manifest,
} => Self::splice_multi_package(workspace_dir, manifests, splicing_manifest),
} => Self::splice_multi_package(workspace_dir, manifests, splicing_manifest, cargo),
}
}

Expand All @@ -212,6 +212,7 @@ impl<'a> SplicerKind<'a> {
path: &&PathBuf,
manifest: &&Manifest,
splicing_manifest: &&SplicingManifest,
cargo: &Cargo,
) -> Result<SplicedManifest> {
let mut manifest = (*manifest).clone();
let manifest_dir = path
Expand All @@ -237,7 +238,7 @@ impl<'a> SplicerKind<'a> {
workspace_metadata.inject_into(&mut manifest)?;

// Write the root manifest
write_root_manifest(&root_manifest_path, manifest)?;
write_root_manifest(&root_manifest_path, manifest, cargo)?;

Ok(SplicedManifest::Workspace(root_manifest_path))
}
Expand All @@ -249,6 +250,7 @@ impl<'a> SplicerKind<'a> {
path: &&PathBuf,
manifest: &&Manifest,
splicing_manifest: &&SplicingManifest,
cargo: &Cargo,
) -> Result<SplicedManifest> {
let manifest_dir = path
.parent()
Expand Down Expand Up @@ -280,7 +282,7 @@ impl<'a> SplicerKind<'a> {
workspace_metadata.inject_into(&mut manifest)?;

// Write the root manifest
write_root_manifest(&root_manifest_path, manifest)?;
write_root_manifest(&root_manifest_path, manifest, cargo)?;

Ok(SplicedManifest::Package(root_manifest_path))
}
Expand All @@ -291,6 +293,7 @@ impl<'a> SplicerKind<'a> {
workspace_dir: &Path,
manifests: &&BTreeMap<PathBuf, Manifest>,
splicing_manifest: &&SplicingManifest,
cargo: &Cargo,
) -> Result<SplicedManifest> {
let mut manifest = default_cargo_workspace_manifest(&splicing_manifest.resolver_version);

Expand Down Expand Up @@ -324,7 +327,7 @@ impl<'a> SplicerKind<'a> {

// Write the root manifest
let root_manifest_path = workspace_dir.join("Cargo.toml");
write_root_manifest(&root_manifest_path, manifest)?;
write_root_manifest(&root_manifest_path, manifest, cargo)?;

Ok(SplicedManifest::MultiPackage(root_manifest_path))
}
Expand Down Expand Up @@ -548,9 +551,9 @@ impl Splicer {
}

/// Build a new workspace root
pub(crate) fn splice_workspace(&self, cargo: &Path) -> Result<SplicedManifest> {
pub(crate) fn splice_workspace(&self, cargo: &Cargo) -> Result<SplicedManifest> {
SplicerKind::new(&self.manifests, &self.splicing_manifest, cargo)?
.splice(&self.workspace_dir)
.splice(&self.workspace_dir, cargo)
}
}

Expand Down Expand Up @@ -646,7 +649,11 @@ pub(crate) fn is_workspace_member(
})
}

pub(crate) fn write_root_manifest(path: &Path, manifest: cargo_toml::Manifest) -> Result<()> {
pub(crate) fn write_root_manifest(
path: &Path,
manifest: cargo_toml::Manifest,
cargo: &Cargo,
) -> Result<()> {
// Remove the file in case one exists already, preventing symlinked files
// from having their contents overwritten.
if path.exists() {
Expand All @@ -658,6 +665,59 @@ pub(crate) fn write_root_manifest(path: &Path, manifest: cargo_toml::Manifest) -
fs::create_dir_all(parent)?;
}

// Write an intermediate manifest so we can run `cargo metadata` to list all the transitive proc-macros.
write_manifest(path, &manifest)?;

let cargo_metadata = cargo.metadata_command()?.manifest_path(path).exec()?;
let proc_macros: BTreeSet<_> = cargo_metadata
.packages
.iter()
.filter(|p| {
p.targets
.iter()
.any(|t| t.kind.iter().any(|k| k == "proc-macro"))
})
.map(|pm| (pm.name.clone(), pm.version.to_string()))
.collect();

// Artificially inject all proc macros as dependency roots.
// Proc macros are built in the exec rather than target configuration.
// If we do cross-compilation, these will be different, and it will be important that we have resolved features and optional dependencies for the exec platform.
// If we don't treat proc macros as roots for the purposes of resolving, we may end up with incorrect platform-specific features.
//
// Example:
// If crate foo only uses a proc macro Linux,
// and that proc-macro depends on syn and requires the feature extra-traits,
// when we resolve on macOS we'll see we don't need the extra-traits feature of syn because the proc macro isn't used.
// But if we're cross-compiling for Linux from macOS, we'll build a syn, but because we're building it for macOS (because proc macros are exec-cfg dependencies),
// we'll build syn but _without_ the extra-traits feature (because our resolve told us it was Linux only).
//
// By artificially injecting all proc macros as root dependencies,
// it means we are forced to resolve the dependencies and features for those proc-macros on all platforms we care about,
// even if they wouldn't be used in some platform when cfg == exec.
//
// This is tested by the "keyring" example in examples/musl_cross_compiling - the keyring crate uses proc-macros only on Linux,
// and if we don't have this fake root injection, cross-compiling from Darwin to Linux won't work because features don't get correctly resolved for the exec=darwin case.
let mut manifest = manifest;
for (dep_name, dep_version) in proc_macros {
let mut detail = cargo_toml::DependencyDetail::default();
detail.package = Some(dep_name.clone());
detail.version = Some(dep_version.clone());
manifest.dependencies.insert(
format!(
"rules_rust_fake_proc_macro_root_{}_{}",
dep_name,
dep_version.replace(".", "_").replace("+", "_")
),
cargo_toml::Dependency::Detailed(Box::new(detail)),
);
}
write_manifest(path, &manifest);

Ok(())
}

fn write_manifest(path: &Path, manifest: &cargo_toml::Manifest) -> Result<()> {
// TODO(https://gitlab.com/crates.rs/cargo_toml/-/issues/3)
let value = toml::Value::try_from(manifest)?;
let content = toml::to_string(&value)?;
Expand Down
19 changes: 19 additions & 0 deletions examples/musl_cross_compiling/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
load("@aspect_bazel_lib//lib:transitions.bzl", "platform_transition_binary")
load("@bazel_skylib//rules:build_test.bzl", "build_test")
load("@rules_rust//rust:defs.bzl", "rust_binary")

rust_binary(
Expand Down Expand Up @@ -38,3 +39,21 @@ sh_test(
],
data = [":hello_linux_arm64_musl"],
)

rust_binary(
name = "keyring",
srcs = ["src/keyring.rs"],
deps = ["@cu//:keyring"],
tags = ["manual"],
)

platform_transition_binary(
name = "keyring_linux_x86_64_musl",
binary = ":keyring",
target_platform = "//platforms:linux_x86_64_musl",
)

build_test(
name = "keyring_linux_x86_64_musl_build_test",
targets = [":keyring_linux_x86_64_musl"],
)
Loading

0 comments on commit 3cc0ab7

Please sign in to comment.