From edff10d113cd271f1b003d2a999c9038c24acbb2 Mon Sep 17 00:00:00 2001 From: Sophie Dankel <47993817+sdankel@users.noreply.github.com> Date: Wed, 3 Jul 2024 06:32:25 -0700 Subject: [PATCH] feat: Add helper function for consistent forc output (#6208) ## Description Makes the indendation and formatting consistent for forc's action messages. ### forc build before ![image](https://github.com/FuelLabs/sway/assets/47993817/08d8a30a-82a0-48f5-92d5-1b6c7329ae57) after ![image](https://github.com/FuelLabs/sway/assets/47993817/5dfd4b78-2f7c-47eb-92cf-5c3f518557ef) ### forc test before ![image](https://github.com/FuelLabs/sway/assets/47993817/6eb2d6fe-416b-47ee-97b2-bcd2fabee4db) after ![image](https://github.com/FuelLabs/sway/assets/47993817/68ee4bf3-7dba-4c26-b696-fd8d6ad50a98) ## Checklist - [ ] I have linked to any relevant issues. - [ ] I have commented my code, particularly in hard-to-understand areas. - [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [ ] I have added tests that prove my fix is effective or that my feature works. - [ ] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [ ] I have requested a review from the relevant team or maintainers. --- Cargo.lock | 24 +++++++++++- forc-pkg/src/lock.rs | 15 ++++--- forc-pkg/src/pkg.rs | 27 ++++++++----- forc-pkg/src/source/git/mod.rs | 14 ++++--- forc-pkg/src/source/ipfs.rs | 29 +++++++------- forc-plugins/forc-doc/Cargo.toml | 2 +- forc-plugins/forc-doc/src/lib.rs | 28 +++++++------ forc-tracing/Cargo.toml | 3 ++ forc-tracing/src/lib.rs | 67 ++++++++++++++++++++++++++++++++ forc-util/src/lib.rs | 10 ++--- forc/src/cli/commands/test.rs | 23 ++++++----- forc/src/ops/forc_template.rs | 11 ++++-- 12 files changed, 182 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d19bd9f0989..bc48708dfad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2084,11 +2084,11 @@ version = "0.61.1" dependencies = [ "anyhow", "clap 4.5.7", - "colored", "comrak", "dir_indexer", "expect-test", "forc-pkg", + "forc-tracing 0.61.1", "forc-util", "horrorshow", "include_dir", @@ -2201,6 +2201,7 @@ dependencies = [ "ansi_term", "tracing", "tracing-subscriber", + "tracing-test", ] [[package]] @@ -7500,6 +7501,27 @@ dependencies = [ "tracing-serde", ] +[[package]] +name = "tracing-test" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "557b891436fe0d5e0e363427fc7f217abf9ccd510d5136549847bdcbcd011d68" +dependencies = [ + "tracing-core", + "tracing-subscriber", + "tracing-test-macro", +] + +[[package]] +name = "tracing-test-macro" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "04659ddb06c87d233c566112c1c9c5b9e98256d9af50ec3bc9c8327f873a7568" +dependencies = [ + "quote", + "syn 2.0.66", +] + [[package]] name = "try-lock" version = "0.2.5" diff --git a/forc-pkg/src/lock.rs b/forc-pkg/src/lock.rs index b44504cf2b7..594c107bc09 100644 --- a/forc-pkg/src/lock.rs +++ b/forc-pkg/src/lock.rs @@ -1,5 +1,6 @@ use crate::{pkg, source, DepKind, Edge}; use anyhow::{anyhow, Result}; +use forc_tracing::{println_action_green, println_action_red}; use petgraph::{visit::EdgeRef, Direction}; use serde::{Deserialize, Serialize}; use std::{ @@ -351,10 +352,9 @@ where true => format!(" {}", pkg.source), false => String::new(), }; - tracing::info!( - " {} {}{src}", - ansi_term::Colour::Red.bold().paint("Removing"), - ansi_term::Style::new().bold().paint(&pkg.name) + println_action_red( + "Removing", + &format!("{}{src}", ansi_term::Style::new().bold().paint(&pkg.name)), ); } } @@ -370,10 +370,9 @@ where true => format!(" {}", pkg.source), false => "".to_string(), }; - tracing::info!( - " {} {}{src}", - ansi_term::Colour::Green.bold().paint("Adding"), - ansi_term::Style::new().bold().paint(&pkg.name) + println_action_green( + "Adding", + &format!("{}{src}", ansi_term::Style::new().bold().paint(&pkg.name)), ); } } diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index 09b00fa5057..fda1ec4d2c1 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -9,7 +9,7 @@ use crate::{ BuildProfile, }; use anyhow::{anyhow, bail, Context, Error, Result}; -use forc_tracing::println_warning; +use forc_tracing::{println_action_green, println_warning}; use forc_util::{ default_output_directory, find_file_name, kebab_to_snake_case, print_compiling, print_on_failure, print_warnings, @@ -497,7 +497,7 @@ impl BuiltPackage { let json_abi_path = output_dir.join(program_abi_stem).with_extension("json"); self.write_json_abi(&json_abi_path, minify)?; - info!(" Bytecode size: {} bytes", self.bytecode.bytes.len()); + debug!(" Bytecode size: {} bytes", self.bytecode.bytes.len()); // Additional ops required depending on the program type match self.tree_type { TreeType::Contract => { @@ -531,7 +531,7 @@ impl BuiltPackage { let hash_file_name = format!("{}{}", &pkg_name, SWAY_BIN_HASH_SUFFIX); let hash_path = output_dir.join(hash_file_name); fs::write(hash_path, &bytecode_hash)?; - info!(" Bytecode hash: {}", bytecode_hash); + debug!(" Bytecode hash: {}", bytecode_hash); } _ => (), } @@ -713,7 +713,10 @@ impl BuildPlan { cause, ); } - info!(" Creating a new `Forc.lock` file. (Cause: {})", cause); + println_action_green( + "Creating", + &format!("a new `Forc.lock` file. (Cause: {})", cause), + ); let member_names = manifests .iter() .map(|(_, manifest)| manifest.project.name.to_string()) @@ -723,7 +726,7 @@ impl BuildPlan { .map_err(|e| anyhow!("failed to serialize lock file: {}", e))?; fs::write(lock_path, string) .map_err(|e| anyhow!("failed to write lock file: {}", e))?; - info!(" Created new lock file at {}", lock_path.display()); + debug!(" Created new lock file at {}", lock_path.display()); } Ok(plan) @@ -2171,11 +2174,13 @@ pub fn build_with_options(build_options: &BuildOpts) -> Result { )?; let output_dir = pkg.output_directory.as_ref().map(PathBuf::from); - let finished = ansi_term::Colour::Green.bold().paint("Finished"); - info!( - " {finished} {} in {:.2}s", - profile_target_string(&build_profile.name, build_target), - build_start.elapsed().as_secs_f32() + println_action_green( + "Finished", + &format!( + "{} in {:.2}s", + profile_target_string(&build_profile.name, build_target), + build_start.elapsed().as_secs_f32() + ), ); for (node_ix, built_package) in built_packages { print_pkg_summary_header(&built_package); @@ -2299,6 +2304,8 @@ pub fn build( let manifest = &plan.manifest_map()[&pkg.id()]; let program_ty = manifest.program_type().ok(); + // TODO: Only print "Compiling" when the dependency is not already compiled. + // https://github.com/FuelLabs/sway/issues/6209 print_compiling( program_ty.as_ref(), &pkg.name, diff --git a/forc-pkg/src/source/git/mod.rs b/forc-pkg/src/source/git/mod.rs index fdaf0b8a8b0..a89e77f3128 100644 --- a/forc-pkg/src/source/git/mod.rs +++ b/forc-pkg/src/source/git/mod.rs @@ -6,6 +6,7 @@ use crate::{ source, }; use anyhow::{anyhow, bail, Context, Result}; +use forc_tracing::println_action_green; use forc_util::git_checkouts_directory; use serde::{Deserialize, Serialize}; use std::fmt::Display; @@ -15,7 +16,6 @@ use std::{ path::{Path, PathBuf}, str::FromStr, }; -use tracing::info; #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Deserialize, Serialize)] pub struct Url { @@ -204,11 +204,13 @@ impl source::Fetch for Pinned { { let _guard = lock.write()?; if !repo_path.exists() { - info!( - " {} {} {}", - ansi_term::Color::Green.bold().paint("Fetching"), - ansi_term::Style::new().bold().paint(ctx.name), - self + println_action_green( + "Fetching", + &format!( + "{} {}", + ansi_term::Style::new().bold().paint(ctx.name), + self + ), ); fetch(ctx.fetch_id(), ctx.name(), self)?; } diff --git a/forc-pkg/src/source/ipfs.rs b/forc-pkg/src/source/ipfs.rs index 70dc988588d..1a1c5e914fc 100644 --- a/forc-pkg/src/source/ipfs.rs +++ b/forc-pkg/src/source/ipfs.rs @@ -4,6 +4,7 @@ use crate::{ source, }; use anyhow::Result; +use forc_tracing::println_action_green; use futures::TryStreamExt; use ipfs_api::IpfsApi; use ipfs_api_backend_hyper as ipfs_api; @@ -14,7 +15,6 @@ use std::{ str::FromStr, }; use tar::Archive; -use tracing::info; #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct Cid(cid::Cid); @@ -67,11 +67,13 @@ impl source::Fetch for Pinned { { let _guard = lock.write()?; if !repo_path.exists() { - info!( - " {} {} {}", - ansi_term::Color::Green.bold().paint("Fetching"), - ansi_term::Style::new().bold().paint(ctx.name), - self + println_action_green( + "Fetching", + &format!( + "{} {}", + ansi_term::Style::new().bold().paint(ctx.name), + self + ), ); let cid = &self.0; let ipfs_client = ipfs_client(); @@ -79,17 +81,16 @@ impl source::Fetch for Pinned { futures::executor::block_on(async { match ctx.ipfs_node() { source::IPFSNode::Local => { - info!( - " {} with local IPFS node", - ansi_term::Color::Green.bold().paint("Fetching") - ); + println_action_green("Fetching", "with local IPFS node"); cid.fetch_with_client(&ipfs_client, &dest).await } source::IPFSNode::WithUrl(ipfs_node_gateway_url) => { - info!( - " {} from {}. Note: This can take several minutes.", - ansi_term::Color::Green.bold().paint("Fetching"), - ipfs_node_gateway_url + println_action_green( + "Fetching", + &format!( + "from {}. Note: This can take several minutes.", + ipfs_node_gateway_url + ), ); cid.fetch_with_gateway_url(ipfs_node_gateway_url, &dest) .await diff --git a/forc-plugins/forc-doc/Cargo.toml b/forc-plugins/forc-doc/Cargo.toml index 19cff7c7990..c9d8c1fdb89 100644 --- a/forc-plugins/forc-doc/Cargo.toml +++ b/forc-plugins/forc-doc/Cargo.toml @@ -11,9 +11,9 @@ repository.workspace = true [dependencies] anyhow = "1.0.65" clap = { version = "4.5.4", features = ["derive"] } -colored = "2.0.0" comrak = "0.16" forc-pkg = { version = "0.61.1", path = "../../forc-pkg" } +forc-tracing = { version = "0.61.1", path = "../../forc-tracing" } forc-util = { version = "0.61.1", path = "../../forc-util" } horrorshow = "0.8.4" include_dir = "0.7.3" diff --git a/forc-plugins/forc-doc/src/lib.rs b/forc-plugins/forc-doc/src/lib.rs index 5369d5b327f..d629b606563 100644 --- a/forc-plugins/forc-doc/src/lib.rs +++ b/forc-plugins/forc-doc/src/lib.rs @@ -6,13 +6,13 @@ pub mod tests; use anyhow::{bail, Result}; use cli::Command; -use colored::Colorize; use doc::Documentation; use forc_pkg as pkg; use forc_pkg::{ manifest::{GenericManifestFile, ManifestFile}, PackageManifestFile, }; +use forc_tracing::println_action_green; use forc_util::default_output_directory; use render::RenderedDocumentation; use std::{ @@ -77,11 +77,13 @@ pub fn compile_html( } fs::create_dir_all(&doc_path)?; - println!( - " {} {} ({})", - "Compiling".bold().yellow(), - pkg_manifest.project_name(), - manifest.dir().to_string_lossy() + println_action_green( + "Compiling", + &format!( + "{} ({})", + pkg_manifest.project_name(), + manifest.dir().to_string_lossy() + ), ); let member_manifests = manifest.member_manifests()?; @@ -179,11 +181,13 @@ fn build_docs( pkg_manifest, } = program_info; - println!( - " {} documentation for {} ({})", - "Building".bold().yellow(), - pkg_manifest.project_name(), - manifest.dir().to_string_lossy() + println_action_green( + "Building", + &format!( + "documentation for {} ({})", + pkg_manifest.project_name(), + manifest.dir().to_string_lossy() + ), ); let raw_docs = Documentation::from_ty_program( @@ -210,7 +214,7 @@ fn build_docs( // write file contents to doc folder write_content(rendered_docs, doc_path)?; - println!(" {}", "Finished".bold().yellow()); + println_action_green("Finished", pkg_manifest.project_name()); Ok(raw_docs) } diff --git a/forc-tracing/Cargo.toml b/forc-tracing/Cargo.toml index 6815ca322ee..b6384abcb43 100644 --- a/forc-tracing/Cargo.toml +++ b/forc-tracing/Cargo.toml @@ -12,3 +12,6 @@ repository.workspace = true ansi_term = "0.12" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["ansi", "env-filter", "json"] } + +[dev-dependencies] +tracing-test = "0.2" \ No newline at end of file diff --git a/forc-tracing/src/lib.rs b/forc-tracing/src/lib.rs index 7411091da19..0141aa7bf64 100644 --- a/forc-tracing/src/lib.rs +++ b/forc-tracing/src/lib.rs @@ -9,6 +9,37 @@ use tracing_subscriber::{ fmt::MakeWriter, }; +const ACTION_COLUMN_WIDTH: usize = 12; + +/// Returns the indentation for the action prefix relative to [ACTION_COLUMN_WIDTH]. +fn get_action_indentation(action: &str) -> String { + if action.len() < ACTION_COLUMN_WIDTH { + " ".repeat(ACTION_COLUMN_WIDTH - action.len()) + } else { + "".to_string() + } +} + +/// Prints an action message with a green-bold prefix like " Compiling ". +pub fn println_action_green(action: &str, txt: &str) { + tracing::info!( + "{}{} {}", + get_action_indentation(action), + Colour::Green.bold().paint(action), + txt + ); +} + +/// Prints an action message with a red-bold prefix like " Removing ". +pub fn println_action_red(action: &str, txt: &str) { + tracing::info!( + "{}{} {}", + get_action_indentation(action), + Colour::Red.bold().paint(action), + txt + ); +} + /// Prints a warning message to stdout with the yellow prefix "warning: ". pub fn println_warning(txt: &str) { tracing::warn!("{}: {}", Colour::Yellow.paint("warning"), txt); @@ -148,3 +179,39 @@ pub fn init_tracing_subscriber(options: TracingSubscriberOptions) { builder.init(); } } + +#[cfg(test)] +mod tests { + use super::*; + use tracing_test::traced_test; + + #[traced_test] + #[test] + fn test_println_action_green() { + let txt = "main.sw"; + println_action_green("Compiling", txt); + + let expected_action = "\x1b[1;32mCompiling\x1b[0m"; + assert!(logs_contain(&format!(" {} {}", expected_action, txt))); + } + + #[traced_test] + #[test] + fn test_println_action_green_long() { + let txt = "main.sw"; + println_action_green("Supercalifragilistic", txt); + + let expected_action = "\x1b[1;32mSupercalifragilistic\x1b[0m"; + assert!(logs_contain(&format!("{} {}", expected_action, txt))); + } + + #[traced_test] + #[test] + fn test_println_action_red() { + let txt = "main"; + println_action_red("Removing", txt); + + let expected_action = "\x1b[1;31mRemoving\x1b[0m"; + assert!(logs_contain(&format!(" {} {}", expected_action, txt))); + } +} diff --git a/forc-util/src/lib.rs b/forc-util/src/lib.rs index da996311fc2..a630f741023 100644 --- a/forc-util/src/lib.rs +++ b/forc-util/src/lib.rs @@ -3,9 +3,8 @@ use annotate_snippets::{ renderer::{AnsiColor, Style}, Annotation, AnnotationType, Renderer, Slice, Snippet, SourceAnnotation, }; -use ansi_term::Colour; use anyhow::{bail, Context, Result}; -use forc_tracing::{println_error, println_red_err, println_yellow_err}; +use forc_tracing::{println_action_green, println_error, println_red_err, println_yellow_err}; use std::{ collections::{hash_map, HashSet}, fmt::Display, @@ -346,10 +345,9 @@ pub fn print_compiling(ty: Option<&TreeType>, name: &str, src: &dyn std::fmt::Di Some(ty) => format!("{} ", program_type_str(ty)), None => "".to_string(), }; - tracing::debug!( - " {} {ty}{} ({src})", - Colour::Green.bold().paint("Compiling"), - ansi_term::Style::new().bold().paint(name) + println_action_green( + "Compiling", + &format!("{ty}{} ({src})", ansi_term::Style::new().bold().paint(name)), ); } diff --git a/forc/src/cli/commands/test.rs b/forc/src/cli/commands/test.rs index f8e4001fa42..86387522029 100644 --- a/forc/src/cli/commands/test.rs +++ b/forc/src/cli/commands/test.rs @@ -3,6 +3,7 @@ use ansi_term::Colour; use clap::Parser; use forc_pkg as pkg; use forc_test::{decode_log_data, TestFilter, TestRunnerCount, TestedPackage}; +use forc_tracing::println_action_green; use forc_util::{tx_utils::format_log_receipts, ForcError, ForcResult}; use pkg::manifest::build_profile::ExperimentalFlags; use sway_core::fuel_prelude::fuel_tx::Receipt; @@ -90,12 +91,15 @@ pub(crate) fn exec(cmd: Command) -> ForcResult<()> { let test_count = built_tests.test_count(test_filter.as_ref()); let num_tests_running = test_count.total - test_count.ignored; let num_tests_ignored = test_count.ignored; - info!( - " Running {} {}, filtered {} {}", - num_tests_running, - formatted_test_count_string(&num_tests_running), - num_tests_ignored, - formatted_test_count_string(&num_tests_ignored) + println_action_green( + "Running", + &format!( + "{} {}, filtered {} {}", + num_tests_running, + formatted_test_count_string(&num_tests_running), + num_tests_ignored, + formatted_test_count_string(&num_tests_ignored) + ), ); let tested = built_tests.run(test_runner_count, test_filter)?; let duration = start.elapsed(); @@ -105,10 +109,11 @@ pub(crate) fn exec(cmd: Command) -> ForcResult<()> { forc_test::Tested::Workspace(pkgs) => { for pkg in &pkgs { let built = &pkg.built.descriptor.name; - info!("\n tested -- {built}\n"); + info!("\ntested -- {built}\n"); print_tested_pkg(pkg, &test_print_opts)?; } - info!("\n Finished in {:?}", duration); + info!(""); + println_action_green("Finished", &format!("in {:?}", duration)); pkgs.iter().all(|pkg| pkg.tests_passed()) } forc_test::Tested::Package(pkg) => { @@ -210,7 +215,7 @@ fn print_tested_pkg(pkg: &TestedPackage, test_print_opts: &TestPrintOpts) -> For .map(|test_result| test_result.duration) .sum(); info!( - " Result: {}. {} passed. {} failed. Finished in {:?}.", + "\ntest result: {}. {} passed; {} failed; finished in {:?}", color.paint(state), succeeded, failed, diff --git a/forc/src/ops/forc_template.rs b/forc/src/ops/forc_template.rs index 07a0b5846bb..23703ea575e 100644 --- a/forc/src/ops/forc_template.rs +++ b/forc/src/ops/forc_template.rs @@ -4,6 +4,7 @@ use forc_pkg::{ manifest::{self, PackageManifest}, source::{self, git::Url}, }; +use forc_tracing::println_action_green; use forc_util::validate_project_name; use fs_extra::dir::{copy, CopyOptions}; use std::fs::File; @@ -11,7 +12,6 @@ use std::io::{Read, Write}; use std::path::{Path, PathBuf}; use std::{env, str::FromStr}; use sway_utils::constants; -use tracing::info; pub fn init(command: TemplateCommand) -> Result<()> { validate_project_name(&command.project_name)?; @@ -31,7 +31,7 @@ pub fn init(command: TemplateCommand) -> Result<()> { let fetch_ts = std::time::Instant::now(); let fetch_id = source::fetch_id(current_dir, fetch_ts); - info!("Resolving the HEAD of {}", source.repo); + println_action_green("Resolving", &format!("the HEAD of {}", source.repo)); let git_source = source::git::pin(fetch_id, &local_repo_name, source)?; let repo_path = source::git::commit_path( @@ -40,7 +40,7 @@ pub fn init(command: TemplateCommand) -> Result<()> { &git_source.commit_hash, ); if !repo_path.exists() { - info!(" Fetching {}", git_source.to_string()); + println_action_green("Fetching", git_source.to_string().as_str()); source::git::fetch(fetch_id, &local_repo_name, &git_source)?; } @@ -65,7 +65,10 @@ pub fn init(command: TemplateCommand) -> Result<()> { // Create the target dir let target_dir = current_dir.join(&command.project_name); - info!("Creating {} from template", &command.project_name); + println_action_green( + "Creating", + &format!("{} from template", &command.project_name), + ); // Copy contents from template to target dir copy_template_to_target(&from_path, &target_dir)?;