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 sozo dev command #2664

Merged
merged 2 commits into from
Nov 8, 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
79 changes: 79 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions bin/sozo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ dojo-utils.workspace = true
dojo-world.workspace = true
itertools.workspace = true
katana-rpc-api.workspace = true
notify = "7.0.0"
tabled = { version = "0.16.0", features = [ "ansi" ] }
scarb.workspace = true
scarb-ui.workspace = true
Expand Down
2 changes: 1 addition & 1 deletion bin/sozo/src/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use tracing::debug;

use crate::commands::check_package_dojo_version;

#[derive(Debug, Args)]
#[derive(Debug, Clone, Args)]
pub struct BuildArgs {
#[arg(long)]
#[arg(help = "Generate Typescript bindings.")]
Expand Down
145 changes: 104 additions & 41 deletions bin/sozo/src/commands/dev.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
use std::sync::mpsc::channel;
use std::time::Duration;
use std::thread;
use std::time::{Duration, Instant};

use anyhow::Result;
use clap::Args;
use notify::event::Event;
use notify::{EventKind, PollWatcher, RecursiveMode, Watcher};
use scarb::core::Config;
use scarb_ui::args::{FeaturesSpec, PackagesFilter};
use tracing::{error, info, trace};

use super::build::BuildArgs;
use super::migrate::MigrateArgs;
use super::options::account::AccountOptions;
use super::options::starknet::StarknetOptions;
use super::options::transaction::TransactionOptions;
use super::options::world::WorldOptions;

#[derive(Debug, Args)]
Expand All @@ -24,80 +27,140 @@

#[command(flatten)]
pub account: AccountOptions,

#[command(flatten)]
pub transaction: TransactionOptions,

#[arg(long)]
#[arg(help = "Generate Typescript bindings.")]
pub typescript: bool,

Check warning on line 36 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L36

Added line #L36 was not covered by tests

#[arg(long)]
#[arg(help = "Generate Typescript bindings.")]
pub typescript_v2: bool,

Check warning on line 40 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L40

Added line #L40 was not covered by tests

#[arg(long)]
#[arg(help = "Generate Unity bindings.")]
pub unity: bool,

Check warning on line 44 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L44

Added line #L44 was not covered by tests

#[arg(long)]
#[arg(help = "Output directory.", default_value = "bindings")]
pub bindings_output: String,

Check warning on line 48 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L48

Added line #L48 was not covered by tests

/// Specify the features to activate.
#[command(flatten)]
pub features: FeaturesSpec,

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

impl DevArgs {
/// Watches the `src` directory that is found at the same level of the `Scarb.toml` manifest
/// of the project into the provided [`Config`].
///
/// When a change is detected, it rebuilds the project and applies the migrations.
pub fn run(self, config: &Config) -> Result<()> {
let (tx, rx) = channel();
let (file_tx, file_rx) = channel();
let (rebuild_tx, rebuild_rx) = channel();

Check warning on line 62 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L61-L62

Added lines #L61 - L62 were not covered by tests

let watcher_config = notify::Config::default().with_poll_interval(Duration::from_secs(1));
let watcher_config =
notify::Config::default().with_poll_interval(Duration::from_millis(500));

Check warning on line 65 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L64-L65

Added lines #L64 - L65 were not covered by tests

let mut watcher = PollWatcher::new(tx, watcher_config)?;
let mut watcher = PollWatcher::new(file_tx, watcher_config)?;

Check warning on line 67 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L67

Added line #L67 was not covered by tests

let watched_directory = config.manifest_path().parent().unwrap().join("src");

watcher.watch(watched_directory.as_std_path(), RecursiveMode::Recursive).unwrap();

// Always build the project before starting the dev loop to make sure that the project is
// in a valid state. Devs may not use `build` anymore when using `dev`.
BuildArgs::default().run(config)?;
// Initial build and migrate
let build_args = BuildArgs {
typescript: self.typescript,
typescript_v2: self.typescript_v2,
unity: self.unity,
bindings_output: self.bindings_output,
features: self.features,
packages: self.packages,
..Default::default()
};
build_args.clone().run(config)?;

Check warning on line 82 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L72-L82

Added lines #L72 - L82 were not covered by tests
info!("Initial build completed.");

let _ =
MigrateArgs::new_apply(self.world.clone(), self.starknet.clone(), self.account.clone())
.run(config);
let migrate_args = MigrateArgs {
world: self.world,
starknet: self.starknet,
account: self.account,
transaction: self.transaction,
};

let _ = migrate_args.clone().run(config);

Check warning on line 92 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L85-L92

Added lines #L85 - L92 were not covered by tests
Comment on lines +85 to +92
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo, sensei! Ensure errors in build and migrate steps are handled.

The run methods for build_args and migrate_args discard errors using let _ =. Handling these errors will provide feedback if these processes fail, improving robustness.

Apply this diff to properly handle errors:

- let _ = build_args.clone().run(config);
- let _ = migrate_args.clone().run(config);
+ if let Err(e) = build_args.clone().run(config) {
+     error!(?e, "Build failed.");
+ }
+ if let Err(e) = migrate_args.clone().run(config) {
+     error!(?e, "Migration failed.");
+ }

Committable suggestion skipped: line range outside the PR's diff.


info!(
directory = watched_directory.to_string(),
"Initial migration completed. Waiting for changes."
);

let mut e_handler = EventHandler;

loop {
let is_rebuild_needed = match rx.recv() {
Ok(maybe_event) => match maybe_event {
Ok(event) => e_handler.process_event(event),
let e_handler = EventHandler;
let rebuild_tx_clone = rebuild_tx.clone();

// Independent thread to handle file events and trigger rebuilds.
config.tokio_handle().spawn(async move {
loop {
match file_rx.recv() {
Ok(maybe_event) => match maybe_event {
Ok(event) => {
trace!(?event, "Event received.");

Check warning on line 108 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L99-L108

Added lines #L99 - L108 were not covered by tests

if e_handler.process_event(event) && rebuild_tx_clone.send(()).is_err()

Check warning on line 110 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L110

Added line #L110 was not covered by tests
{
break;
}

Check warning on line 113 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L112-L113

Added lines #L112 - L113 were not covered by tests
}
Err(error) => {
error!(?error, "Processing event.");
break;

Check warning on line 117 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L115-L117

Added lines #L115 - L117 were not covered by tests
}
},
Err(error) => {
error!(?error, "Processing event.");
error!(?error, "Receiving event.");

Check warning on line 121 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L121

Added line #L121 was not covered by tests
break;
}
},
Err(error) => {
error!(?error, "Receiving event.");
break;
}
};

if is_rebuild_needed {
// Ignore the fails of those commands as the `run` function
// already logs the error.
let _ = BuildArgs::default().run(config);

let _ = MigrateArgs::new_apply(
self.world.clone(),
self.starknet.clone(),
self.account.clone(),
)
.run(config);
}
});
Comment on lines +99 to +126
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Thread and channel cleanup needed in dev command

Ohayo sensei! After analyzing the code, there are indeed concerns about thread and channel management:

  • The tokio task spawned for event handling has no explicit cleanup mechanism
  • The PollWatcher instance is not explicitly dropped or cleaned up
  • The channels (file_tx/rx and rebuild_tx/rx) are not properly closed

To fix this, consider:

  • Implementing a proper shutdown mechanism using a shutdown channel or flag
  • Adding proper cleanup in a Drop implementation or explicit cleanup method
  • Using tokio::select! to handle graceful shutdown of the spawned task
🔗 Analysis chain

Verify proper closure of threads and channels.

The spawned thread may outlive the run function scope, potentially causing issues during shutdown. Ensure threads and channels are properly managed to avoid panics or resource leaks.

Run the following script to check for proper thread and channel management:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all threads are joined and channels are closed appropriately.

# Expected: No threads left running unexpectedly; channels are properly closed.

# Check for thread joins
rg 'spawn' -A 5 | rg -q 'join\(\)' && echo "All threads are joined." || echo "Threads may not be properly joined."

# Check for channel closure
rg 'channel' -A 5 | rg -q 'drop\(' && echo "Channels are properly closed." || echo "Channels may not be properly closed."

Length of output: 233


Script:

#!/bin/bash
# Let's check the full context of the thread spawning and channel usage in dev.rs
rg -l "spawn|channel" bin/sozo/src/commands/dev.rs

# If found, let's see the full implementation
cat bin/sozo/src/commands/dev.rs

# Let's also check for any cleanup or drop implementations
ast-grep --pattern 'impl Drop for $_ { $$ }'

# Check for any shutdown or cleanup related functions
rg -A 5 "shutdown|cleanup|drop|close" bin/sozo/src/commands/dev.rs

Length of output: 6439


// Main thread handles the rebuilds.
let mut last_event_time = None;
let debounce_period = Duration::from_millis(1500);

Check warning on line 130 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L126-L130

Added lines #L126 - L130 were not covered by tests

loop {
match rebuild_rx.try_recv() {
Ok(()) => {
last_event_time = Some(Instant::now());
}

Check warning on line 136 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L133-L136

Added lines #L133 - L136 were not covered by tests
Err(std::sync::mpsc::TryRecvError::Empty) => {
if let Some(last_time) = last_event_time {
if last_time.elapsed() >= debounce_period {
let _ = build_args.clone().run(config);
let _ = migrate_args.clone().run(config);
last_event_time = None;
} else {
trace!("Change detected, waiting for debounce period.");

Check warning on line 144 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L138-L144

Added lines #L138 - L144 were not covered by tests
}
}
thread::sleep(Duration::from_millis(300));

Check warning on line 147 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L146-L147

Added lines #L146 - L147 were not covered by tests
}
Err(std::sync::mpsc::TryRecvError::Disconnected) => break,

Check warning on line 149 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L149

Added line #L149 was not covered by tests
}
}

Ok(())
}
}

#[derive(Debug, Default)]
#[derive(Debug)]
struct EventHandler;

impl EventHandler {
/// Processes a debounced event and return true if a rebuild is needed.
/// Only considers Cairo file and the Scarb.toml manifest.
fn process_event(&mut self, event: Event) -> bool {
fn process_event(&self, event: Event) -> bool {

Check warning on line 163 in bin/sozo/src/commands/dev.rs

View check run for this annotation

Codecov / codecov/patch

bin/sozo/src/commands/dev.rs#L163

Added line #L163 was not covered by tests
trace!(?event, "Processing event.");

let paths = match event.kind {
Expand Down
10 changes: 5 additions & 5 deletions bin/sozo/src/commands/migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@ use super::options::transaction::TransactionOptions;
use super::options::world::WorldOptions;
use crate::utils;

#[derive(Debug, Args)]
#[derive(Debug, Clone, Args)]
pub struct MigrateArgs {
#[command(flatten)]
transaction: TransactionOptions,
pub transaction: TransactionOptions,

#[command(flatten)]
world: WorldOptions,
pub world: WorldOptions,

#[command(flatten)]
starknet: StarknetOptions,
pub starknet: StarknetOptions,

#[command(flatten)]
account: AccountOptions,
pub account: AccountOptions,
}

impl MigrateArgs {
Expand Down
Loading
Loading