From 3d03db247897dbdb88aeba78373ba13fea5bada1 Mon Sep 17 00:00:00 2001 From: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Date: Thu, 2 May 2024 11:23:29 +0100 Subject: [PATCH] feat(up parachain): improve build ux (#123) * build(deps): update cliclack * feat(up parachain): improve build ux * refactor: address warnings after dependency * refactor: improve callback type * docs: add doc comments * refactor: improve types to better convey intent --- Cargo.lock | 4 +- Cargo.toml | 2 +- crates/pop-cli/src/commands/call/contract.rs | 6 +- crates/pop-cli/src/commands/new/contract.rs | 2 +- crates/pop-cli/src/commands/new/pallet.rs | 2 +- crates/pop-cli/src/commands/new/parachain.rs | 2 +- crates/pop-cli/src/commands/up/contract.rs | 4 +- crates/pop-cli/src/commands/up/parachain.rs | 53 ++++++++--- crates/pop-parachains/src/lib.rs | 2 +- crates/pop-parachains/src/up.rs | 98 ++++++++++++++------ 10 files changed, 125 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 99c532769..a5f2d2506 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1301,9 +1301,9 @@ checksum = "98cc8fbded0c607b7ba9dd60cd98df59af97e84d24e49c8557331cfc26d301ce" [[package]] name = "cliclack" -version = "0.1.13" +version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "be29210ca32b96e4f67fe9a520d2eeacc078d94ff4027100dc6b7262fdfec5c4" +checksum = "4febf49beeedc40528e4956995631f1bbdb4d8804ef940b44351f393a996c739" dependencies = [ "console", "indicatif", diff --git a/Cargo.toml b/Cargo.toml index 5bc9af060..7f4a287db 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,7 @@ git2_credentials = "0.13.0" # pop-cli clap = { version = "4.4", features = ["derive"] } -cliclack = "0.1" +cliclack = "0.2" console = "0.15" strum = "0.26" strum_macros = "0.26" diff --git a/crates/pop-cli/src/commands/call/contract.rs b/crates/pop-cli/src/commands/call/contract.rs index a91387b5c..4fa275332 100644 --- a/crates/pop-cli/src/commands/call/contract.rs +++ b/crates/pop-cli/src/commands/call/contract.rs @@ -75,7 +75,7 @@ impl CallContractCommand { .await?; if !self.execute { - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Calling the contract..."); let call_dry_run_result = dry_run_call(&call_exec).await?; log::info(format!("Result: {}", call_dry_run_result))?; @@ -90,7 +90,7 @@ impl CallContractCommand { weight_limit = Weight::from_parts(self.gas_limit.unwrap(), self.proof_size.unwrap()); } else { - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Doing a dry run to estimate the gas..."); weight_limit = match dry_run_gas_estimate_call(&call_exec).await { Ok(w) => { @@ -104,7 +104,7 @@ impl CallContractCommand { }, }; } - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Calling the contract..."); let call_result = call_smart_contract(call_exec, weight_limit, &self.url) diff --git a/crates/pop-cli/src/commands/new/contract.rs b/crates/pop-cli/src/commands/new/contract.rs index 78dc3edf1..909eb666b 100644 --- a/crates/pop-cli/src/commands/new/contract.rs +++ b/crates/pop-cli/src/commands/new/contract.rs @@ -47,7 +47,7 @@ impl NewContractCommand { fs::remove_dir_all(contract_path.as_path())?; } fs::create_dir_all(contract_path.as_path())?; - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Generating contract..."); create_smart_contract(&self.name, contract_path.as_path())?; diff --git a/crates/pop-cli/src/commands/new/pallet.rs b/crates/pop-cli/src/commands/new/pallet.rs index 03c997ecc..9f5698239 100644 --- a/crates/pop-cli/src/commands/new/pallet.rs +++ b/crates/pop-cli/src/commands/new/pallet.rs @@ -45,7 +45,7 @@ impl NewPalletCommand { } fs::remove_dir_all(pallet_path)?; } - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Generating pallet..."); create_pallet_template( self.path.clone(), diff --git a/crates/pop-cli/src/commands/new/parachain.rs b/crates/pop-cli/src/commands/new/parachain.rs index dfd75b72e..9e7b8e914 100644 --- a/crates/pop-cli/src/commands/new/parachain.rs +++ b/crates/pop-cli/src/commands/new/parachain.rs @@ -156,7 +156,7 @@ fn generate_parachain_from_template( let destination_path = check_destination_path(name_template)?; - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Generating parachain..."); let tag = instantiate_template_dir(template, destination_path, tag_version, config)?; if let Err(err) = Git::git_init(destination_path, "initialized parachain") { diff --git a/crates/pop-cli/src/commands/up/contract.rs b/crates/pop-cli/src/commands/up/contract.rs index aa9b8e3ac..14bcbb7c4 100644 --- a/crates/pop-cli/src/commands/up/contract.rs +++ b/crates/pop-cli/src/commands/up/contract.rs @@ -73,7 +73,7 @@ impl UpContractCommand { if self.gas_limit.is_some() && self.proof_size.is_some() { weight_limit = Weight::from_parts(self.gas_limit.unwrap(), self.proof_size.unwrap()); } else { - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Doing a dry run to estimate the gas..."); weight_limit = match dry_run_gas_estimate_instantiate(&instantiate_exec).await { Ok(w) => { @@ -87,7 +87,7 @@ impl UpContractCommand { }, }; } - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("Uploading and instantiating the contract..."); let contract_address = instantiate_smart_contract(instantiate_exec, weight_limit) .await diff --git a/crates/pop-cli/src/commands/up/parachain.rs b/crates/pop-cli/src/commands/up/parachain.rs index 6a6774cc2..7eebffff4 100644 --- a/crates/pop-cli/src/commands/up/parachain.rs +++ b/crates/pop-cli/src/commands/up/parachain.rs @@ -2,9 +2,13 @@ use crate::style::{style, Theme}; use clap::Args; -use cliclack::{clear_screen, confirm, intro, log, outro, outro_cancel, set_theme}; +use cliclack::{ + clear_screen, confirm, intro, log, multi_progress, outro, outro_cancel, set_theme, ProgressBar, +}; use console::{Emoji, Style}; -use pop_parachains::{NetworkNode, Zombienet}; +use pop_parachains::{NetworkNode, Status, Zombienet}; +use std::time::Duration; +use tokio::time::sleep; #[derive(Args)] pub(crate) struct ZombienetCommand { @@ -46,28 +50,43 @@ impl ZombienetCommand { let missing = zombienet.missing_binaries(); if missing.len() > 0 { log::warning(format!( - "The following missing binaries are required: {}", + "⚠ī¸ The following missing binaries are required: {}", missing.iter().map(|b| b.name.as_str()).collect::>().join(", ") ))?; - if !confirm("Would you like to source them automatically now?") + if !confirm("đŸ“Ļ Would you like to source them automatically now?") .initial_value(true) .interact()? { - outro_cancel("Cannot deploy parachain to local network until all required binaries are available.")?; + outro_cancel("đŸšĢ Cannot deploy parachain to local network until all required binaries are available.")?; return Ok(()); } - log::info(format!("They will be cached at {}", &cache.to_str().unwrap()))?; + log::info(format!("ℹī¸ They will be cached at {}", &cache.to_str().unwrap()))?; // Source binaries - let mut spinner = cliclack::spinner(); for binary in missing { - spinner.start(format!("Sourcing {}...", binary.name)); - binary.source(&cache).await?; - spinner.start(format!("Sourcing {} complete.", binary.name)); + let multi = multi_progress(format!("đŸ“Ļ Sourcing {}...", binary.name)); + let progress = multi.add(cliclack::spinner()); + let progress_reporter = ProgressReporter(&progress); + for attempt in (0..=1).rev() { + if let Err(e) = binary.source(&cache, progress_reporter).await { + match attempt { + 0 => { + progress.error(format!("đŸšĢ Sourcing failed: {e}")); + multi.stop(); + return Ok(()); + }, + _ => { + progress.error("đŸšĢ Sourcing attempt failed, retrying..."); + sleep(Duration::from_secs(1)).await; + }, + } + } + } + progress.stop(format!("✅ Sourcing {} complete.", binary.name)); + multi.stop(); } - spinner.stop("Sourcing complete."); } // Finally spawn network and wait for signal to terminate - let mut spinner = cliclack::spinner(); + let spinner = cliclack::spinner(); spinner.start("🚀 Launching local network..."); //tracing_subscriber::fmt().init(); match zombienet.spawn().await { @@ -131,3 +150,13 @@ impl ZombienetCommand { Ok(()) } } + +/// Reports any observed status updates to a progress bar. +#[derive(Copy, Clone)] +struct ProgressReporter<'a>(&'a ProgressBar); + +impl Status for ProgressReporter<'_> { + fn update(&self, status: &str) { + self.0.start(status.replace(" Compiling", "Compiling")) + } +} diff --git a/crates/pop-parachains/src/lib.rs b/crates/pop-parachains/src/lib.rs index 823556017..a5fa0af98 100644 --- a/crates/pop-parachains/src/lib.rs +++ b/crates/pop-parachains/src/lib.rs @@ -12,7 +12,7 @@ pub use build::build_parachain; pub use new_pallet::{create_pallet_template, TemplatePalletConfig}; pub use new_parachain::instantiate_template_dir; pub use templates::{Config, Provider, Template}; -pub use up::Zombienet; +pub use up::{Status, Zombienet}; pub use utils::git::{Git, GitHub, Release}; pub use utils::pallet_helpers::resolve_pallet_path; // External exports diff --git a/crates/pop-parachains/src/up.rs b/crates/pop-parachains/src/up.rs index 612e11b54..5f26f2cd9 100644 --- a/crates/pop-parachains/src/up.rs +++ b/crates/pop-parachains/src/up.rs @@ -6,7 +6,7 @@ use indexmap::IndexMap; use std::{ env::current_dir, fs::{copy, metadata, remove_dir_all, write, File}, - io::Write, + io::{BufRead, Write}, os::unix::fs::PermissionsExt, path::{Path, PathBuf}, }; @@ -382,7 +382,9 @@ impl Zombienet { } } +/// A binary used to launch a node. pub struct Binary { + /// The name of a binary. pub name: String, version: String, path: PathBuf, @@ -390,9 +392,16 @@ pub struct Binary { } impl Binary { - pub async fn source(&self, cache: &PathBuf) -> Result<(), Error> { + /// Sources the binary by either downloading from a url or by cloning a git repository and + /// building locally from the resulting source code. + /// + /// # Arguments + /// + /// * `cache` - path to the local cache + /// * `status` - used to observe status updates + pub async fn source(&self, cache: &PathBuf, status: impl Status) -> Result<(), Error> { for source in &self.sources { - source.process(cache).await?; + source.process(cache, status).await?; } Ok(()) } @@ -426,20 +435,17 @@ impl Source { path: &Path, package: &str, names: impl Iterator, + status: impl Status, ) -> Result<(), Error> { // Build binaries and then copy to cache and target - cmd( - "cargo", - vec![ - "build", - "--release", - "-p", - package, - // "--quiet" - ], - ) - .dir(path) - .run()?; + let reader = cmd("cargo", vec!["build", "--release", "-p", package]) + .dir(path) + .stderr_to_stdout() + .reader()?; + let mut output = std::io::BufReader::new(reader).lines(); + while let Some(Ok(line)) = output.next() { + status.update(&line); + } for (name, dest) in names { copy(path.join(format!("target/release/{name}")), dest)?; } @@ -458,7 +464,18 @@ impl Source { Ok(()) } - pub async fn process(&self, cache: &Path) -> Result>, Error> { + /// Processes the binary source, by either downloading the binary from a url or by cloning a + /// git repository and building locally from the resulting source code. + /// + /// # Arguments + /// + /// * `cache` - path to the local cache + /// * `status` - used to observe status updates + pub async fn process( + &self, + cache: &Path, + status: impl Status, + ) -> Result>, Error> { // Download or clone and build from source match self { Source::Url { name, version, url } => { @@ -469,6 +486,7 @@ impl Source { } // Download required version of binaries + status.update(&format!("Downloading from {url}...")); Self::download(&url, &cache.join(&versioned_name)).await?; Ok(None) }, @@ -483,24 +501,37 @@ impl Source { } let repository_name = GitHub::name(url)?; - // Clone repository into working directory let working_dir = cache.join(".src").join(repository_name); let working_dir = Path::new(&working_dir); - if let Err(e) = Git::clone(url, working_dir, branch.as_deref()) { - if working_dir.exists() { - Self::remove(working_dir)?; + + // Clone repository into working directory + if !working_dir.exists() { + status.update(&format!("Cloning {url}...")); + if let Err(e) = Git::clone(url, working_dir, branch.as_deref()) { + if working_dir.exists() { + // Preserve original error + let _ = Self::remove(working_dir); + } + return Err(e.into()); } - return Err(e.into()); } // Build binaries and finally remove working directory - Self::build_binaries( + if let Err(e) = Self::build_binaries( working_dir, package, versioned_names .iter() .map(|(binary, versioned)| (*binary, cache.join(versioned))), + status, ) - .await?; + .await + { + if working_dir.exists() { + // Preserve original error + let _ = Self::remove(working_dir); + } + return Err(e.into()); + } Self::remove(working_dir)?; Ok(None) }, @@ -519,6 +550,10 @@ impl Source { } /// A versioned name of a binary. + /// + /// # Arguments + /// + /// * `version` - an optional version to be appended to the binary name pub fn versioned_name(name: &str, version: Option<&str>) -> String { match version { Some(version) => format!("{name}-{version}"), @@ -527,6 +562,17 @@ impl Source { } } +/// Trait for observing status updates. +pub trait Status: Copy { + /// Update the observer with the provided `status`. + fn update(&self, status: &str); +} + +impl Status for () { + // no-op: status updates are ignored + fn update(&self, _: &str) {} +} + #[cfg(test)] mod tests { @@ -802,7 +848,7 @@ mod tests { .await?; let missing_binaries = zombienet.missing_binaries(); for binary in missing_binaries { - binary.source(&cache).await?; + binary.source(&cache, ()).await?; } let spawn = zombienet.spawn().await; @@ -841,7 +887,7 @@ mod tests { version: TESTING_POLKADOT_VERSION.to_string(), url: "https://github.com/paritytech/polkadot-sdk/releases/download/polkadot-v1.7.0/polkadot".to_string() }; - let result = source.process(&cache).await; + let result = source.process(&cache, ()).await; assert!(result.is_ok()); assert!(temp_dir.path().join(POLKADOT_BINARY).exists()); @@ -867,7 +913,7 @@ mod tests { version: Some(version), }; - let result = source.process(&cache).await; + let result = source.process(&cache, ()).await; assert!(result.is_ok()); assert!(temp_dir.path().join(POLKADOT_BINARY).exists());