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

feat(sozo): add full workspace and accept non-dojo targets #2207

Merged
merged 13 commits into from
Jul 25, 2024
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 19 additions & 24 deletions bin/sozo/src/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
use dojo_bindgen::{BuiltinPlugins, PluginManager};
use dojo_lang::scarb_internal::compile_workspace;
use dojo_world::manifest::MANIFESTS_DIR;
use dojo_world::metadata::dojo_metadata_from_workspace;
use prettytable::format::consts::FORMAT_NO_LINESEP_WITH_TITLE;
use prettytable::{format, Cell, Row, Table};
use scarb::core::{Config, TargetKind};
use scarb::core::{Config, Package, TargetKind};
use scarb::ops::CompileOpts;
use scarb_ui::args::FeaturesSpec;
use scarb_ui::args::{FeaturesSpec, PackagesFilter};
use sozo_ops::statistics::{get_contract_statistics_for_dir, ContractStatistics};
use tracing::trace;

Expand Down Expand Up @@ -43,39 +42,32 @@
/// Specify the features to activate.
#[command(flatten)]
pub features: FeaturesSpec,

/// Specify packages to build.
#[command(flatten)]
pub packages: Option<PackagesFilter>,
}

impl BuildArgs {
pub fn run(self, config: &Config) -> Result<()> {
let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;

if let Ok(current_package) = ws.current_package() {
if current_package.target(&TargetKind::new("dojo")).is_none() {
return Err(anyhow::anyhow!(
"No Dojo target found in the {} package. Add [[target.dojo]] to the {} \
manifest to enable Dojo features and compile with sozo.",
current_package.id.to_string(),
current_package.manifest_path()
));
}
}

// Namespaces are required to compute contracts/models data. Hence, we can't continue
// if no metadata are found.
// Once sozo will support package option, users will be able to do `-p` to select
// the package directly from the workspace instead of using `--manifest-path`.
let dojo_metadata = dojo_metadata_from_workspace(&ws)?;
let packages: Vec<Package> = if let Some(filter) = self.packages {
filter.match_many(&ws)?.into_iter().collect()

Check warning on line 56 in bin/sozo/src/commands/build.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/build.rs#L56

Added line #L56 was not covered by tests
} else {
ws.members().collect()
};

let profile_name =
ws.current_profile().expect("Scarb profile is expected at this point.").to_string();

// Manifest path is always a file, we can unwrap safely to get the
// parent folder.
// Manifest path is always a file, we can unwrap safely to get the parent folder.
let manifest_dir = ws.manifest_path().parent().unwrap().to_path_buf();

let profile_dir = manifest_dir.join(MANIFESTS_DIR).join(profile_name);
CleanArgs::clean_manifests(&profile_dir)?;
let packages: Vec<scarb::core::PackageId> = ws.members().map(|p| p.id).collect();

trace!(?packages);

let compile_info = compile_workspace(
config,
Expand All @@ -85,7 +77,7 @@
exclude_target_kinds: vec![TargetKind::TEST],
features: self.features.try_into()?,
},
packages,
packages.iter().map(|p| p.id).collect(),
)?;
trace!(?compile_info, "Compiled workspace.");

Expand Down Expand Up @@ -146,9 +138,11 @@
};
trace!(pluginManager=?bindgen, "Generating bindings.");

// TODO: check about the skip migration as now we process the metadata
// directly during the compilation to get the data we need from it.
tokio::runtime::Runtime::new()
.unwrap()
.block_on(bindgen.generate(dojo_metadata.skip_migration))
.block_on(bindgen.generate(None))
.expect("Error generating bindings");

Ok(())
Expand All @@ -167,6 +161,7 @@
unity: false,
bindings_output: "bindings".to_string(),
stats: false,
packages: None,
}
}
}
Expand Down
25 changes: 21 additions & 4 deletions bin/sozo/src/commands/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
use dojo_lang::scarb_internal::crates_config_for_compilation_unit;
use scarb::compiler::helpers::collect_main_crate_ids;
use scarb::compiler::{CairoCompilationUnit, CompilationUnit, CompilationUnitAttributes};
use scarb::core::{Config, TargetKind};
use scarb::core::{Config, Package, PackageId, TargetKind};
use scarb::ops::{self, CompileOpts};
use scarb_ui::args::FeaturesSpec;
use scarb_ui::args::{FeaturesSpec, PackagesFilter};
use tracing::trace;

