-
Notifications
You must be signed in to change notification settings - Fork 205
fix(sozo): rework the detection of Dojo default package #3327
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ use dojo_world::local::WorldLocal; | |
| use scarb_interop::fsx; | ||
| use scarb_metadata::{DepKind, Metadata, MetadataCommand, MetadataCommandError, PackageMetadata}; | ||
| use serde::Serialize; | ||
| use tracing::debug; | ||
|
|
||
| #[derive(Debug, PartialEq)] | ||
| pub enum TestRunner { | ||
|
|
@@ -20,7 +21,7 @@ pub enum TestRunner { | |
|
|
||
| const CAIRO_TEST_RUNNER_NAME: &str = "cairo_test"; | ||
| const SNF_TEST_RUNNER_NAME: &str = "snforge_std"; | ||
| const DOJO_MACROS_NAME: &str = "dojo_macros"; | ||
| const DOJO_WORLD_NAME: &str = "dojo::world::world_contract::world"; | ||
|
|
||
| impl From<&String> for TestRunner { | ||
| fn from(value: &String) -> Self { | ||
|
|
@@ -297,30 +298,61 @@ enum WorldPackageResult { | |
| /// performing some tasks like binding generation and migrations. | ||
| /// | ||
| /// To solve the issue where a virtual workspace has no package name, this function looks for a | ||
| /// package that is *not* a `lib` target and uses `dojo_macros` as a dependency. | ||
| /// If it finds exactly one package that matches the criteria, it returns the package name. | ||
| /// package that has a starknet-contract target that has a build-external-contracts parameter that | ||
| /// contains the dojo world. We can't rely on the package being a library, since some libraries | ||
| /// might be used in the contracts to be deployed with a world. For this reason, we first look for a | ||
| /// package that is not a library and then a package that is a library. If it finds exactly one | ||
| /// package that matches the criteria, it returns the package name. | ||
| /// | ||
| /// Otherwise, it returns a `MultiplePackages` variant, which will be handled by the caller, which | ||
| /// generally should indicate to the user that using multiple packages with contracts to be deployed | ||
| /// with a world is not supported at the workspace level. Therefore, the user will want to build and | ||
| /// migrate the package with contracts themselves by going into the package directory and running | ||
| /// the commands. | ||
| fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult { | ||
| let mut candidates = Vec::new(); | ||
| let mut not_lib_candidates = Vec::new(); | ||
| let mut lib_candidates = Vec::new(); | ||
|
|
||
| for p in packages { | ||
| if p.targets.iter().all(|t| t.kind != "lib") { | ||
| for d in &p.dependencies { | ||
| if d.name == DOJO_MACROS_NAME { | ||
| candidates.push(p.name.clone()); | ||
| let is_lib = p.targets.iter().any(|t| t.kind == "lib"); | ||
|
|
||
| debug!(%p.name, is_lib, "Verifying package type."); | ||
|
|
||
| for t in &p.targets { | ||
| if t.kind == "starknet-contract" { | ||
| if let Some(build_external_contracts) = | ||
| t.params.as_object().and_then(|obj| obj.get("build-external-contracts")) | ||
| { | ||
| if let Some(build_external_contracts) = build_external_contracts.as_array() { | ||
| if build_external_contracts | ||
| .contains(&serde_json::Value::String(DOJO_WORLD_NAME.to_string())) | ||
| { | ||
| if is_lib { | ||
| lib_candidates.push(p.name.clone()); | ||
| } else { | ||
| not_lib_candidates.push(p.name.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
312
to
340
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainLimit detection to workspace members only default_dojo_package_name scans all self.packages via caller, which commonly includes dependencies. This can pick a dependency’s package (or cause false “Multiple packages” errors).
Example (outside this hunk, for workspace_package_name): let root = &self.workspace.root;
let pkgs = self
.packages
.iter()
.filter(|p| p.manifest_path.starts_with(root));
match default_dojo_package_name(pkgs) { /* unchanged */ }To support that, generalize the helper signature in this hunk: -fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult {
+fn default_dojo_package_name<'a, I>(packages: I) -> WorldPackageResult
+where
+ I: IntoIterator<Item = &'a PackageMetadata>,
+{
- for p in packages {
+ for p in packages {
// unchanged
}Verification prompts: 🌐 Web query: 💡 Result: Short answer: Yes — the metadata "packages" array contains the resolved packages for the project (workspace members plus their dependencies), not only workspace members. (docs.swmansion.com, docs.rs) Citations:
Filter default_dojo_package_name inputs to workspace members only
let root = &self.workspace.root;
let pkgs = self
.packages
.iter()
.filter(|p| p.manifest_path.starts_with(root));
match default_dojo_package_name(pkgs) { /* unchanged */ }
-fn default_dojo_package_name(packages: &[PackageMetadata]) -> WorldPackageResult {
+fn default_dojo_package_name<'a, I>(packages: I) -> WorldPackageResult
+where
+ I: IntoIterator<Item = &'a PackageMetadata>,
{
// unchanged
}🤖 Prompt for AI Agents |
||
|
|
||
| match candidates.len() { | ||
| 0 => WorldPackageResult::NoPackage, | ||
| 1 => WorldPackageResult::Ok(candidates[0].clone()), | ||
| _ => WorldPackageResult::MultiplePackages(candidates), | ||
| } | ||
| debug!(?not_lib_candidates, ?lib_candidates, "Collect candidates for Sozo build."); | ||
|
|
||
| // Prioritize not lib candidates, then lib candidates. | ||
| let r = match not_lib_candidates.len() { | ||
| 0 => match lib_candidates.len() { | ||
| 0 => WorldPackageResult::NoPackage, | ||
| 1 => WorldPackageResult::Ok(lib_candidates[0].clone()), | ||
| _ => WorldPackageResult::MultiplePackages(lib_candidates), | ||
| }, | ||
| 1 => WorldPackageResult::Ok(not_lib_candidates[0].clone()), | ||
| _ => WorldPackageResult::MultiplePackages(not_lib_candidates), | ||
| }; | ||
|
|
||
| debug!(?r, "Extract default dojo package name from workspace."); | ||
|
|
||
| r | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential compile error in tracing field shorthand
debug!(%p.name, ...) is not valid shorthand because p.name isn’t an identifier. Give the field an explicit key.
📝 Committable suggestion
🤖 Prompt for AI Agents