From 4d734ebd8988fe6f8d60f5d15b3523d8f394cd90 Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Sun, 24 Nov 2024 21:03:03 +0200 Subject: [PATCH 1/6] chore(infra): canonicalize path resolution commit-id:c2547652 --- Cargo.lock | 3 --- crates/infra_utils/Cargo.toml | 3 --- crates/infra_utils/src/path.rs | 27 ++++++--------------------- crates/infra_utils/src/path_test.rs | 9 ++------- crates/papyrus_config/src/lib.rs | 3 --- 5 files changed, 8 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c312f12818..5e7278717e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5163,9 +5163,6 @@ checksum = "b248f5224d1d606005e02c97f5aa4e88eeb230488bcc03bc9ca4d7991399f2b5" [[package]] name = "infra_utils" version = "0.0.0" -dependencies = [ - "thiserror", -] [[package]] name = "inout" diff --git a/crates/infra_utils/Cargo.toml b/crates/infra_utils/Cargo.toml index ed18931577..69c33687b6 100644 --- a/crates/infra_utils/Cargo.toml +++ b/crates/infra_utils/Cargo.toml @@ -8,6 +8,3 @@ description = "Infrastructure utility." [lints] workspace = true - -[dependencies] -thiserror.workspace = true diff --git a/crates/infra_utils/src/path.rs b/crates/infra_utils/src/path.rs index cfa2444a24..f3ceeb0802 100644 --- a/crates/infra_utils/src/path.rs +++ b/crates/infra_utils/src/path.rs @@ -1,28 +1,16 @@ -use std::env; use std::path::{Path, PathBuf}; use std::sync::LazyLock; - -use thiserror::Error; +use std::{env, fs}; #[cfg(test)] #[path = "path_test.rs"] mod path_test; -#[derive(Debug, Error)] -pub enum PathResolutionError { - // TODO(Arni): Handle manifest dir not exist here? - #[error("No file exists at '{path}'")] - PathDoesNotExist { path: PathBuf }, - /// This error is raised when file existence can be neither confirmed nor denied. See - /// [`std::path::Path::try_exists`] for more information. - #[error(transparent)] - IoError(#[from] std::io::Error), -} - // TODO(tsabary): wrap path-related env::* invocations in the repo as utility functions static PATH_TO_CARGO_MANIFEST_DIR: LazyLock> = LazyLock::new(|| env::var("CARGO_MANIFEST_DIR").ok().map(|dir| Path::new(&dir).into())); +// TODO(Tsabary): should not be public. Use a getter instead. pub fn cargo_manifest_dir() -> Option { PATH_TO_CARGO_MANIFEST_DIR.clone() } @@ -34,16 +22,13 @@ pub fn cargo_manifest_dir() -> Option { /// * `relative_path` - A string slice representing the relative path from the project root. /// /// # Returns -/// * An absolute `PathBuf` representing the resolved path starting from the project root. -pub fn resolve_project_relative_path(relative_path: &str) -> Result { +/// * A `PathBuf` representing the resolved path starting from the project root. +pub fn resolve_project_relative_path(relative_path: &str) -> Result { let base_dir = path_of_project_root(); - let path = base_dir.join(relative_path); - if !path.try_exists()? { - return Err(PathResolutionError::PathDoesNotExist { path }); - } + let absolute_path = fs::canonicalize(path)?; - Ok(path) + Ok(absolute_path) } fn path_of_project_root() -> PathBuf { diff --git a/crates/infra_utils/src/path_test.rs b/crates/infra_utils/src/path_test.rs index 88d7f5ff23..8133007262 100644 --- a/crates/infra_utils/src/path_test.rs +++ b/crates/infra_utils/src/path_test.rs @@ -1,6 +1,5 @@ -use crate::path::{path_of_project_root, resolve_project_relative_path, PathResolutionError}; +use crate::path::{path_of_project_root, resolve_project_relative_path}; -// TODO: Add a test for PathResolutionError::IoError. #[test] fn resolve_project_relative_path_on_non_existent_path() { let relative_path = "does_not_exist.txt"; @@ -8,11 +7,7 @@ fn resolve_project_relative_path_on_non_existent_path() { assert!(!expected_path.exists()); let result = resolve_project_relative_path(relative_path); - if let Err(PathResolutionError::PathDoesNotExist { path }) = result { - assert_eq!(path, expected_path); - } else { - panic!("Expected PathDoesNotExist error, got {:?}", result); - } + assert!(result.is_err(), "Expected an non-existent path error, got {:?}", result); } #[test] diff --git a/crates/papyrus_config/src/lib.rs b/crates/papyrus_config/src/lib.rs index 77e4106f16..7a383cef54 100644 --- a/crates/papyrus_config/src/lib.rs +++ b/crates/papyrus_config/src/lib.rs @@ -49,7 +49,6 @@ use clap::parser::MatchesError; use dumping::REQUIRED_PARAM_DESCRIPTION_PREFIX; -use infra_utils::path::PathResolutionError; use serde::{Deserialize, Serialize}; use serde_json::Value; use validator::ValidationError; @@ -180,8 +179,6 @@ pub enum ConfigError { #[error(transparent)] CommandMatches(#[from] MatchesError), #[error(transparent)] - GetPathError(#[from] PathResolutionError), - #[error(transparent)] IOError(#[from] std::io::Error), // TODO(Eitan): Improve error message #[error("Insert a new param is not allowed: {param_path}.")] From 21525e856ce580f35fa96d724b304e883f8cd113 Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Sun, 24 Nov 2024 21:03:18 +0200 Subject: [PATCH 2/6] chore(infra): add project dir path fn commit-id:6207c777 --- crates/infra_utils/src/path.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/infra_utils/src/path.rs b/crates/infra_utils/src/path.rs index f3ceeb0802..d8080216fe 100644 --- a/crates/infra_utils/src/path.rs +++ b/crates/infra_utils/src/path.rs @@ -31,6 +31,14 @@ pub fn resolve_project_relative_path(relative_path: &str) -> Result Result { + resolve_project_relative_path(".") +} + fn path_of_project_root() -> PathBuf { cargo_manifest_dir() // Attempt to get the `CARGO_MANIFEST_DIR` environment variable and convert it to `PathBuf`. From f92053a26177d31a4954cae763a91d4560686885 Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Mon, 25 Nov 2024 11:14:34 +0200 Subject: [PATCH 3/6] chore: add shell command builder commit-id:96859c27 --- Cargo.lock | 8 +++++++ crates/infra_utils/Cargo.toml | 6 ++++++ crates/infra_utils/src/command.rs | 29 ++++++++++++++++++++++++++ crates/infra_utils/src/command_test.rs | 15 +++++++++++++ crates/infra_utils/src/lib.rs | 1 + 5 files changed, 59 insertions(+) create mode 100644 crates/infra_utils/src/command.rs create mode 100644 crates/infra_utils/src/command_test.rs diff --git a/Cargo.lock b/Cargo.lock index 5e7278717e..5bad2ad0fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5163,6 +5163,14 @@ checksum = "b248f5224d1d606005e02c97f5aa4e88eeb230488bcc03bc9ca4d7991399f2b5" [[package]] name = "infra_utils" version = "0.0.0" +<<<<<<< HEAD +======= +dependencies = [ + "rstest", + "thiserror", + "tokio", +] +>>>>>>> 32f1a4b3b (chore: add shell command builder) [[package]] name = "inout" diff --git a/crates/infra_utils/Cargo.toml b/crates/infra_utils/Cargo.toml index 69c33687b6..6b42af283d 100644 --- a/crates/infra_utils/Cargo.toml +++ b/crates/infra_utils/Cargo.toml @@ -8,3 +8,9 @@ description = "Infrastructure utility." [lints] workspace = true + +[dependencies] +tokio = { workspace = true, features = ["process"] } + +[dev-dependencies] +rstest.workspace = true diff --git a/crates/infra_utils/src/command.rs b/crates/infra_utils/src/command.rs new file mode 100644 index 0000000000..3d4e955030 --- /dev/null +++ b/crates/infra_utils/src/command.rs @@ -0,0 +1,29 @@ +use std::env; + +use tokio::process::Command; + +use crate::path::project_path; + +#[cfg(test)] +#[path = "command_test.rs"] +mod command_test; + +/// Returns a shell command originating from the project root, with cargo environment variables +/// filtered out. +/// +/// # Arguments +/// * `command_name` - The shell command name. +/// +/// # Returns +/// * A [`std::process::Command`] object with the current directory set to the project root, and +/// cleared out cargo related environment variables. +pub fn create_shell_command(command_name: &str) -> Command { + let project_path = project_path().expect("Failed to get project path"); + let mut command = Command::new(command_name); + command.current_dir(&project_path); + // Filter out all CARGO_ environment variables. + env::vars().filter(|(key, _)| key.starts_with("CARGO_")).for_each(|(key, _)| { + command.env_remove(key); + }); + command +} diff --git a/crates/infra_utils/src/command_test.rs b/crates/infra_utils/src/command_test.rs new file mode 100644 index 0000000000..2d18e1cf0d --- /dev/null +++ b/crates/infra_utils/src/command_test.rs @@ -0,0 +1,15 @@ +use rstest::rstest; + +use crate::command::create_shell_command; + +#[rstest] +#[tokio::test] +async fn create_shell_command_example() { + let mut ls_command = create_shell_command("ls"); + let output = ls_command.output().await.expect("Failed to execute command"); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!(output.status.success()); + // Project root should contain the `crates` directory. + assert!(stdout.contains("crates")); +} diff --git a/crates/infra_utils/src/lib.rs b/crates/infra_utils/src/lib.rs index 4da9789237..f744151bf9 100644 --- a/crates/infra_utils/src/lib.rs +++ b/crates/infra_utils/src/lib.rs @@ -1 +1,2 @@ +pub mod command; pub mod path; From 06ab57ace27b19b138f7025558856a7ead1ad76f Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Mon, 25 Nov 2024 11:46:10 +0200 Subject: [PATCH 4/6] chore(sequencer_node): compile the node, not the entire project commit-id:7f25ebe8 --- .../src/test_utils/compilation.rs | 25 ++++++++++--------- .../src/test_utils/compilation_test.rs | 9 ++++--- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/starknet_sequencer_node/src/test_utils/compilation.rs b/crates/starknet_sequencer_node/src/test_utils/compilation.rs index 645fe536ae..c89d62d397 100644 --- a/crates/starknet_sequencer_node/src/test_utils/compilation.rs +++ b/crates/starknet_sequencer_node/src/test_utils/compilation.rs @@ -1,6 +1,7 @@ -use std::process::{Command, ExitStatus, Stdio}; -use std::{env, io}; +use std::io; +use std::process::{ExitStatus, Stdio}; +use infra_utils::command::create_shell_command; use tracing::info; #[cfg(test)] @@ -16,26 +17,26 @@ pub enum NodeCompilationError { } /// Compiles the node using `cargo build` for testing purposes. -fn compile_node() -> io::Result { - info!("Compiling the project"); - // Get the current working directory for the project - let project_path = env::current_dir().expect("Failed to get current directory"); +async fn compile_node() -> io::Result { + info!("Compiling the starknet_sequencer_node binary"); // Run `cargo build` to compile the project - let compilation_result = Command::new("cargo") + let compilation_result = create_shell_command("cargo") .arg("build") - .current_dir(&project_path) + .arg("--bin") + .arg("starknet_sequencer_node") .arg("--quiet") .stderr(Stdio::inherit()) .stdout(Stdio::inherit()) - .status(); + .status() + .await?; info!("Compilation result: {:?}", compilation_result); - compilation_result + Ok(compilation_result) } -pub fn compile_node_result() -> Result<(), NodeCompilationError> { - match compile_node() { +pub async fn compile_node_result() -> Result<(), NodeCompilationError> { + match compile_node().await { Ok(status) if status.success() => Ok(()), Ok(status) => Err(NodeCompilationError::Status(status)), Err(e) => Err(NodeCompilationError::IO(e)), diff --git a/crates/starknet_sequencer_node/src/test_utils/compilation_test.rs b/crates/starknet_sequencer_node/src/test_utils/compilation_test.rs index 3dd888fbba..3e2f6b6200 100644 --- a/crates/starknet_sequencer_node/src/test_utils/compilation_test.rs +++ b/crates/starknet_sequencer_node/src/test_utils/compilation_test.rs @@ -1,6 +1,9 @@ +use rstest::rstest; + use crate::test_utils::compilation::compile_node_result; -#[test] -fn test_compile_node() { - assert!(compile_node_result().is_ok(), "Compilation failed"); +#[rstest] +#[tokio::test] +async fn test_compile_node() { + assert!(compile_node_result().await.is_ok(), "Compilation failed"); } From 3a5080501e402fa780930ad9aa228aab77b30ab2 Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Mon, 25 Nov 2024 12:03:09 +0200 Subject: [PATCH 5/6] chore(sequencer_node): replace run node shell command commit-id:3dd1cf11 --- Cargo.lock | 5 +---- crates/starknet_integration_tests/Cargo.toml | 1 + .../tests/end_to_end_integration_test.rs | 10 +++------- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5bad2ad0fa..426db2779a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5163,14 +5163,10 @@ checksum = "b248f5224d1d606005e02c97f5aa4e88eeb230488bcc03bc9ca4d7991399f2b5" [[package]] name = "infra_utils" version = "0.0.0" -<<<<<<< HEAD -======= dependencies = [ "rstest", - "thiserror", "tokio", ] ->>>>>>> 32f1a4b3b (chore: add shell command builder) [[package]] name = "inout" @@ -10312,6 +10308,7 @@ dependencies = [ "cairo-lang-starknet-classes", "futures", "indexmap 2.6.0", + "infra_utils", "mempool_test_utils", "papyrus_common", "papyrus_consensus", diff --git a/crates/starknet_integration_tests/Cargo.toml b/crates/starknet_integration_tests/Cargo.toml index 2a77b40534..8b3b43ef64 100644 --- a/crates/starknet_integration_tests/Cargo.toml +++ b/crates/starknet_integration_tests/Cargo.toml @@ -46,6 +46,7 @@ tracing.workspace = true [dev-dependencies] futures.workspace = true +infra_utils.workspace = true pretty_assertions.workspace = true rstest.workspace = true starknet_sequencer_infra.workspace = true diff --git a/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs b/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs index 1602e1f87c..00ee5ea2a9 100644 --- a/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs +++ b/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs @@ -1,8 +1,8 @@ -use std::env; use std::path::PathBuf; use std::process::Stdio; use std::time::Duration; +use infra_utils::command::create_shell_command; use mempool_test_utils::starknet_api_test_utils::{AccountId, MultiAccountTransactionGenerator}; use papyrus_execution::execution_utils::get_nonce_at; use papyrus_storage::state::StateStorageReader; @@ -15,7 +15,7 @@ use starknet_integration_tests::integration_test_setup::IntegrationTestSetup; use starknet_integration_tests::utils::{create_integration_test_tx_generator, send_account_txs}; use starknet_sequencer_infra::trace_util::configure_tracing; use starknet_types_core::felt::Felt; -use tokio::process::{Child, Command}; +use tokio::process::Child; use tokio::task::{self, JoinHandle}; use tokio::time::interval; use tracing::{error, info}; @@ -27,18 +27,14 @@ fn tx_generator() -> MultiAccountTransactionGenerator { // TODO(Tsabary): Move to a suitable util location. async fn spawn_node_child_task(node_config_path: PathBuf) -> Child { - // Get the current working directory for the project - let project_path = env::current_dir().expect("Failed to get current directory").join("../.."); - // TODO(Tsabary): Capture output to a log file, and present it in case of a failure. // TODO(Tsabary): Change invocation from "cargo run" to separate compilation and invocation // (build, and then invoke the binary). - Command::new("cargo") + create_shell_command("cargo") .arg("run") .arg("--bin") .arg("starknet_sequencer_node") .arg("--quiet") - .current_dir(&project_path) .arg("--") .arg("--config_file") .arg(node_config_path.to_str().unwrap()) From c543c8a70076b79cb0258567267747bfa50868fe Mon Sep 17 00:00:00 2001 From: Itay Tsabary Date: Mon, 25 Nov 2024 12:37:03 +0200 Subject: [PATCH 6/6] chore(tests_integration): replace cargo run with build and direct invocation commit-id:b949599a --- .../tests/end_to_end_integration_test.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs b/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs index 00ee5ea2a9..992a8c8daf 100644 --- a/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs +++ b/crates/starknet_integration_tests/tests/end_to_end_integration_test.rs @@ -14,6 +14,7 @@ use starknet_api::state::StateNumber; use starknet_integration_tests::integration_test_setup::IntegrationTestSetup; use starknet_integration_tests::utils::{create_integration_test_tx_generator, send_account_txs}; use starknet_sequencer_infra::trace_util::configure_tracing; +use starknet_sequencer_node::test_utils::compilation::compile_node_result; use starknet_types_core::felt::Felt; use tokio::process::Child; use tokio::task::{self, JoinHandle}; @@ -28,14 +29,8 @@ fn tx_generator() -> MultiAccountTransactionGenerator { // TODO(Tsabary): Move to a suitable util location. async fn spawn_node_child_task(node_config_path: PathBuf) -> Child { // TODO(Tsabary): Capture output to a log file, and present it in case of a failure. - // TODO(Tsabary): Change invocation from "cargo run" to separate compilation and invocation - // (build, and then invoke the binary). - create_shell_command("cargo") - .arg("run") - .arg("--bin") - .arg("starknet_sequencer_node") - .arg("--quiet") - .arg("--") + compile_node_result().await.expect("Failed to compile the sequencer node."); + create_shell_command("target/debug/starknet_sequencer_node") .arg("--config_file") .arg(node_config_path.to_str().unwrap()) .stderr(Stdio::inherit())