pub(crate) const LOG_TARGET: &str = "sozo::cli::commands::test";
Expand Down Expand Up @@ -62,6 +62,9 @@
/// Specify the features to activate.
#[command(flatten)]
features: FeaturesSpec,
/// Specify packages to test.
#[command(flatten)]
pub packages: Option<PackagesFilter>,
}

impl TestArgs {
Expand All @@ -71,6 +74,14 @@
std::process::exit(1);
});

let packages: Vec<Package> = if let Some(filter) = self.packages {
filter.match_many(&ws)?.into_iter().collect()

Check warning on line 78 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L77-L78

Added lines #L77 - L78 were not covered by tests
} else {
ws.members().collect()

Check warning on line 80 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L80

Added line #L80 was not covered by tests
};

let package_ids = packages.iter().map(|p| p.id).collect::<Vec<PackageId>>();

Check warning on line 83 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L83

Added line #L83 was not covered by tests

let resolve = ops::resolve_workspace(&ws)?;

let opts = CompileOpts {
Expand All @@ -87,6 +98,7 @@
opts.include_target_kinds.is_empty()
|| opts.include_target_kinds.contains(&cu.main_component().target_kind())
})
.filter(|cu| package_ids.contains(&cu.main_package_id()))

Check warning on line 101 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L101

Added line #L101 was not covered by tests
.collect::<Vec<_>>();

for unit in compilation_units {
Expand All @@ -96,6 +108,11 @@
continue;
};

config.ui().print(format!("testing {}", unit.name()));

Check warning on line 111 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L111

Added line #L111 was not covered by tests

// Injecting the cfg_set for the unit makes compiler panics.
// We rely then on the default namespace for testing...?

let props: Props = unit.main_component().target_props()?;
let db = build_root_database(&unit)?;

Expand Down Expand Up @@ -155,10 +172,10 @@
let crate_roots = unit
.components
.iter()
.filter(|model| !model.package.id.is_core())
.filter(|c| !c.package.id.is_core())

Check warning on line 175 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L175

Added line #L175 was not covered by tests
// NOTE: We're taking the first target of each compilation unit, which should always be the
// main package source root due to the order maintained by scarb.
.map(|model| (model.cairo_package_name(), model.targets[0].source_root().into()))
.map(|c| (c.cairo_package_name(), c.targets[0].source_root().into()))

Check warning on line 178 in bin/sozo/src/commands/test.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/test.rs#L178

Added line #L178 was not covered by tests
.collect();

let corelib =
Expand Down
2 changes: 1 addition & 1 deletion crates/dojo-core/Scarb.lock
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ dependencies = [
[[package]]
name = "dojo_plugin"
version = "0.7.3"
source = "git+https://github.com/dojoengine/dojo?rev=d90b52b#d90b52b89749ac8af82f352dc08aa0b1378cfae6"
source = "git+https://github.com/dojoengine/dojo?rev=71b1f1a4#71b1f1a467534cbeeb901356f41e612ed4187bd1"
4 changes: 3 additions & 1 deletion crates/dojo-core/Scarb.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ version = "0.7.3"
[dependencies]
# Rev points to support for Cairo 2.7.0-rc.3 without any tag yet. Should be
# updated once a release is cut with `2.7.0-rc.3` support in it.
dojo_plugin = { git = "https://github.com/dojoengine/dojo", rev = "d90b52b" }
dojo_plugin = { git = "https://github.com/dojoengine/dojo", rev = "71b1f1a4" }
starknet = "=2.7.0-rc.3"

[lib]

# Dojo core is tested with sozo, hence we need a namespace for the test
# command to work.
[tool.dojo.world]
Expand Down
1 change: 1 addition & 0 deletions crates/dojo-lang/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ once_cell.workspace = true
regex.workspace = true
salsa.workspace = true
scarb.workspace = true
scarb-ui.workspace = true
semver.workspace = true
serde.workspace = true
serde_json.workspace = true
Expand Down
37 changes: 33 additions & 4 deletions crates/dojo-lang/src/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,15 @@ impl Compiler for DojoCompiler {
let props: Props = unit.main_component().target_props()?;
let target_dir = unit.target_dir(ws);

// TODO: if we want to output the manifests at the package level,
// we must iterate on the ws members, to find the location of the
// sole package with the `dojo` target.
// In this case, we can use this path to output the manifests.

Comment on lines +101 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohayo, sensei! Acknowledge the TODO comment.

The TODO comment indicates a potential future enhancement to output manifests at the package level.

Would you like me to create a GitHub issue to track this task, or provide assistance in implementing this feature?

let compiler_config = build_compiler_config(&unit, ws);

trace!(target: LOG_TARGET, unit = %unit.name(), ?props, "Compiling unit dojo compiler.");

let mut main_crate_ids = collect_main_crate_ids(&unit, db);
let core_crate_ids: Vec<CrateId> = collect_core_crate_ids(db);
main_crate_ids.extend(core_crate_ids);
Expand Down Expand Up @@ -347,10 +354,21 @@ fn update_files(
fs::create_dir_all(contracts_dir.path_unchecked())?;
}

// Ensure `contracts` dir exist event if no contracts are compiled
// to avoid errors when loading manifests.
let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR);
let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR);
if !base_contracts_dir.exists() {
std::fs::create_dir_all(&base_contracts_dir)?;
}
if !base_contracts_abis_dir.exists() {
std::fs::create_dir_all(&base_contracts_abis_dir)?;
}

