Prevent non-lib packages from being used as dependencies in other packages

scarb/src/core/manifest/target.rs

@@ -55,6 +55,10 @@ impl Target {
     pub fn new(name: SmolStr, kind: TargetKind) -> Self {
         Self(Arc::new(TargetInner { name, kind }))
     }
+
+    pub fn is_lib(&self) -> bool {
+        matches!(self.kind, TargetKind::Lib(_))
+    }
 }
 
 impl TargetKind {

scarb/src/core/mod.rs

@@ -10,7 +10,7 @@ pub use manifest::{
     TomlExternalTarget, TomlLibTarget, TomlManifest, TomlPackage, TomlTargetKindName,
 };
 pub use package::{Package, PackageId, PackageIdInner, PackageInner, PackageName};
-pub use resolver::{PackageComponentsIds, Resolve};
+pub use resolver::Resolve;
 pub use source::{GitReference, SourceId, SourceIdInner, SourceKind};
 pub use workspace::Workspace;
 

scarb/src/core/package/mod.rs

@@ -8,6 +8,7 @@ pub use id::*;
 pub use name::*;
 
 use crate::core::manifest::Manifest;
+use crate::core::Target;
 use crate::DEFAULT_SOURCE_DIR_NAME;
 
 mod id;
@@ -55,6 +56,10 @@ impl Package {
     pub fn source_dir(&self) -> Utf8PathBuf {
         self.root().join(DEFAULT_SOURCE_DIR_NAME)
     }
+
+    pub fn is_lib(&self) -> bool {
+        self.manifest.targets.iter().any(Target::is_lib)
+    }
 }
 
 impl fmt::Display for Package {

scarb/src/core/resolver.rs

@@ -1,5 +1,4 @@
-use std::collections::HashSet;
-
+use itertools::Itertools;
 use petgraph::graphmap::DiGraphMap;
 use petgraph::visit::{Dfs, Walker};
 
@@ -17,10 +16,6 @@ pub struct Resolve {
     pub graph: DiGraphMap<PackageId, ()>,
 }
 
-/// Collection of all [`PackageId`]s of packages needed to provide as _crate roots_ to
-/// the Cairo compiler in order to build a particular package (named _root package_).
-pub type PackageComponentsIds = HashSet<PackageId>;
-
 impl Resolve {
     /// Iterator over all [`PackageId`]s (nodes) present in this graph.
     ///
@@ -31,12 +26,18 @@ impl Resolve {
 
     /// Collect all [`PackageId`]s needed to compile a root package.
     ///
+    /// Returns a collection of all [`PackageId`]s of packages needed to provide as _crate roots_
+    /// to the Cairo compiler in order to build a particular package (named _root package_).
+    ///
     /// # Safety
     /// * Asserts that `root_package` is a node in this graph.
-    pub fn package_components_of(&self, root_package: PackageId) -> PackageComponentsIds {
+    pub fn package_components_of(
+        &self,
+        root_package: PackageId,
+    ) -> impl Iterator<Item = PackageId> + '_ {
         assert!(&self.graph.contains_node(root_package));
         Dfs::new(&self.graph, root_package)
             .iter(&self.graph)
-            .collect()
+            .unique()
     }
 }

scarb/src/ops/resolve.rs

@@ -18,13 +18,14 @@ pub struct WorkspaceResolve {
 }
 
 impl WorkspaceResolve {
-    pub fn package_components_of(&self, root_package: PackageId) -> Vec<Package> {
+    pub fn package_components_of(
+        &self,
+        root_package: PackageId,
+    ) -> impl Iterator<Item = Package> + '_ {
         assert!(self.packages.contains_key(&root_package));
         self.resolve
             .package_components_of(root_package)
-            .into_iter()
             .map(|id| self.packages[&id].clone())
-            .collect()
     }
 }
 
@@ -46,11 +47,6 @@ pub fn resolve_workspace(ws: &Workspace<'_>) -> Result<WorkspaceResolve> {
 
         let resolve = resolver::resolve(&members_summaries, &mut registry_cache).await?;
 
-        // Gather [`Package`] instances from this resolver result, by asking the [`RegistryCache`]
-        // to download resolved packages.
-        //
-        // Currently, it is expected that all packages are already downloaded during resolution,
-        // so the `download` calls in this method should be cheap, but this may change the future.
         let packages = collect_packages_from_resolve_graph(&resolve, &mut registry_cache).await?;
 
         Ok(WorkspaceResolve { resolve, packages })
@@ -65,7 +61,21 @@ pub fn generate_compilation_units(
 ) -> Result<Vec<CompilationUnit>> {
     let mut units = Vec::with_capacity(ws.members().size_hint().0);
     for member in ws.members() {
-        let components = resolve.package_components_of(member.id);
+        let components = resolve
+            .package_components_of(member.id)
+            .filter(|pkg| {
+                let is_self_or_lib = member.id == pkg.id || pkg.is_lib();
+                // Print a warning if this dependency is not a library.
+                if !is_self_or_lib {
+                    ws.config().ui().warn(format!(
+                        "{} ignoring invalid dependency `{}` which is missing a lib target",
+                        member.id, pkg.id.name
+                    ));
+                }
+                is_self_or_lib
+            })
+            .collect::<Vec<_>>();
+
         for target in &member.manifest.targets {
             let unit = CompilationUnit {
                 package: member.clone(),
@@ -82,6 +92,11 @@ pub fn generate_compilation_units(
     Ok(units)
 }
 
+/// Gather [`Package`] instances from this resolver result, by asking the [`RegistryCache`]
+/// to download resolved packages.
+///
+/// Currently, it is expected that all packages are already downloaded during resolution,
+/// so the `download` calls in this method should be cheap, but this may change the future.
 #[tracing::instrument(level = "trace", skip_all)]
 async fn collect_packages_from_resolve_graph(
     resolve: &Resolve,

scarb/tests/e2e/build.rs

@@ -1,6 +1,7 @@
 use std::fs;
 
 use assert_fs::prelude::*;
+use assert_fs::TempDir;
 use indoc::indoc;
 use predicates::prelude::*;
 
@@ -13,9 +14,9 @@ fn compile_simple() {
     // `TempDir::new` creates the directory, while `create_output_dir` does not mark directory as
     // ephemeral if it already exists.
     // Therefore, we use `.child` here to ensure that cache directory does not exist when running.
-    let cache_dir = assert_fs::TempDir::new().unwrap().child("c");
+    let cache_dir = TempDir::new().unwrap().child("c");
 
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start().name("hello").build(&t);
 
     Scarb::quick_snapbox()
@@ -38,7 +39,7 @@ fn compile_simple() {
 
 #[test]
 fn quiet_output() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start().build(&t);
 
     Scarb::quick_snapbox()
@@ -58,7 +59,7 @@ fn quiet_output() {
 
 #[test]
 fn compile_with_syntax_error() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start()
         .name("hello")
         .version("0.1.0")
@@ -84,7 +85,7 @@ fn compile_with_syntax_error() {
 
 #[test]
 fn compile_with_syntax_error_json() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start()
         .name("hello")
         .version("0.1.0")
@@ -106,7 +107,7 @@ fn compile_with_syntax_error_json() {
 
 #[test]
 fn compile_without_manifest() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     let cause = fs::read(t.child("Scarb.toml")).unwrap_err();
     Scarb::quick_snapbox()
         .arg("build")
@@ -126,7 +127,7 @@ Caused by:
 #[test]
 #[cfg(target_os = "linux")]
 fn compile_with_lowercase_scarb_toml() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     t.child("scarb.toml")
         .write_str(
             r#"
@@ -154,7 +155,7 @@ Caused by:
 
 #[test]
 fn compile_with_manifest_not_a_file() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     t.child("Scarb.toml").create_dir_all().unwrap();
     let cause = fs::read(t.child("Scarb.toml")).unwrap_err();
     Scarb::quick_snapbox()
@@ -174,7 +175,7 @@ Caused by:
 
 #[test]
 fn compile_with_invalid_empty_name() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     t.child("Scarb.toml")
         .write_str(
             r#"
@@ -203,7 +204,7 @@ fn compile_with_invalid_empty_name() {
 
 #[test]
 fn compile_with_invalid_version() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     t.child("Scarb.toml")
         .write_str(
             r#"
@@ -232,7 +233,7 @@ fn compile_with_invalid_version() {
 
 #[test]
 fn compile_with_invalid_non_numeric_dep_version() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     t.child("Scarb.toml")
         .write_str(
             r#"
@@ -264,7 +265,7 @@ fn compile_with_invalid_non_numeric_dep_version() {
 
 #[test]
 fn compile_multiple_packages() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
 
     t.child("Scarb.toml")
         .write_str(
@@ -332,7 +333,7 @@ fn compile_multiple_packages() {
 
 #[test]
 fn compile_with_nested_deps() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
 
     t.child("Scarb.toml")
         .write_str(
@@ -414,114 +415,11 @@ fn compile_with_nested_deps() {
         .assert(predicates::str::is_empty().not());
 }
 
-#[test]
-fn compile_with_duplicate_targets_1() {
-    let t = assert_fs::TempDir::new().unwrap();
-    t.child("Scarb.toml")
-        .write_str(
-            r#"
-            [package]
-            name = "hello"
-            version = "0.1.0"
-
-            [[target.example]]
-
-            [[target.example]]
-            "#,
-        )
-        .unwrap();
-
-    Scarb::quick_snapbox()
-        .arg("build")
-        .current_dir(&t)
-        .assert()
-        .failure()
-        .stdout_matches(indoc! {r#"
-        error: failed to parse manifest at `[..]/Scarb.toml`
-
-        Caused by:
-            manifest contains duplicate target definitions `example`, consider explicitly naming targets with the `name` field
-        "#});
-}
-
-#[test]
-fn compile_with_duplicate_targets_2() {
-    let t = assert_fs::TempDir::new().unwrap();
-    t.child("Scarb.toml")
-        .write_str(
-            r#"
-            [package]
-            name = "hello"
-            version = "0.1.0"
-
-            [[target.example]]
-            name = "x"
-
-            [[target.example]]
-            name = "x"
-            "#,
-        )
-        .unwrap();
-
-    Scarb::quick_snapbox()
-        .arg("build")
-        .current_dir(&t)
-        .assert()
-        .failure()
-        .stdout_matches(indoc! {r#"
-        error: failed to parse manifest at `[..]/Scarb.toml`
-
-        Caused by:
-            manifest contains duplicate target definitions `example (x)`, use different target names to resolve the conflict
-        "#});
-}
-
-#[test]
-fn compile_with_custom_lib_target() {
-    let t = assert_fs::TempDir::new().unwrap();
-    t.child("Scarb.toml")
-        .write_str(
-            r#"
-            [package]
-            name = "hello"
-            version = "0.1.0"
-
-            [lib]
-            name = "not_hello"
-            sierra = false
-            casm = true
-            "#,
-        )
-        .unwrap();
-    t.child("src/lib.cairo")
-        .write_str(r#"fn f() -> felt { 42 }"#)
-        .unwrap();
-
-    Scarb::quick_snapbox()
-        .arg("build")
-        .current_dir(&t)
-        .assert()
-        .success()
-        .stdout_matches(indoc! {r#"
-        [..] Compiling hello v0.1.0 ([..])
-        [..]  Finished release target(s) in [..]
-        "#});
-
-    t.child("target/release/not_hello.casm")
-        .assert(predicates::str::is_empty().not());
-    t.child("target/release/not_hello.sierra")
-        .assert(predicates::path::exists().not());
-    t.child("target/release/hello.sierra")
-        .assert(predicates::path::exists().not());
-    t.child("target/release/hello.casm")
-        .assert(predicates::path::exists().not());
-}
-
 #[test]
 fn override_target_dir() {
-    let target_dir = assert_fs::TempDir::new().unwrap();
+    let target_dir = TempDir::new().unwrap();
 
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start().name("hello").build(&t);
 
     Scarb::quick_snapbox()