Skip to content

Commit

Permalink
Allow binary-only dependencies in metadata
Browse files Browse the repository at this point in the history
A number of projects, such as https://github.com/google/cargo-raze
consume the output of `cargo metadata` when generating input to other
build systems, such as Bazel and Buck. Typically, these projects use a
`Cargo.toml` file as the input to drive their work, and it can be useful
to express to these systems that a binary is required as part of the
build.

If such a dependency happens to have a lib target, sufficient dependency
information happens to be exposed via the resolve field to recreate the
dependency structure adequately.

However, for binary-only targets, that entire branch of the dependency
tree is cut off, making it hard to recreate this information.

This PR adds an option to `cargo metadata` to allow rendering these
subtrees, describing these deps as of kind "binary". This isn't really a
concept in cargo-proper, but may become one via
rust-lang/rfcs#3168 and/or
rust-lang/rfcs#3028 - this output format is
forwards-compatible with these proposals.

In an RFC 3028 world, binary deps would appear as `dep_kind` "binary",
and `cdylib` or `staticlib` deps would appear as new `DepKind` variants.
We'd probably add a new value of the flag, `declared`, and set that to
be the default. We may also want to deprecate the
`include-if-no-library-dep` value, and make these ignored binary deps a
hard error (replacing the existing warning log).

In an RFC 3168 world, there's an interesting metadata question to have -
there are (at least) three reasonable options in terms of handling
metadata:
1. Require a direct dependency on any package whose binary deps are
   being used from the crate which uses them - this gives a nicely
   restricted search space, and allows for nicely curated metadata
   output.
2. Allow any transitive package dependencies to be used as binary deps -
   this may significantly expand the output produced, but would model
   the implementation of the feature.
3. Require explicitly tagging binary deps as being used as binary deps -
   this looks a lot like RFC 3028, and would tend in that direction.
  • Loading branch information
illicitonion committed Oct 13, 2021
1 parent c8b38af commit ea4a156
Show file tree
Hide file tree
Showing 11 changed files with 284 additions and 14 deletions.
4 changes: 3 additions & 1 deletion benches/benchsuite/benches/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use cargo::core::compiler::{CompileKind, RustcTargetData};
use cargo::core::resolver::features::{CliFeatures, FeatureOpts, FeatureResolver, ForceAllTargets};
use cargo::core::resolver::{HasDevUnits, ResolveBehavior};
use cargo::core::{PackageIdSpec, Workspace};
use cargo::ops::WorkspaceResolve;
use cargo::ops::{BinaryOnlyDepsBehavior, WorkspaceResolve};
use cargo::Config;
use criterion::{criterion_group, criterion_main, Criterion};
use std::fs;
Expand Down Expand Up @@ -198,6 +198,7 @@ fn do_resolve<'cfg>(config: &'cfg Config, ws_root: &Path) -> ResolveInfo<'cfg> {
&specs,
has_dev_units,
force_all_targets,
BinaryOnlyDepsBehavior::Ignore,
)
.unwrap();
ResolveInfo {
Expand Down Expand Up @@ -272,6 +273,7 @@ fn resolve_ws(c: &mut Criterion) {
specs,
*has_dev_units,
*force_all_targets,
BinaryOnlyDepsBehavior::Ignore,
)
.unwrap();
})
Expand Down
27 changes: 26 additions & 1 deletion src/bin/cargo/commands/metadata.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::command_prelude::*;
use cargo::ops::{self, OutputMetadataOptions};
use anyhow::anyhow;
use cargo::ops::{self, BinaryDepsMode, OutputMetadataOptions};

pub fn cli() -> App {
subcommand("metadata")
Expand All @@ -26,6 +27,11 @@ pub fn cli() -> App {
.value_name("VERSION")
.possible_value("1"),
)
.arg(
opt("binary-deps", "How to treat binary dependencies")
.possible_values(&["include-if-no-library-dep", "ignore"])
.default_value("ignore"),
)
.after_help("Run `cargo help metadata` for more detailed information.\n")
}

Expand All @@ -43,11 +49,30 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
Some(version) => version.parse().unwrap(),
};

let binary_deps = {
match args
.value_of("binary-deps")
.unwrap()
.to_ascii_lowercase()
.as_str()
{
"include-if-no-library-dep" => BinaryDepsMode::IncludeIfNoLibraryDep,
"ignore" => BinaryDepsMode::Ignore,
s => {
return Err(CliError::new(
anyhow!("invalid binary-deps specifier: `{}`", s),
1,
))
}
}
};

let options = OutputMetadataOptions {
cli_features: args.cli_features()?,
no_deps: args.is_present("no-deps"),
filter_platforms: args._values_of("filter-platform"),
version,
binary_deps,
};