Comment on lines +357 to +367
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohayo, sensei! Nice addition of directory creation logic.

Ensuring the contracts directory exists even if no contracts are compiled prevents potential errors when loading manifests. Consider using fs::create_dir_all once for both directories.

-  if !base_contracts_dir.exists() {
-      std::fs::create_dir_all(&base_contracts_dir)?;
-  }
-  if !base_contracts_abis_dir.exists() {
-      std::fs::create_dir_all(&base_contracts_abis_dir)?;
-  }
+  [base_contracts_dir, base_contracts_abis_dir].iter().try_for_each(|dir| {
+      if !dir.exists() {
+          std::fs::create_dir_all(dir)
+      } else {
+          Ok(())
+      }
+  })?;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Ensure `contracts` dir exist event if no contracts are compiled
// to avoid errors when loading manifests.
let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR);
let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR);
if !base_contracts_dir.exists() {
std::fs::create_dir_all(&base_contracts_dir)?;
}
if !base_contracts_abis_dir.exists() {
std::fs::create_dir_all(&base_contracts_abis_dir)?;
}
// Ensure `contracts` dir exist event if no contracts are compiled
// to avoid errors when loading manifests.
let base_contracts_dir = base_manifests_dir.join(CONTRACTS_DIR);
let base_contracts_abis_dir = base_abis_dir.join(CONTRACTS_DIR);
[base_contracts_dir, base_contracts_abis_dir].iter().try_for_each(|dir| {
if !dir.exists() {
std::fs::create_dir_all(dir)
} else {
Ok(())
}
})?;

