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

Require explicit dependence on cairo_test (i.e. test_plugin)" #1359

Merged
merged 1 commit into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions examples/dependencies/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ version = "0.1.0"

[dependencies]
alexandria_math = { git = "https://github.com/keep-starknet-strange/alexandria.git" }

[dev-dependencies]
cairo_test = "2.6.0"
3 changes: 3 additions & 0 deletions examples/hello_world/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ edition = "2023_10"
# See more keys and their definitions at https://docs.swmansion.com/scarb/docs/reference/manifest.html

[dependencies]

[dev-dependencies]
cairo_test = "2.6.0"
3 changes: 3 additions & 0 deletions examples/starknet_hello_world/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@ version = "0.1.0"
[dependencies]
starknet = "2.6.0"

[dev-dependencies]
cairo_test = "2.6.0"

[[target.starknet-contract]]
3 changes: 3 additions & 0 deletions examples/starknet_multiple_contracts/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@ version = "0.1.0"
[dependencies]
starknet = "2.6.0"

[dev-dependencies]
cairo_test = "2.6.0"

[[target.starknet-contract]]
4 changes: 4 additions & 0 deletions examples/workspaces/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ test = "snforge"
exit_first = true

[workspace.dependencies]
cairo_test = "2.6.0"
starknet = "2.6.0"

[workspace.package]
Expand All @@ -30,4 +31,7 @@ starknet.workspace = true
fibonacci = { path = "crates/fibonacci" }
addition = { path = "crates/addition" }

[dev-dependencies]
cairo_test.workspace = true

[[target.starknet-contract]]
3 changes: 3 additions & 0 deletions examples/workspaces/crates/addition/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@ name = "addition"
version.workspace = true

[lib]

[dev-dependencies]
cairo_test.workspace = true
3 changes: 3 additions & 0 deletions examples/workspaces/crates/fibonacci/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ snforge.workspace = true

[dependencies]
addition = { path = "../addition" }

[dev-dependencies]
cairo_test.workspace = true
4 changes: 4 additions & 0 deletions extensions/scarb-cairo-test/tests/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ fn can_test_without_gas() {
}
}
"#})
.dep_cairo_test()
.manifest_extra(indoc! {r#"
[cairo]
enable-gas = false
Expand Down Expand Up @@ -86,6 +87,7 @@ fn can_print_test_resources() {
}
}
"#})
.dep_cairo_test()
.build(&t);
Scarb::quick_snapbox()
.arg("cairo-test")
Expand Down Expand Up @@ -130,6 +132,7 @@ fn get_features_test_build(t: &TempDir) {
}
}
"#})
.dep_cairo_test()
.build(t);
}

Expand Down Expand Up @@ -219,6 +222,7 @@ fn integration_tests() {
{test_case}
"#})
.dep_cairo_test()
.src("tests/a.cairo", test_case)
.src("tests/b.cairo", test_case)
.build(&t);
Expand Down
22 changes: 1 addition & 21 deletions scarb/src/core/manifest/summary.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::collections::HashSet;
use std::ops::Deref;
use std::sync::Arc;

Expand All @@ -8,8 +7,7 @@ use typed_builder::TypedBuilder;
#[cfg(doc)]
use crate::core::Manifest;
use crate::core::{
Checksum, DepKind, DependencyVersionReq, ManifestDependency, PackageId, PackageName, SourceId,
TargetKind,
Checksum, DepKind, DependencyVersionReq, ManifestDependency, PackageId, PackageName,
};

/// Subset of a [`Manifest`] that contains only the most important information about a package.
Expand All @@ -27,7 +25,6 @@ pub struct SummaryInner {
pub package_id: PackageId,
#[builder(default)]
pub dependencies: Vec<ManifestDependency>,
pub target_kinds: HashSet<TargetKind>,
#[builder(default = false)]
pub no_core: bool,
#[builder(default)]
Expand Down Expand Up @@ -79,27 +76,10 @@ impl Summary {
.version_req(DependencyVersionReq::exact(&cairo_version))
.build()
});

static TEST_PLUGIN_DEPENDENCY: Lazy<ManifestDependency> = Lazy::new(|| {
// NOTE: Pin test plugin to exact version, because we know that's the only one we have.
maciektr marked this conversation as resolved.
Show resolved Hide resolved
let cairo_version = crate::version::get().cairo.version.parse().unwrap();
ManifestDependency::builder()
.kind(DepKind::Target(TargetKind::TEST))
.name(PackageName::TEST_PLUGIN)
.source_id(SourceId::default())
.version_req(DependencyVersionReq::exact(&cairo_version))
.build()
});

let mut deps: Vec<&ManifestDependency> = Vec::new();

if !self.no_core {
deps.push(&CORE_DEPENDENCY);
}
if self.target_kinds.contains(&TargetKind::TEST) {
deps.push(&TEST_PLUGIN_DEPENDENCY);
}

deps.into_iter()
}