let result = ops::output_metadata(&ws, &options)?;
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 @@ -118,6 +118,7 @@ pub fn resolve_std<'cfg>(
&specs,
HasDevUnits::No,
crate::core::resolver::features::ForceAllTargets::No,
ops::BinaryOnlyDepsBehavior::Warn,
)?;
Ok((
resolve.pkg_set,
Expand Down
7 changes: 6 additions & 1 deletion src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ impl<'cfg> PackageSet<'cfg> {
requested_kinds: &[CompileKind],
target_data: &RustcTargetData<'_>,
force_all_targets: ForceAllTargets,
treat_binary_only_as_lib: bool,
) -> BTreeMap<PackageId, Vec<&Package>> {
root_ids
.iter()
Expand All @@ -583,7 +584,11 @@ impl<'cfg> PackageSet<'cfg> {
)
.filter_map(|package_id| {
if let Ok(dep_pkg) = self.get_one(package_id) {
if !dep_pkg.targets().iter().any(|t| t.is_lib()) {
if !dep_pkg
.targets()
.iter()
.any(|t| t.is_lib() || (treat_binary_only_as_lib && t.is_bin()))
{
Some(dep_pkg)
} else {
None
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ pub fn create_bcx<'a, 'cfg>(
&specs,
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
ops::BinaryOnlyDepsBehavior::Warn,
)?;
let WorkspaceResolve {
mut pkg_set,
Expand Down
95 changes: 86 additions & 9 deletions src/cargo/ops/cargo_output_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::ops::{self, Packages};
use crate::util::interning::InternedString;
use crate::util::CargoResult;
use cargo_platform::Platform;
use serde::Serialize;
use serde::{Serialize, Serializer};
use std::collections::BTreeMap;
use std::path::PathBuf;

Expand All @@ -18,6 +18,7 @@ pub struct OutputMetadataOptions {
pub no_deps: bool,
pub version: u32,
pub filter_platforms: Vec<String>,
pub binary_deps: BinaryDepsMode,
}

/// Loads the manifest, resolves the dependencies of the package to the concrete
Expand Down Expand Up @@ -88,19 +89,52 @@ struct Dep {

#[derive(Serialize, PartialEq, Eq, PartialOrd, Ord)]
struct DepKindInfo {
kind: DepKind,
kind: DepKindIncludingBinaryDeps,
target: Option<Platform>,
}

impl From<&Dependency> for DepKindInfo {
fn from(dep: &Dependency) -> DepKindInfo {
DepKindInfo {
kind: dep.kind(),
kind: dep.kind().into(),
target: dep.platform().cloned(),
}
}
}

#[derive(PartialEq, Eq, PartialOrd, Ord)]
enum DepKindIncludingBinaryDeps {
Normal,
Development,
Build,
Binary,
}

impl From<DepKind> for DepKindIncludingBinaryDeps {
fn from(dep_kind: DepKind) -> Self {
match dep_kind {
DepKind::Normal => DepKindIncludingBinaryDeps::Normal,
DepKind::Development => DepKindIncludingBinaryDeps::Development,
DepKind::Build => DepKindIncludingBinaryDeps::Build,
}
}
}

impl Serialize for DepKindIncludingBinaryDeps {
fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
match *self {
DepKindIncludingBinaryDeps::Normal => None,
DepKindIncludingBinaryDeps::Development => Some("dev"),
DepKindIncludingBinaryDeps::Build => Some("build"),
DepKindIncludingBinaryDeps::Binary => Some("binary"),
}
.serialize(s)
}
}

/// Builds the resolve graph as it will be displayed to the user.
fn build_resolve_graph(
ws: &Workspace<'_>,
Expand All @@ -119,6 +153,11 @@ fn build_resolve_graph(
crate::core::resolver::features::ForceAllTargets::No
};

let binary_only_deps_behavior = match metadata_opts.binary_deps {
BinaryDepsMode::Ignore => ops::BinaryOnlyDepsBehavior::Warn,
BinaryDepsMode::IncludeIfNoLibraryDep => ops::BinaryOnlyDepsBehavior::Ignore,
};

// Note that even with --filter-platform we end up downloading host dependencies as well,
// as that is the behavior of download_accessible.
let ws_resolve = ops::resolve_ws_with_opts(
Expand All @@ -129,6 +168,7 @@ fn build_resolve_graph(
&specs,
HasDevUnits::Yes,
force_all,
binary_only_deps_behavior,
)?;

let package_map: BTreeMap<PackageId, Package> = ws_resolve
Expand All @@ -149,6 +189,7 @@ fn build_resolve_graph(
&package_map,
&target_data,
&requested_kinds,
metadata_opts.binary_deps,
);
}
// Get a Vec of Packages.
Expand All @@ -166,13 +207,20 @@ fn build_resolve_graph(
Ok((actual_packages, mr))
}

#[derive(Clone, Copy)]
pub enum BinaryDepsMode {
IncludeIfNoLibraryDep,
Ignore,
}

fn build_resolve_graph_r(
node_map: &mut BTreeMap<PackageId, MetadataResolveNode>,
pkg_id: PackageId,
resolve: &Resolve,
package_map: &BTreeMap<PackageId, Package>,
target_data: &RustcTargetData<'_>,
requested_kinds: &[CompileKind],
binary_deps: BinaryDepsMode,
) {
if node_map.contains_key(&pkg_id) {
return;
Expand Down Expand Up @@ -206,14 +254,42 @@ fn build_resolve_graph_r(
})
}
})
.filter_map(|(dep_id, deps)| {
let mut dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
dep_kinds.sort();
.flat_map(|(dep_id, deps)| {
package_map
.get(&dep_id)
.and_then(|pkg| pkg.targets().iter().find(|t| t.is_lib()))
.and_then(|lib_target| resolve.extern_crate_name(pkg_id, dep_id, lib_target).ok())
.map(|name| Dep {
.into_iter()
.flat_map(move |pkg| {
if let Some(lib_target) = pkg.targets().iter().find(|t| t.is_lib()) {
let mut dep_kinds: Vec<_> = deps.iter().map(DepKindInfo::from).collect();
dep_kinds.sort();
vec![(lib_target, dep_kinds)]
} else {
match binary_deps {
BinaryDepsMode::IncludeIfNoLibraryDep => pkg
.targets()
.iter()
.filter(|t| t.is_bin())
.map(|t| {
(
t,
vec![DepKindInfo {
kind: DepKindIncludingBinaryDeps::Binary,
target: None,
}],
)
})
.collect(),
BinaryDepsMode::Ignore => vec![],
}
}
})
.filter_map(move |(lib_target, dep_kinds)| {
resolve
.extern_crate_name(pkg_id, dep_id, lib_target)
.ok()
.map(|crate_name| (crate_name, dep_kinds))
})
.map(move |(name, dep_kinds)| Dep {
name,
pkg: normalize_id(dep_id),
dep_kinds,
Expand All @@ -237,6 +313,7 @@ fn build_resolve_graph_r(
package_map,
target_data,
requested_kinds,
binary_deps,
);
}
}
1 change: 1 addition & 0 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ fn check_resolver_change(ws: &Workspace<'_>, opts: &FixOptions) -> CargoResult<(
&specs,
has_dev_units,
crate::core::resolver::features::ForceAllTargets::No,
ops::BinaryOnlyDepsBehavior::Warn,
)?;

let feature_opts = FeatureOpts::new_behavior(ResolveBehavior::V2, has_dev_units);
Expand Down
6 changes: 4 additions & 2 deletions src/cargo/ops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ pub use self::cargo_generate_lockfile::update_lockfile;
pub use self::cargo_generate_lockfile::UpdateOptions;
pub use self::cargo_install::{install, install_list};
pub use self::cargo_new::{init, new, NewOptions, VersionControl};
pub use self::cargo_output_metadata::{output_metadata, ExportInfo, OutputMetadataOptions};
pub use self::cargo_output_metadata::{
output_metadata, BinaryDepsMode, ExportInfo, OutputMetadataOptions,
};
pub use self::cargo_package::{package, package_one, PackageOpts};
pub use self::cargo_pkgid::pkgid;
pub use self::cargo_read_manifest::{read_package, read_packages};
Expand All @@ -28,7 +30,7 @@ pub use self::registry::{needs_custom_http_transport, registry_login, registry_l
pub use self::registry::{publish, registry_configuration, RegistryConfig};
pub use self::resolve::{
add_overrides, get_resolved_packages, resolve_with_previous, resolve_ws, resolve_ws_with_opts,
WorkspaceResolve,
BinaryOnlyDepsBehavior, WorkspaceResolve,
};
pub use self::vendor::{vendor, VendorOptions};

Expand Down
8 changes: 8 additions & 0 deletions src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ pub fn resolve_ws<'a>(ws: &Workspace<'a>) -> CargoResult<(PackageSet<'a>, Resolv
Ok((packages, resolve))
}

#[derive(Eq, PartialEq)]
pub enum BinaryOnlyDepsBehavior {
Warn,
Ignore,
}

/// Resolves dependencies for some packages of the workspace,
/// taking into account `paths` overrides and activated features.
///
Expand All @@ -87,6 +93,7 @@ pub fn resolve_ws_with_opts<'cfg>(
specs: &[PackageIdSpec],
has_dev_units: HasDevUnits,
force_all_targets: ForceAllTargets,
binary_only_deps_behaviour: BinaryOnlyDepsBehavior,
) -> CargoResult<WorkspaceResolve<'cfg>> {
let mut registry = PackageRegistry::new(ws.config())?;
let mut add_patches = true;
Expand Down Expand Up @@ -178,6 +185,7 @@ pub fn resolve_ws_with_opts<'cfg>(
requested_targets,
target_data,
force_all_targets,
binary_only_deps_behaviour == BinaryOnlyDepsBehavior::Ignore,
);
for (pkg_id, dep_pkgs) in no_lib_pkgs {
for dep_pkg in dep_pkgs {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/tree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ pub fn build_and_print(ws: &Workspace<'_>, opts: &TreeOptions) -> CargoResult<()
&specs,
has_dev,
force_all,
ops::BinaryOnlyDepsBehavior::Warn,
)?;

let package_map: HashMap<PackageId, &Package> = ws_resolve
Expand Down
Loading

0 comments on commit ea4a156

Please sign in to comment.