for (_, (manifest, class, module_id)) in contracts.iter_mut() {
write_manifest_and_abi(
&base_manifests_dir.join(CONTRACTS_DIR),
&base_abis_dir.join(CONTRACTS_DIR),
&base_contracts_dir,
&base_contracts_abis_dir,
&manifest_dir,
manifest,
&class.abi,
Expand All @@ -373,10 +391,21 @@ fn update_files(
fs::create_dir_all(models_dir.path_unchecked())?;
}

// Ensure `models` dir exist event if no models are compiled
// to avoid errors when loading manifests.
let base_models_dir = base_manifests_dir.join(MODELS_DIR);
let base_models_abis_dir = base_abis_dir.join(MODELS_DIR);
if !base_models_dir.exists() {
std::fs::create_dir_all(&base_models_dir)?;
}
if !base_models_abis_dir.exists() {
std::fs::create_dir_all(&base_models_abis_dir)?;
}

Comment on lines +394 to +404
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohayo, sensei! Nice addition of directory creation logic.

Ensuring the models directory exists even if no models are compiled prevents potential errors when loading manifests. Consider using fs::create_dir_all once for both directories.

-  if !base_models_dir.exists() {
-      std::fs::create_dir_all(&base_models_dir)?;
-  }
-  if !base_models_abis_dir.exists() {
-      std::fs::create_dir_all(&base_models_abis_dir)?;
-  }
+  [base_models_dir, base_models_abis_dir].iter().try_for_each(|dir| {
+      if !dir.exists() {
+          std::fs::create_dir_all(dir)
+      } else {
+          Ok(())
+      }
+  })?;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Ensure `models` dir exist event if no models are compiled
// to avoid errors when loading manifests.
let base_models_dir = base_manifests_dir.join(MODELS_DIR);
let base_models_abis_dir = base_abis_dir.join(MODELS_DIR);
if !base_models_dir.exists() {
std::fs::create_dir_all(&base_models_dir)?;
}
if !base_models_abis_dir.exists() {
std::fs::create_dir_all(&base_models_abis_dir)?;
}
// Ensure `models` dir exist event if no models are compiled
// to avoid errors when loading manifests.
let base_models_dir = base_manifests_dir.join(MODELS_DIR);
let base_models_abis_dir = base_abis_dir.join(MODELS_DIR);
[base_models_dir, base_models_abis_dir].iter().try_for_each(|dir| {
if !dir.exists() {
std::fs::create_dir_all(dir)
} else {
Ok(())
}
})?;

for (_, (manifest, class, module_id)) in models.iter_mut() {
write_manifest_and_abi(
&base_manifests_dir.join(MODELS_DIR),
&base_abis_dir.join(MODELS_DIR),
&base_models_dir,
&base_models_abis_dir,
&manifest_dir,
manifest,
&class.abi,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ dependencies = [
[[package]]
name = "dojo_plugin"
version = "0.7.3"
source = "git+https://github.com/dojoengine/dojo?rev=d90b52b#d90b52b89749ac8af82f352dc08aa0b1378cfae6"
source = "git+https://github.com/dojoengine/dojo?rev=71b1f1a4#71b1f1a467534cbeeb901356f41e612ed4187bd1"
49 changes: 36 additions & 13 deletions crates/dojo-lang/src/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
PluginDiagnostic, PluginGeneratedFile, PluginResult,
};
use cairo_lang_diagnostics::Severity;
use cairo_lang_filesystem::cfg::Cfg;
use cairo_lang_semantic::plugin::PluginSuite;
use cairo_lang_starknet::plugin::aux_data::StarkNetEventAuxData;
use cairo_lang_syntax::attribute::structured::{AttributeArgVariant, AttributeStructurize};
Expand Down Expand Up @@ -37,7 +38,7 @@
use crate::introspect::{handle_introspect_enum, handle_introspect_struct};
use crate::model::handle_model_struct;
use crate::print::{handle_print_enum, handle_print_struct};
use crate::utils::get_namespace_config;
use crate::utils;

pub const DOJO_CONTRACT_ATTR: &str = "dojo::contract";
pub const DOJO_INTERFACE_ATTR: &str = "dojo::interface";
Expand Down Expand Up @@ -339,27 +340,49 @@
}

impl MacroPlugin for BuiltinDojoPlugin {
// This function is called for every item in whole db. Hence,
// the sooner we can return, the better.
// As an example, compiling spawn-and-move project, it's almost 14K calls to this
// function.
fn generate_code(
&self,
db: &dyn SyntaxGroup,
item_ast: ast::ModuleItem,
metadata: &MacroPluginMetadata<'_>,
) -> PluginResult {
let namespace_config = match get_namespace_config(db) {
Ok(config) => config,
Err(e) => {
return PluginResult {
code: Option::None,
diagnostics: vec![PluginDiagnostic {
stable_ptr: item_ast.stable_ptr().0,
message: format!("{e}"),
severity: Severity::Error,
}],
remove_original_item: false,
};
let namespace_config: NamespaceConfig = if db.cfg_set().contains(&Cfg::kv("target", "test"))
{
// In test mode, we can't inject namespace information into the `CfgSet`
// as the compiler panics. We then extract the info from the main component
// of the compilation unit.
match utils::get_namespace_config(db) {
Ok(config) => config,
Err(e) => {
return PluginResult {
code: Option::None,
diagnostics: vec![PluginDiagnostic {
stable_ptr: item_ast.stable_ptr().0,
message: format!("{e}"),
severity: Severity::Error,
}],
remove_original_item: false,
};

Check warning on line 369 in crates/dojo-lang/src/plugin.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo-lang/src/plugin.rs#L358-L369

Added lines #L358 - L369 were not covered by tests
}
}
} else {
// Metadata gives information from the crates from where `item_ast` was parsed.
// During the compilation phase, we inject namespace information into the `CfgSet`
// so that it can be used here.
metadata.cfg_set.into()
};

// Avoid the whole plugin checks if there is no default namespace.
// The compiler already checked for invalid package configuration,
// so empty default namespace can be skipped.
// if namespace_config.default.is_empty() {
// return PluginResult::default();
// }

match item_ast {
ast::ModuleItem::Module(module_ast) => {
self.handle_mod(db, module_ast, &namespace_config, metadata)
Expand Down
Loading
Loading