diff --git a/extensions/scarb-cairo-test/src/main.rs b/extensions/scarb-cairo-test/src/main.rs index 8d460f818..b3133d36a 100644 --- a/extensions/scarb-cairo-test/src/main.rs +++ b/extensions/scarb-cairo-test/src/main.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::{env, fs}; use anyhow::{Context, Result}; @@ -58,11 +59,22 @@ fn main() -> Result<()> { .unwrap_or(default_target_dir) .join(profile); + let mut deduplicator = TargetGroupDeduplicator::default(); for package in matched { println!("testing {} ...", package.name); for target in find_testable_targets(&package) { - let file_path = target_dir.join(format!("{}.test.json", target.name.clone())); + let name = target + .params + .get("group-id") + .and_then(|v| v.as_str()) + .map(ToString::to_string) + .unwrap_or(target.name.clone()); + let already_seen = deduplicator.visit(package.name.clone(), name.clone()); + if already_seen { + continue; + } + let file_path = target_dir.join(format!("{}.test.json", name)); let test_compilation = serde_json::from_str::( &fs::read_to_string(file_path.clone()) .with_context(|| format!("failed to read file: {file_path}"))?, @@ -86,6 +98,18 @@ fn main() -> Result<()> { Ok(()) } +#[derive(Default)] +struct TargetGroupDeduplicator { + seen: HashSet<(String, String)>, +} + +impl TargetGroupDeduplicator { + /// Returns true if already visited. + pub fn visit(&mut self, package_name: String, group_name: String) -> bool { + !self.seen.insert((package_name, group_name)) + } +} + fn is_gas_enabled(metadata: &Metadata, package_id: &PackageId, target: &TargetMetadata) -> bool { metadata .compilation_units diff --git a/extensions/scarb-cairo-test/tests/build.rs b/extensions/scarb-cairo-test/tests/build.rs index 24d23f96b..f90920f88 100644 --- a/extensions/scarb-cairo-test/tests/build.rs +++ b/extensions/scarb-cairo-test/tests/build.rs @@ -1,5 +1,5 @@ use assert_fs::TempDir; -use indoc::indoc; +use indoc::{formatdoc, indoc}; use scarb_test_support::command::Scarb; use scarb_test_support::project_builder::ProjectBuilder; @@ -187,3 +187,59 @@ fn features_test_build_failed() { error: process did not exit successfully: exit code: 1 "#}); } + +#[test] +fn integration_tests() { + let t = TempDir::new().unwrap(); + let test_case = indoc! {r#" + #[cfg(test)] + mod tests { + use hello::fib; + + #[test] + fn it_works() { + assert(fib(16) == 987, 'it works!'); + } + } + "#}; + ProjectBuilder::start() + .name("hello") + .lib_cairo(formatdoc! {r#" + fn fib(mut n: u32) -> u32 {{ + let mut a: u32 = 0; + let mut b: u32 = 1; + while n != 0 {{ + n = n - 1; + let temp = b; + b = a + b; + a = temp; + }}; + a + }} + + {test_case} + "#}) + .src("tests/a.cairo", test_case) + .src("tests/b.cairo", test_case) + .build(&t); + Scarb::quick_snapbox() + .arg("cairo-test") + .current_dir(&t) + .assert() + .success() + .stdout_matches(indoc! {r#" + [..]Compiling test(hello_unittest) hello v1.0.0 ([..]Scarb.toml) + [..]Compiling test(hello_integrationtest) hello_a v1.0.0 ([..]Scarb.toml) + [..]Compiling test(hello_integrationtest) hello_b v1.0.0 ([..]Scarb.toml) + [..]Finished release target(s) in [..] + testing hello ... + running 1 test + test hello_b::b::tests::it_works ... ok (gas usage est.: 42440) + test result: ok. 1 passed; 0 failed; 0 ignored; 0 filtered out; + + running 1 test + test hello::tests::it_works ... ok (gas usage est.: 42440) + test result: ok. 1 passed; 0 failed; 0 ignored; 0 filtered out; + + "#}); +} diff --git a/scarb/src/compiler/compilation_unit.rs b/scarb/src/compiler/compilation_unit.rs index 04951c0a4..cdd20a171 100644 --- a/scarb/src/compiler/compilation_unit.rs +++ b/scarb/src/compiler/compilation_unit.rs @@ -1,12 +1,15 @@ use std::fmt::Write; use std::hash::{Hash, Hasher}; +use anyhow::{ensure, Result}; use cairo_lang_filesystem::cfg::CfgSet; +use itertools::Itertools; +use serde::{Deserialize, Serialize}; use smol_str::SmolStr; use typed_builder::TypedBuilder; use crate::compiler::Profile; -use crate::core::{ManifestCompilerConfig, Package, PackageId, Target, Workspace}; +use crate::core::{ManifestCompilerConfig, Package, PackageId, Target, TargetKind, Workspace}; use crate::flock::Filesystem; use scarb_stable_hash::StableHasher; @@ -75,7 +78,7 @@ pub struct CompilationUnitComponent { /// The Scarb [`Package`] to be built. pub package: Package, /// Information about the specific target to build, out of the possible targets in `package`. - pub target: Target, + pub targets: Vec, /// Items for the Cairo's `#[cfg(...)]` attribute to be enabled in this component. pub cfg_set: Option, } @@ -101,10 +104,6 @@ pub trait CompilationUnitAttributes { component } - fn target(&self) -> &Target { - &self.main_component().target - } - fn id(&self) -> String { format!("{}-{}", self.main_package_id().name, self.digest()) } @@ -121,18 +120,18 @@ pub trait CompilationUnitAttributes { } fn has_custom_name(&self) -> bool { - self.main_component().target.kind.as_str() != self.main_package_id().name.as_str() + self.main_component().target_kind().as_str() != self.main_package_id().name.as_str() } fn name(&self) -> String { let mut string = String::new(); let main_component = self.main_component(); - if self.is_sole_for_package() || self.target().is_test() { - write!(&mut string, "{}", main_component.target.kind).unwrap(); + if self.is_sole_for_package() || self.main_component().target_kind().is_test() { + write!(&mut string, "{}", main_component.target_kind()).unwrap(); if self.has_custom_name() { - write!(&mut string, "({})", main_component.target.name).unwrap(); + write!(&mut string, "({})", main_component.target_name()).unwrap(); } write!(&mut string, " ").unwrap(); @@ -218,15 +217,111 @@ impl CairoCompilationUnit { pub fn target_dir(&self, ws: &Workspace<'_>) -> Filesystem { ws.target_dir().child(self.profile.as_str()) } + + /// Rewrite single compilation unit with multiple targets, into multiple compilation units + /// with single targets. + pub fn rewrite_to_single_source_paths(&self) -> Vec { + let rewritten_main = self + .main_component() + .targets + .iter() + .map(|target| { + let mut main = self.main_component().clone(); + main.targets = vec![target.clone()]; + main + }) + .collect_vec(); + + let mut components = self.components.clone(); + components.remove(0); + + rewritten_main + .into_iter() + .map(|component| { + let mut unit = self.clone(); + unit.components = vec![component]; + unit.components.extend(components.clone()); + unit + }) + .collect_vec() + } } impl CompilationUnitComponent { + /// Validate input and create new [CompilationUnitComponent] instance. + pub fn try_new( + package: Package, + targets: Vec, + cfg_set: Option, + ) -> Result { + ensure!( + !targets.is_empty(), + "a compilation unit component must have at least one target" + ); + ensure!( + targets + .iter() + .map(|t| &t.kind) + .collect::>() + .len() + == 1, + "all targets in a compilation unit component must have the same kind" + ); + ensure!( + targets + .iter() + .map(|t| &t.params) + .all(|p| *p == targets[0].params), + "all targets in a compilation unit component must have the same params" + ); + ensure!( + targets + .iter() + .map(|t| t.source_root()) + .all(|p| p == targets[0].source_root()), + "all targets in a compilation unit component must have the same source path parent" + ); + if targets.len() > 1 { + ensure!( + targets.iter().all(|t| t.group_id.is_some()), + "all targets in a compilation unit component with multiple targets must have group_id defined" + ); + } + Ok(Self { + package, + targets, + cfg_set, + }) + } + + pub fn first_target(&self) -> &Target { + &self.targets[0] + } + + pub fn target_kind(&self) -> TargetKind { + self.first_target().kind.clone() + } + + pub fn target_props<'de, P>(&self) -> Result

+ where + P: Default + Serialize + Deserialize<'de>, + { + self.first_target().props::

() + } + + pub fn target_name(&self) -> SmolStr { + self.first_target() + .group_id + .clone() + .unwrap_or(self.first_target().name.clone()) + } + pub fn cairo_package_name(&self) -> SmolStr { self.package.id.name.to_smol_str() } fn hash(&self, hasher: &mut impl Hasher) { self.package.id.hash(hasher); - self.target.hash(hasher); + self.targets.hash(hasher); } } diff --git a/scarb/src/compiler/compilers/lib.rs b/scarb/src/compiler/compilers/lib.rs index ddc400ef3..948969022 100644 --- a/scarb/src/compiler/compilers/lib.rs +++ b/scarb/src/compiler/compilers/lib.rs @@ -46,7 +46,7 @@ impl Compiler for LibCompiler { db: &mut RootDatabase, ws: &Workspace<'_>, ) -> Result<()> { - let props: Props = unit.target().props()?; + let props: Props = unit.main_component().target_props()?; if !props.sierra && !props.casm && !props.sierra_text { ws.config().ui().warn( "Sierra, textual Sierra and CASM lib targets have been disabled, \ @@ -74,20 +74,23 @@ impl Compiler for LibCompiler { if props.sierra { write_json( - format!("{}.sierra.json", unit.target().name).as_str(), + format!("{}.sierra.json", unit.main_component().target_name()).as_str(), "output file", &target_dir, ws, &sierra_program, ) .with_context(|| { - format!("failed to serialize Sierra program {}", unit.target().name) + format!( + "failed to serialize Sierra program {}", + unit.main_component().target_name() + ) })?; } if props.sierra_text { write_string( - format!("{}.sierra", unit.target().name).as_str(), + format!("{}.sierra", unit.main_component().target_name()).as_str(), "output file", &target_dir, ws, @@ -121,7 +124,7 @@ impl Compiler for LibCompiler { }; write_string( - format!("{}.casm", unit.target().name).as_str(), + format!("{}.casm", unit.main_component().target_name()).as_str(), "output file", &target_dir, ws, diff --git a/scarb/src/compiler/compilers/starknet_contract.rs b/scarb/src/compiler/compilers/starknet_contract.rs index b92f1e362..1e626a7cf 100644 --- a/scarb/src/compiler/compilers/starknet_contract.rs +++ b/scarb/src/compiler/compilers/starknet_contract.rs @@ -207,7 +207,7 @@ impl Compiler for StarknetContractCompiler { db: &mut RootDatabase, ws: &Workspace<'_>, ) -> Result<()> { - let props: Props = unit.target().props()?; + let props: Props = unit.main_component().target_props()?; if !props.sierra && !props.casm { ws.config().ui().warn( "both Sierra and CASM Starknet contract targets have been disabled, \ @@ -276,7 +276,7 @@ impl Compiler for StarknetContractCompiler { let mut artifacts = StarknetArtifacts::default(); let mut file_stem_calculator = ContractFileStemCalculator::new(contract_paths); - let target_name = &unit.target().name; + let target_name = &unit.main_component().target_name(); for (decl, class, casm_class) in izip!(contracts, classes, casm_classes) { let contract_name = decl.submodule_id.name(db.upcast_mut()); let contract_path = decl.module_id().full_path(db.upcast_mut()); diff --git a/scarb/src/compiler/compilers/test.rs b/scarb/src/compiler/compilers/test.rs index 3af12ae4f..8fa921c67 100644 --- a/scarb/src/compiler/compilers/test.rs +++ b/scarb/src/compiler/compilers/test.rs @@ -44,7 +44,7 @@ impl Compiler for TestCompiler { { let _ = trace_span!("serialize_test").enter(); - let file_name = format!("{}.test.json", unit.target().name); + let file_name = format!("{}.test.json", unit.main_component().target_name()); write_json( &file_name, "output file", diff --git a/scarb/src/compiler/db.rs b/scarb/src/compiler/db.rs index 0044a946e..7b0721c7a 100644 --- a/scarb/src/compiler/db.rs +++ b/scarb/src/compiler/db.rs @@ -65,7 +65,7 @@ fn load_plugins( /// /// This approach allows compiling crates that do not define `lib.cairo` file. /// For example, single file crates can be created this way. -/// The actual single file module is defined as `mod` item in created lib file. +/// The actual single file modules are defined as `mod` items in created lib file. fn inject_virtual_wrapper_lib(db: &mut RootDatabase, unit: &CairoCompilationUnit) -> Result<()> { let components: Vec<&CompilationUnitComponent> = unit .components @@ -73,29 +73,42 @@ fn inject_virtual_wrapper_lib(db: &mut RootDatabase, unit: &CairoCompilationUnit .filter(|component| !component.package.id.is_core()) // Skip components defining the default source path, as they already define lib.cairo files. .filter(|component| { - component - .target - .source_path - .file_name() - .map(|file_name| file_name != DEFAULT_MODULE_MAIN_FILE) - .unwrap_or(false) + !component.targets.is_empty() + && (component.targets.len() > 1 + || component + .first_target() + .source_path + .file_name() + .map(|file_name| file_name != DEFAULT_MODULE_MAIN_FILE) + .unwrap_or(false)) }) .collect(); for component in components { let crate_name = component.cairo_package_name(); let crate_id = db.intern_crate(CrateLongId::Real(crate_name)); - let file_stem = component.target.source_path.file_stem().ok_or_else(|| { - anyhow!( - "failed to get file stem for component {}", - component.target.source_path - ) - })?; + let file_stems = component + .targets + .iter() + .map(|target| { + target + .source_path + .file_stem() + .map(|file_stem| format!("mod {file_stem};")) + .ok_or_else(|| { + anyhow!( + "failed to get file stem for component {}", + target.source_path + ) + }) + }) + .collect::>>()?; + let content = file_stems.join("\n"); let module_id = ModuleId::CrateRoot(crate_id); let file_id = db.module_main_file(module_id).unwrap(); // Inject virtual lib file wrapper. db.as_files_group_mut() - .override_file_content(file_id, Some(Arc::new(format!("mod {file_stem};")))); + .override_file_content(file_id, Some(Arc::new(content))); } Ok(()) @@ -109,7 +122,7 @@ fn build_project_config(unit: &CairoCompilationUnit) -> Result { .map(|component| { ( component.cairo_package_name(), - component.target.source_root().into(), + component.first_target().source_root().into(), ) }) .collect(); @@ -142,7 +155,7 @@ fn build_project_config(unit: &CairoCompilationUnit) -> Result { let corelib = unit .core_package_component() - .map(|core| Directory::Real(core.target.source_root().into())); + .map(|core| Directory::Real(core.first_target().source_root().into())); let content = ProjectConfigContent { crate_roots, diff --git a/scarb/src/compiler/repository.rs b/scarb/src/compiler/repository.rs index 37e4479e9..523f5a5eb 100644 --- a/scarb/src/compiler/repository.rs +++ b/scarb/src/compiler/repository.rs @@ -47,7 +47,7 @@ impl CompilerRepository { db: &mut RootDatabase, ws: &Workspace<'_>, ) -> Result<()> { - let target_kind = &unit.target().kind; + let target_kind = &unit.main_component().target_kind(); let Some(compiler) = self.compilers.get(target_kind.as_str()) else { bail!("unknown compiler for target `{target_kind}`"); }; diff --git a/scarb/src/core/manifest/toml_manifest.rs b/scarb/src/core/manifest/toml_manifest.rs index 11e5f01e5..075e20299 100644 --- a/scarb/src/core/manifest/toml_manifest.rs +++ b/scarb/src/core/manifest/toml_manifest.rs @@ -718,7 +718,7 @@ impl TomlManifest { Some(&target_config), &package_name, root, - Some("integration_tests".into()), + Some(format!("{package_name}_integrationtest").into()), )?); } } diff --git a/scarb/src/ops/compile.rs b/scarb/src/ops/compile.rs index ee6c60ee4..e8a47c8e1 100644 --- a/scarb/src/ops/compile.rs +++ b/scarb/src/ops/compile.rs @@ -99,9 +99,13 @@ where let compilation_units = ops::generate_compilation_units(&resolve, &opts.features, ws)? .into_iter() .filter(|cu| { - let is_excluded = opts.exclude_targets.contains(&cu.target().kind); - let is_included = - opts.include_targets.is_empty() || opts.include_targets.contains(&cu.target().kind); + let is_excluded = opts + .exclude_targets + .contains(&cu.main_component().target_kind()); + let is_included = opts.include_targets.is_empty() + || opts + .include_targets + .contains(&cu.main_component().target_kind()); let is_selected = packages.contains(&cu.main_package_id()); let is_cairo_plugin = matches!(cu, CompilationUnit::ProcMacro(_)); is_cairo_plugin || (is_selected && is_included && !is_excluded) @@ -204,7 +208,9 @@ fn check_starknet_dependency( // `starknet` dependency will error in 99% real-world Starknet contract projects. // I think we can get away with emitting false positives for users who write raw contracts // without using Starknet code generators. Such people shouldn't do what they do 😁 - if unit.target().kind == TargetKind::STARKNET_CONTRACT && !has_starknet_plugin(db) { + if unit.main_component().target_kind() == TargetKind::STARKNET_CONTRACT + && !has_starknet_plugin(db) + { ws.config().ui().warn(formatdoc! { r#" package `{package_name}` declares `starknet-contract` target, but does not depend on `starknet` package diff --git a/scarb/src/ops/metadata.rs b/scarb/src/ops/metadata.rs index 69089f4fe..cc0087d76 100644 --- a/scarb/src/ops/metadata.rs +++ b/scarb/src/ops/metadata.rs @@ -46,7 +46,7 @@ pub fn collect_metadata(opts: &MetadataOptions, ws: &Workspace<'_>) -> Result = ops::generate_compilation_units(&resolve, &opts.features, ws)? .iter() - .map(collect_compilation_unit_metadata) + .flat_map(collect_compilation_unit_metadata) .collect(); (packages, compilation_units) @@ -212,10 +212,14 @@ fn collect_target_metadata(target: &Target) -> m::TargetMetadata { fn collect_compilation_unit_metadata( compilation_unit: &CompilationUnit, -) -> m::CompilationUnitMetadata { +) -> Vec { match compilation_unit { - CompilationUnit::Cairo(cu) => collect_cairo_compilation_unit_metadata(cu), - CompilationUnit::ProcMacro(cu) => collect_proc_macro_compilation_unit_metadata(cu), + CompilationUnit::Cairo(cu) => cu + .rewrite_to_single_source_paths() + .into_iter() + .map(|cu| collect_cairo_compilation_unit_metadata(&cu)) + .collect_vec(), + CompilationUnit::ProcMacro(cu) => vec![collect_proc_macro_compilation_unit_metadata(cu)], } } @@ -254,10 +258,20 @@ fn collect_cairo_compilation_unit_metadata( .map(|c| c.package.to_string()) .collect::>(); + assert_eq!( + compilation_unit.main_component().targets.len(), + 1, + "compilation unit should have been rewritten to have a single target" + ); + m::CompilationUnitMetadataBuilder::default() .id(compilation_unit.id()) .package(wrap_package_id(compilation_unit.main_package_id())) - .target(collect_target_metadata(compilation_unit.target())) + .target(collect_target_metadata( + // We use first_target, as compilation units with multiple targets + // have already been rewritten to single target ones. + &compilation_unit.main_component().first_target().clone(), + )) .components(components) .cairo_plugins(cairo_plugins) .compiler_config(compiler_config) @@ -274,10 +288,17 @@ fn collect_proc_macro_compilation_unit_metadata( compilation_unit: &ProcMacroCompilationUnit, ) -> m::CompilationUnitMetadata { let components = collect_compilation_unit_components(compilation_unit.components.iter()); + assert_eq!( + compilation_unit.main_component().targets.len(), + 1, + "proc macro compilation unit should have only one target" + ); m::CompilationUnitMetadataBuilder::default() .id(compilation_unit.id()) .package(wrap_package_id(compilation_unit.main_package_id())) - .target(collect_target_metadata(compilation_unit.target())) + .target(collect_target_metadata( + &compilation_unit.main_component().first_target().clone(), + )) .components(components) .cairo_plugins(Vec::new()) .compiler_config(serde_json::Value::Null) @@ -298,7 +319,9 @@ where m::CompilationUnitComponentMetadataBuilder::default() .package(wrap_package_id(c.package.id)) .name(c.cairo_package_name()) - .source_path(c.target.source_path.clone()) + // We use first_target, as compilation units with multiple targets + // have already been rewritten to single target ones. + .source_path(c.first_target().source_path.clone()) .cfg(c.cfg_set.as_ref().map(|cfg_set| cfg_set .iter() .map(|cfg| { diff --git a/scarb/src/ops/resolve.rs b/scarb/src/ops/resolve.rs index 9514d1e9a..c41222883 100644 --- a/scarb/src/ops/resolve.rs +++ b/scarb/src/ops/resolve.rs @@ -290,11 +290,7 @@ fn cairo_compilation_unit_for_target( } }; - Ok(CompilationUnitComponent { - package, - target, - cfg_set, - }) + CompilationUnitComponent::try_new(package, vec![target], cfg_set) }) .collect::>()?; @@ -314,11 +310,11 @@ fn cairo_compilation_unit_for_target( }); // Add `lib` target for tested package, to be available as dependency. - components.push(CompilationUnitComponent { - package: member.clone(), - cfg_set: None, - target, - }); + components.push(CompilationUnitComponent::try_new( + member.clone(), + vec![target], + None, + )?); // Set test package as main package for this compilation unit. test_package_id @@ -536,14 +532,14 @@ fn generate_cairo_plugin_compilation_units(member: &Package) -> Result(); - let content = t.child("target/dev/hello_test1.test.json").read_to_string(); + let content = t + .child("target/dev/hello_integrationtest.test.json") + .read_to_string(); let json: serde_json::Value = serde_json::from_str(&content).unwrap(); let tests = json.get("named_tests").unwrap().as_array().unwrap(); assert_eq!(tests.len(), 1);