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

Consolidate duplicated targeted dependencies. #512

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
14 changes: 11 additions & 3 deletions impl/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

use std::collections::{BTreeMap, BTreeSet};

use crate::{features::Features, settings::CrateSettings};
use crate::{features::Features, planning::PlatformCrateAttribute, settings::CrateSettings};
use camino::Utf8PathBuf;
use semver::Version;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -141,9 +141,17 @@ impl CrateDependencyContext {

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
pub struct CrateTargetedDepContext {
pub target: String,
pub deps: CrateDependencyContext,
pub platform_targets: Vec<String>,
pub deps: CrateDependencyContext,
}

impl PlatformCrateAttribute<CrateDependencyContext> for CrateTargetedDepContext {
fn new(platforms: Vec<String>, attrs: Vec<CrateDependencyContext>) -> Self {
CrateTargetedDepContext {
platform_targets: platforms,
deps: attrs[0].clone(),
sayrer marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
Expand Down
55 changes: 20 additions & 35 deletions impl/src/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::path::Path;
use std::process::Command;

use crate::error::RazeError;
use crate::planning::{consolidate_platform_attributes, PlatformCrateAttribute};
use crate::settings::RazeSettings;
use crate::util::cargo_bin_path;
use anyhow::{Error, Result};
Expand Down Expand Up @@ -47,6 +48,15 @@ pub struct TargetedFeatures {
pub features: Vec<String>,
}

impl PlatformCrateAttribute<String> for TargetedFeatures {
fn new(platforms: Vec<String>, attrs: Vec<String>) -> Self {
TargetedFeatures {
platforms,
features: attrs,
}
}
}

// A function that runs `cargo-tree` to analyze per-platform features.
// This step should not need to be separate from cargo-metadata, but cargo-metadata's
// output is incomplete in this respect.
Expand Down Expand Up @@ -248,48 +258,23 @@ fn consolidate_features(
let common_features = sets.iter().skip(1).fold(sets[0].clone(), |acc, hs| {
acc.intersection(hs).cloned().collect()
});

// Partition the platform features
let mut platform_map: BTreeMap<String, Vec<String>> = BTreeMap::new();
for (platform, pfs) in features {
for feature in pfs {
if !common_features.contains(&feature) {
platform_map
.entry(feature)
.and_modify(|e| e.push(platform.clone()))
.or_insert_with(|| vec![platform.clone()]);
}
}
}

let mut platforms_to_features: BTreeMap<Vec<String>, Vec<String>> = BTreeMap::new();
for (feature, platforms) in platform_map {
let key = platforms.clone();
platforms_to_features
.entry(key)
.and_modify(|e| e.push(feature.clone()))
.or_insert_with(|| vec![feature]);
}

let mut common_vec: Vec<String> = common_features.iter().cloned().collect();
common_vec.sort();

let targeted_features: Vec<TargetedFeatures> = platforms_to_features
let platform_features: BTreeMap<String, BTreeSet<String>> = features
.iter()
.map(|ptf| {
let (platforms, features) = ptf;
TargetedFeatures {
platforms: platforms.to_vec(),
features: features.to_vec(),
}
.map(|(platform, pfs)| {
(
platform.to_string(),
pfs.difference(&common_features).cloned().collect(),
)
})
.collect();

(
id,
Features {
features: common_vec,
targeted_features,
features: common_features.iter().cloned().collect(),
targeted_features: consolidate_platform_attributes::<String, TargetedFeatures>(
platform_features,
),
},
)
}
Expand Down
71 changes: 71 additions & 0 deletions impl/src/planning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ mod crate_catalog;
mod license;
mod subplanners;

use std::collections::{BTreeMap, BTreeSet};

use anyhow::Result;
use cargo_lock::Lockfile;

Expand Down Expand Up @@ -79,6 +81,42 @@ impl BuildPlannerImpl {
}
}

/// Items that need to be rendered with platform select blocks.
pub trait PlatformCrateAttribute<T> {
fn new(platforms: Vec<String>, attrs: Vec<T>) -> Self;
}

/// Group PlatformCrateAttribute items into concise select blocks, with no duplicates.
pub fn consolidate_platform_attributes<
AttrType: Ord + Clone,
T: PlatformCrateAttribute<AttrType>,
>(
platform_attributes: BTreeMap<String, BTreeSet<AttrType>>,
) -> Vec<T> {
let mut platform_map: BTreeMap<AttrType, Vec<String>> = BTreeMap::new();
for (platform, pfs) in platform_attributes {
for feature in pfs {
platform_map
.entry(feature)
.and_modify(|e| e.push(platform.clone()))
.or_insert_with(|| vec![platform.clone()]);
}
}

let mut platforms_to_features: BTreeMap<Vec<String>, Vec<AttrType>> = BTreeMap::new();
for (feature, platforms) in platform_map {
platforms_to_features
.entry(platforms.clone())
.and_modify(|e| e.push(feature.clone()))
.or_insert_with(|| vec![feature.clone()]);
}

platforms_to_features
.iter()
.map(|(platforms, features)| T::new(platforms.to_vec(), features.to_vec()))
.collect()
}

#[cfg(test)]
pub mod tests {
use std::{collections::BTreeMap, collections::HashMap, collections::HashSet};
Expand Down Expand Up @@ -412,6 +450,39 @@ pub mod tests {
assert_eq!(flate2.targeted_deps[0].deps.dependencies.len(), 0);
}

#[test]
// Tests the fix for https://github.com/google/cargo-raze/issues/451.
// Bazel errors out if mutually exclusive select branches contain the
// same dependency. rust-errno is a real world example of this problem,
// where libc is listed under 'cfg(unix)' and 'cfg(target_os="wasi")'.
fn test_plan_build_consolidates_targets_across_platforms() {
let mut settings = dummy_raze_settings();
settings.genmode = GenMode::Remote;
let mut triples = HashSet::new();
triples.insert("wasm32-wasi".to_string());
triples.insert("x86_64-unknown-linux-gnu".to_string());
settings.target = None;
settings.targets = Some(triples);

let planner = BuildPlannerImpl::new(
dummy_workspace_crate_metadata(templates::SUBPLAN_CONSOLIDATES_TARGETED_DEPS),
settings,
);
let planned_build = planner.plan_build(None).unwrap();

let errno = planned_build
.crate_contexts
.iter()
.find(|ctx| ctx.pkg_name == "errno")
.unwrap();

assert_eq!(errno.targeted_deps.len(), 1);
assert_eq!(
errno.targeted_deps[0].platform_targets,
vec!["wasm32-wasi", "x86_64-unknown-linux-gnu",]
);
}

fn dummy_binary_dependency_metadata(is_remote_genmode: bool) -> (RazeMetadata, RazeSettings) {
let (mut fetcher, server, index_dir) = dummy_raze_metadata_fetcher();

Expand Down
43 changes: 19 additions & 24 deletions impl/src/planning/subplanners.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

use std::{
collections::{BTreeMap, HashMap, HashSet},
collections::{BTreeMap, BTreeSet, HashMap, HashSet},
io, iter,
str::FromStr,
};
Expand All @@ -34,7 +34,7 @@ use crate::{
error::{RazeError, PLEASE_FILE_A_BUG},
features::Features,
metadata::RazeMetadata,
planning::license,
planning::{consolidate_platform_attributes, license},
settings::{CrateSettings, GenMode, RazeSettings},
util,
};
Expand Down Expand Up @@ -308,28 +308,23 @@ impl<'planner> CrateSubplanner<'planner> {
ctx.subtract(&default_deps);
}

// Build a list of dependencies while addression a potential allowlist of target triples
let mut targeted_deps = deps
.into_iter()
.map(|(target, deps)| {
let target = target.unwrap();
let platform_targets = util::get_matching_bazel_triples(&target, &self.settings.targets)?
.map(|x| x.to_string())
.collect();

Ok(CrateTargetedDepContext {
target,
deps,
platform_targets,
})
})
.filter(|res| match res {
Ok(ctx) => !ctx.platform_targets.is_empty(),
Err(_) => true,
})
.collect::<Result<Vec<_>>>()?;

targeted_deps.sort();
// Build a list of dependencies while addressing a potential allowlist of target triples and consolidate any dependencies duplicated across platforms.
let targeted_deps =
consolidate_platform_attributes::<CrateDependencyContext, CrateTargetedDepContext>(
deps
.into_iter()
.flat_map(|(target, dep_context)| {
let target = target.unwrap();
if let Ok(triples) = util::get_matching_bazel_triples(&target, &self.settings.targets) {
triples
.map(|i| (i.to_string(), BTreeSet::from([dep_context.clone()])))
.collect()
} else {
vec![]
}
})
.collect(),
);

let mut workspace_member_dependents: Vec<Utf8PathBuf> = Vec::new();
let mut workspace_member_dev_dependents: Vec<Utf8PathBuf> = Vec::new();
Expand Down
2 changes: 2 additions & 0 deletions impl/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ pub mod templates {
pub const PLAN_BUILD_PRODUCES_PROC_MACRO_DEPENDENCIES: &str =
"plan_build_produces_proc_macro_dependencies.json.template";
pub const SEMVER_MATCHING: &str = "semver_matching.json.template";
pub const SUBPLAN_CONSOLIDATES_TARGETED_DEPS: &str =
"subplan_consolidates_targeted_deps.json.template";
pub const SUBPLAN_PRODUCES_CRATE_ROOT_WITH_FORWARD_SLASH: &str =
"subplan_produces_crate_root_with_forward_slash.json.template";
pub const SUBPLAN_OMITS_PLATFORM_DEPS_ALREADY_IN_DEFAULT_DEPS: &str =
Expand Down
Loading