From cf950a2e0cb7c823eeb119cfc17b6ee5822f7d08 Mon Sep 17 00:00:00 2001
From: maciektr <maciej.tratnowiecki@swmansion.com>
Date: Mon, 15 Jan 2024 13:30:25 +0100
Subject: [PATCH] Do not propagate dev deps (#1050)

commit-id:f67e9fd8
---
 scarb/src/core/manifest/dependency.rs | 13 +++++++++++++
 scarb/src/core/manifest/summary.rs    | 23 +++++++++++++++++++++++
 scarb/src/resolver/mod.rs             | 16 +++++++++++++---
 scarb/tests/metadata.rs               |  6 ------
 4 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/scarb/src/core/manifest/dependency.rs b/scarb/src/core/manifest/dependency.rs
index 94a8bb5ff..fde9be07e 100644
--- a/scarb/src/core/manifest/dependency.rs
+++ b/scarb/src/core/manifest/dependency.rs
@@ -30,6 +30,19 @@ pub enum DepKind {
     Target(TargetKind),
 }
 
+impl DepKind {
+    pub fn is_propagated(&self) -> bool {
+        !self.is_test()
+    }
+
+    pub fn is_test(&self) -> bool {
+        match self {
+            DepKind::Target(kind) => kind.is_test(),
+            _ => false,
+        }
+    }
+}
+
 impl Deref for ManifestDependency {
     type Target = ManifestDependencyInner;
 
diff --git a/scarb/src/core/manifest/summary.rs b/scarb/src/core/manifest/summary.rs
index 591fbeac3..e49b9eefe 100644
--- a/scarb/src/core/manifest/summary.rs
+++ b/scarb/src/core/manifest/summary.rs
@@ -62,6 +62,14 @@ impl Summary {
         self.dependencies.iter().chain(self.implicit_dependencies())
     }
 
+    pub fn filtered_full_dependencies(
+        &self,
+        dep_filter: DependencyFilter,
+    ) -> impl Iterator<Item = &ManifestDependency> {
+        self.full_dependencies()
+            .filter(move |dep| dep_filter.filter(dep))
+    }
+
     pub fn implicit_dependencies(&self) -> impl Iterator<Item = &ManifestDependency> {
         static CORE_DEPENDENCY: Lazy<ManifestDependency> = Lazy::new(|| {
             // NOTE: Pin `core` to exact version, because we know that's the only one we have.
@@ -103,3 +111,18 @@ impl Summary {
             .filter(|dep| dep.kind == DepKind::Normal)
     }
 }
+
+#[derive(Default)]
+pub struct DependencyFilter {
+    pub do_propagate: bool,
+}
+
+impl DependencyFilter {
+    pub fn propagation(do_propagate: bool) -> Self {
+        Self { do_propagate }
+    }
+
+    pub fn filter(&self, dep: &ManifestDependency) -> bool {
+        self.do_propagate || dep.kind.is_propagated()
+    }
+}
diff --git a/scarb/src/resolver/mod.rs b/scarb/src/resolver/mod.rs
index 34e0d8042..9439624a1 100644
--- a/scarb/src/resolver/mod.rs
+++ b/scarb/src/resolver/mod.rs
@@ -4,12 +4,14 @@ use anyhow::{bail, Result};
 use indoc::{formatdoc, indoc};
 use petgraph::graphmap::DiGraphMap;
 use scarb_ui::Ui;
+use std::collections::HashSet;
 
 use crate::core::lockfile::Lockfile;
 use crate::core::registry::Registry;
 use crate::core::resolver::{DependencyEdge, Resolve};
 use crate::core::{
-    DepKind, DependencyVersionReq, ManifestDependency, PackageId, Summary, TargetKind,
+    DepKind, DependencyFilter, DependencyVersionReq, ManifestDependency, PackageId, Summary,
+    TargetKind,
 };
 
 /// Builds the list of all packages required to build the first argument.
@@ -41,6 +43,10 @@ pub async fn resolve(
     // TODO(#2): This is very bad, use PubGrub here.
     let mut graph = DiGraphMap::<PackageId, DependencyEdge>::new();
 
+    let main_packages = summaries
+        .iter()
+        .map(|sum| sum.package_id)
+        .collect::<HashSet<PackageId>>();
     let mut packages: HashMap<_, _> = HashMap::from_iter(
         summaries
             .iter()
@@ -59,7 +65,10 @@ pub async fn resolve(
         for package_id in queue {
             graph.add_node(package_id);
 
-            for dep in summaries[&package_id].clone().full_dependencies() {
+            let summary = summaries[&package_id].clone();
+            let dep_filter =
+                DependencyFilter::propagation(main_packages.contains(&summary.package_id));
+            for dep in summary.filtered_full_dependencies(dep_filter) {
                 let dep = rewrite_dependency_source_id(registry, &package_id, dep).await?;
 
                 let locked_package_id = lockfile.packages_matching(dep.clone());
@@ -129,7 +138,8 @@ pub async fn resolve(
     // Detect incompatibilities and bail in case ones are found.
     let mut incompatibilities = Vec::new();
     for from_package in graph.nodes() {
-        for manifest_dependency in summaries[&from_package].full_dependencies() {
+        let dep_filter = DependencyFilter::propagation(main_packages.contains(&from_package));
+        for manifest_dependency in summaries[&from_package].filtered_full_dependencies(dep_filter) {
             let to_package = packages[&manifest_dependency.name];
             if !manifest_dependency.matches_package_id(to_package) {
                 let message = format!(
diff --git a/scarb/tests/metadata.rs b/scarb/tests/metadata.rs
index 62102cbe1..0eff0d1a3 100644
--- a/scarb/tests/metadata.rs
+++ b/scarb/tests/metadata.rs
@@ -273,9 +273,7 @@ fn dev_dependencies() {
 }
 
 #[test]
-#[ignore = "not implemented yet"]
 fn dev_deps_are_not_propagated() {
-    // TODO(maciektr): Make sure dev-deps are not propagated.
     let t = assert_fs::TempDir::new().unwrap();
 
     let dep1 = t.child("dep1");
@@ -314,10 +312,6 @@ fn dev_deps_are_not_propagated() {
                     "test_plugin".to_string(),
                 ]
             ),
-            (
-                "dep1".to_string(),
-                vec!["core".to_string(), "test_plugin".to_string()]
-            ),
             (
                 "dep2".to_string(),
                 vec![