Expand Down
1 change: 0 additions & 1 deletion scarb/src/core/manifest/toml_manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,6 @@ impl TomlManifest {
let targets = self.collect_targets(package.name.to_smol_str(), root)?;

let summary = Summary::builder()
.target_kinds(targets.iter().map(|t| t.kind.clone()).collect())
.package_id(package_id)
.dependencies(dependencies)
.no_core(no_core)
Expand Down
5 changes: 1 addition & 4 deletions scarb/src/core/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ pub(crate) mod mock {

use crate::core::package::PackageName;
use crate::core::registry::Registry;
use crate::core::{
ManifestBuilder, ManifestDependency, Package, PackageId, SourceId, Summary, TargetKind,
};
use crate::core::{ManifestBuilder, ManifestDependency, Package, PackageId, SourceId, Summary};

#[derive(Debug, Default)]
pub struct MockRegistry {
Expand Down Expand Up @@ -111,7 +109,6 @@ pub(crate) mod mock {

fn build_package(package_id: PackageId, dependencies: Vec<ManifestDependency>) -> Package {
let summary = Summary::builder()
.target_kinds(HashSet::from_iter(vec![TargetKind::LIB]))
.package_id(package_id)
.dependencies(dependencies)
.no_core(package_id.is_core())
Expand Down
23 changes: 20 additions & 3 deletions scarb/src/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,7 @@ pub fn resolve_workspace_with_opts(
read_lockfile(ws)?
};

let resolve =
resolver::resolve(&members_summaries, &patched, lockfile, ws.config().ui()).await?;
let resolve = resolver::resolve(&members_summaries, &patched, lockfile).await?;

write_lockfile(Lockfile::from_resolve(&resolve), ws)?;

Expand Down Expand Up @@ -242,6 +241,7 @@ fn generate_cairo_compilation_units(
.into_iter()
.chain(grouped)
.collect();
solution.show_warnings();
Ok(result)
}

Expand Down Expand Up @@ -433,6 +433,7 @@ pub struct PackageSolutionCollector<'a> {
packages: Option<Vec<Package>>,
cairo_plugins: Option<Vec<CompilationUnitCairoPlugin>>,
target_kind: Option<TargetKind>,
warnings: HashSet<String>,
mkaput marked this conversation as resolved.
Show resolved Hide resolved
}

impl<'a> PackageSolutionCollector<'a> {
Expand All @@ -444,6 +445,7 @@ impl<'a> PackageSolutionCollector<'a> {
packages: None,
cairo_plugins: None,
target_kind: None,
warnings: HashSet::new(),
}
}

Expand All @@ -464,7 +466,7 @@ impl<'a> PackageSolutionCollector<'a> {
}

fn pull_from_graph(
&self,
&mut self,
target_kind: &TargetKind,
) -> Result<(Vec<Package>, Vec<CompilationUnitCairoPlugin>)> {
let mut classes = self
Expand Down Expand Up @@ -504,6 +506,15 @@ impl<'a> PackageSolutionCollector<'a> {

check_cairo_version_compatibility(&packages, self.ws)?;

// Print warnings for dependencies that are not usable.
let other = classes.remove(&PackageClass::Other).unwrap_or_default();
for pkg in other {
self.warnings.insert(format!(
"{} ignoring invalid dependency `{}` which is missing a lib or cairo-plugin target",
self.member.id, pkg.id.name
));
}

let cairo_plugins = cairo_plugins
.into_iter()
.map(|package| {
Expand All @@ -519,6 +530,12 @@ impl<'a> PackageSolutionCollector<'a> {

Ok((packages, cairo_plugins))
}

pub fn show_warnings(self) {
for warning in self.warnings {
self.ws.config().ui().warn(warning);
}
}
}

/// Build a set of `cfg` items to enable while building the compilation unit.
Expand Down
16 changes: 1 addition & 15 deletions scarb/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::collections::HashMap;
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;
Expand Down Expand Up @@ -38,7 +37,6 @@ pub async fn resolve(
summaries: &[Summary],
registry: &dyn Registry,
lockfile: Lockfile,
ui: Ui,
) -> Result<Resolve> {
// TODO(#2): This is very bad, use PubGrub here.
let mut graph = DiGraphMap::<PackageId, DependencyEdge>::new();
Expand Down Expand Up @@ -90,15 +88,6 @@ pub async fn resolve(
};
let dep = dep_summary.package_id;

if !(dep_summary.target_kinds.contains(&TargetKind::CAIRO_PLUGIN)
|| dep_summary.target_kinds.contains(&TargetKind::LIB))
{
ui.warn(format!(
"{} ignoring invalid dependency `{}` which is missing a lib or cairo-plugin target",
package_id, dep.name
));
}

if let Some(existing) = packages.get(dep.name.as_ref()) {
if existing.source_id != dep.source_id {
bail!(
Expand Down Expand Up @@ -215,8 +204,6 @@ mod tests {
use anyhow::Result;
use indoc::indoc;
use itertools::Itertools;
use scarb_ui::Verbosity::Verbose;
use scarb_ui::{OutputFormat, Ui};
use semver::Version;
use similar_asserts::assert_serde_eq;
use tokio::runtime::Builder;
Expand Down Expand Up @@ -300,8 +287,7 @@ mod tests {
.collect_vec();

let lockfile = Lockfile::new(locks.iter().cloned());
let ui = Ui::new(Verbose, OutputFormat::Text);
runtime.block_on(super::resolve(&summaries, &registry, lockfile, ui))
runtime.block_on(super::resolve(&summaries, &registry, lockfile))
}

fn package_id<S: AsRef<str>>(name: S) -> PackageId {
Expand Down
4 changes: 1 addition & 3 deletions scarb/src/sources/registry.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use std::collections::HashSet;
use std::fmt;

use anyhow::{anyhow, bail, Context, Result};
Expand All @@ -16,7 +15,7 @@ use crate::core::registry::package_source_store::PackageSourceStore;
use crate::core::source::Source;
use crate::core::{
Checksum, Config, DependencyVersionReq, ManifestDependency, Package, PackageId, SourceId,
Summary, TargetKind,
Summary,
};
use crate::flock::FileLockGuard;
use crate::sources::PathSource;
Expand Down Expand Up @@ -105,7 +104,6 @@ impl<'c> Source for RegistrySource<'c> {
Summary::builder()
.package_id(package_id)
.dependencies(dependencies)
.target_kinds(HashSet::from_iter([TargetKind::LIB]))
.no_core(record.no_core)
.checksum(Some(record.checksum.clone()))
.build()
Expand Down
2 changes: 2 additions & 0 deletions scarb/tests/build_targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ fn compile_test_target() {
let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("hello")
.dep_cairo_test()
.lib_cairo(r#"fn f() -> felt252 { 42 }"#)
.build(&t);
t.child("tests").create_dir_all().unwrap();
Expand Down Expand Up @@ -365,6 +366,7 @@ fn integration_tests_do_not_enable_cfg_in_main_package() {
let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("hello")
.dep_cairo_test()
.lib_cairo(indoc! {r#"
#[cfg(test)]
fn f() -> felt252 { 42 }
Expand Down
3 changes: 3 additions & 0 deletions scarb/tests/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ fn expand_integration_test() {
"#};
ProjectBuilder::start()
.name("hello")
.dep_cairo_test()
.lib_cairo(formatdoc! {r#"
fn fib(mut n: u32) -> u32 {{
let mut a: u32 = 0;
Expand Down Expand Up @@ -170,6 +171,7 @@ fn can_select_target_by_kind() {
let t = TempDir::new().unwrap();
ProjectBuilder::start()
.name("hello")
.dep_cairo_test()
.lib_cairo(indoc! {r#"
fn hello() -> felt252 {
42
Expand Down Expand Up @@ -237,6 +239,7 @@ fn can_expand_multiple_targets() {
"#};
ProjectBuilder::start()
.name("hello")
.dep_cairo_test()
.lib_cairo(formatdoc! {r#"
fn fib(mut n: u32) -> u32 {{
let mut a: u32 = 0;
Expand Down
3 changes: 3 additions & 0 deletions scarb/tests/git_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -423,10 +423,12 @@ fn transitive_path_dep() {
let git_dep = gitx::new("dep1", |t| {
ProjectBuilder::start()
.name("dep0")
.dep_cairo_test()
.lib_cairo("fn hello() -> felt252 { 42 }")
.build(&t.child("zero"));
ProjectBuilder::start()
.name("dep1")
.dep_cairo_test()
.lib_cairo("fn hello() -> felt252 { dep0::hello() }")
.dep("dep0", Dep.path("../zero"))
.build(&t.child("one"));
Expand All @@ -436,6 +438,7 @@ fn transitive_path_dep() {
ProjectBuilder::start()
.name("hello")
.version("1.0.0")
.dep_cairo_test()
.dep("dep0", &git_dep)
.dep("dep1", &git_dep)
.lib_cairo("fn world() -> felt252 { dep1::hello() }")
Expand Down
Loading
Loading