Consolidate duplicated targeted dependencies.

impl/src/context.rs

@@ -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};
@@ -137,13 +137,54 @@ impl CrateDependencyContext {
       .cloned()
       .collect();
   }
+
+  pub fn add(&mut self, other: &CrateDependencyContext) {
+    self.dependencies = self
+      .dependencies
+      .union(&other.dependencies)
+      .cloned()
+      .collect();
+    self.proc_macro_dependencies = self
+      .proc_macro_dependencies
+      .union(&other.proc_macro_dependencies)
+      .cloned()
+      .collect();
+    self.build_dependencies = self
+      .build_dependencies
+      .union(&other.build_dependencies)
+      .cloned()
+      .collect();
+    self.build_proc_macro_dependencies = self
+      .build_proc_macro_dependencies
+      .union(&other.build_proc_macro_dependencies)
+      .cloned()
+      .collect();
+    self.dev_dependencies = self
+      .dev_dependencies
+      .union(&other.dev_dependencies)
+      .cloned()
+      .collect();
+  }
 }
 
 #[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 {
+    let deps = attrs.iter().skip(1).fold(attrs[0].clone(), |mut acc, hs| {
+      acc.add(hs);
+      acc
+    });
+
+    CrateTargetedDepContext {
+      platform_targets: platforms,
+      deps,
+    }
+  }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]

impl/src/features.rs

@@ -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};
@@ -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.
@@ -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,
+      ),
     },
   )
 }

impl/src/planning.rs

@@ -16,6 +16,11 @@ mod crate_catalog;
 mod license;
 mod subplanners;
 
+use std::{
+  collections::{BTreeMap, BTreeSet},
+  fmt::Debug,
+};
+
 use anyhow::Result;
 use cargo_lock::Lockfile;
 
@@ -79,6 +84,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 + Debug,
+  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 attr in pfs {
+      platform_map
+        .entry(attr)
+        .and_modify(|e| e.push(platform.clone()))
+        .or_insert_with(|| vec![platform.clone()]);
+    }
+  }
+
+  let mut platforms_to_attrs: BTreeMap<Vec<String>, Vec<AttrType>> = BTreeMap::new();
+  for (attr, platforms) in platform_map {
+    platforms_to_attrs
+      .entry(platforms.clone())
+      .and_modify(|e| e.push(attr.clone()))
+      .or_insert_with(|| vec![attr.clone()]);
+  }
+
+  platforms_to_attrs
+    .iter()
+    .map(|(platforms, attrs)| T::new(platforms.to_vec(), attrs.to_vec()))
+    .collect()
+}
+
 #[cfg(test)]
 pub mod tests {
   use std::{collections::BTreeMap, collections::HashMap, collections::HashSet};
@@ -412,6 +453,114 @@ pub mod tests {
     assert_eq!(flate2.targeted_deps[0].deps.dependencies.len(), 0);
   }
 
+  #[test]
+  fn test_plan_includes_all_not_unknown_targets() {
+    let mut settings = dummy_raze_settings();
+    settings.genmode = GenMode::Remote;
+    let mut triples = HashSet::new();
+    triples.insert("aarch64-apple-darwin".to_string());
+    triples.insert("aarch64-unknown-linux-gnu".to_string());
+    triples.insert("i686-apple-darwin".to_string());
+    triples.insert("i686-pc-windows-msvc".to_string());
+    triples.insert("i686-unknown-linux-gnu".to_string());
+    triples.insert("x86_64-apple-darwin".to_string());
+    triples.insert("x86_64-pc-windows-msvc".to_string());
+    triples.insert("x86_64-unknown-linux-gnu".to_string());
+    triples.insert("wasm32-wasi".to_string());
+    triples.insert("wasm32-unknown-unknown".to_string());
+    settings.targets = Some(triples);
+
+    let planner = BuildPlannerImpl::new(
+      dummy_workspace_crate_metadata(templates::TARGET_OS_IS_NOT_UNKNOWN),
+      settings,
+    );
+    let planned_build = planner.plan_build(None).unwrap();
+    let async_std = planned_build
+      .crate_contexts
+      .iter()
+      .find(|ctx| ctx.pkg_name == "async-std")
+      .unwrap();
+
+    // The wasm-targeted deps should have both wasm platforms
+    let wasm_targeted_dep_context = async_std
+      .targeted_deps
+      .iter()
+      .find(|dep_context| {
+        dep_context
+          .deps
+          .dependencies
+          .iter()
+          .find(|dep| dep.name == "wasm-bindgen-futures")
+          .is_some()
+      })
+      .unwrap();
+    assert_eq!(
+      wasm_targeted_dep_context.platform_targets,
+      vec!["wasm32-unknown-unknown", "wasm32-wasi",]
+    );
+
+    // The os-targeted deps should only have wasm32-wasi
+    let os_targeted_dep_context = async_std
+      .targeted_deps
+      .iter()
+      .find(|dep_context| {
+        dep_context
+          .deps
+          .dependencies
+          .iter()
+          .find(|dep| dep.name == "async-io")
+          .is_some()
+      })
+      .unwrap();
+    assert_eq!(
+      os_targeted_dep_context.platform_targets,
+      vec![
+        "aarch64-apple-darwin",
+        "aarch64-unknown-linux-gnu",
+        "i686-apple-darwin",
+        "i686-pc-windows-msvc",
+        "i686-unknown-linux-gnu",
+        "wasm32-wasi",
+        "x86_64-apple-darwin",
+        "x86_64-pc-windows-msvc",
+        "x86_64-unknown-linux-gnu"
+      ]
+    );
+  }
+
+  #[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();