Skip to content
Merged
48 changes: 42 additions & 6 deletions crates/icp-cli/src/commands/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use icp::{

use crate::{
commands::Context,
progress::{ProgressManager, ScriptProgressHandler},
progress::{MAX_LINES_PER_STEP, ProgressManager, RollingLines, ScriptProgressHandler},
};

#[derive(Parser, Debug)]
Expand Down Expand Up @@ -39,6 +39,9 @@ pub enum CommandError {

#[error(transparent)]
Unexpected(#[from] anyhow::Error),

#[error("failed to join build output")]
JoinError(#[from] tokio::task::JoinError),
}

/// Executes the build command, compiling canisters defined in the project manifest.
Expand Down Expand Up @@ -93,34 +96,46 @@ pub async fn exec(ctx: &Context, cmd: Cmd) -> Result<(), CommandError> {
let wasm_output_path = build_dir.path().join("out.wasm");

let step_count = c.build.steps.len();
let mut step_outputs = vec![];
for (i, step) in c.build.steps.iter().enumerate() {
// Indicate to user the current step being executed
let current_step = i + 1;
let pb_hdr = format!("Building: {step} {current_step} of {step_count}");
let pb_hdr = format!("\nBuilding: {step} {current_step} of {step_count}");

let script_handler = ScriptProgressHandler::new(pb.clone(), pb_hdr.clone());

// Setup script progress handling and receiver join handle
let (tx, rx) = script_handler.setup_output_handler();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we have this script_handler and setup_output_handler if you are now handling output in the command itself? Can the script handler not be made to support outputting failure information as well? The point was to remove the burden of handling these details from the command (I believe this applies to build and sync both).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I reworked output display significantly. It's now all handled by the progress bar


// Perform build step
ctx.builder
let build_result = ctx
.builder
.build(
step, // step
&Params {
path: canister_path.to_owned(),
output: wasm_output_path.to_owned(),
},
Some(tx),
tx,
Comment thread
viviveevee marked this conversation as resolved.
Outdated
)
.await?;
.await;

// Ensure background receiver drains all messages
let _ = rx.await;
let step_output = rx.await?;
step_outputs.push(StepOutput {
title: pb_hdr,
output: step_output,
});

if let Err(e) = build_result {
dump_build_output(&c.name, step_outputs);
return Err(CommandError::Build(e));
}
}

// Verify a file exists in the wasm output path
if !wasm_output_path.exists() {
dump_build_output(&c.name, step_outputs);
return Err(CommandError::MissingOutput);
}

Expand Down Expand Up @@ -158,3 +173,24 @@ pub async fn exec(ctx: &Context, cmd: Cmd) -> Result<(), CommandError> {

Ok(())
}

#[derive(Debug)]
struct StepOutput {
title: String,
output: RollingLines,
}

fn dump_build_output(canister_name: &str, step_outputs: Vec<StepOutput>) {
let crop_message = if step_outputs.len() == MAX_LINES_PER_STEP {
format!(" (last {MAX_LINES_PER_STEP} lines)")
} else {
String::new()
};
println!("Build output for canister {canister_name}{crop_message}:");
Comment thread
viviveevee marked this conversation as resolved.
Outdated
for step_output in step_outputs {
println!("{}", step_output.title);
for line in step_output.output.iter() {
println!("{}", line);
}
}
}
31 changes: 15 additions & 16 deletions crates/icp-cli/src/progress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ use itertools::Itertools;
use tokio::{sync::mpsc, task::JoinHandle};
use tracing::debug;

/// The maximum number of lines to display for a step output
pub const MAX_LINES_PER_STEP: usize = 10_000;

// Animation frames for the spinner - creates a rotating star effect
const TICKS: &[&str] = &["✶", "✸", "✹", "✺", "✹", "✷"];

Expand Down Expand Up @@ -40,14 +43,9 @@ pub struct RollingLines {
}

impl RollingLines {
/// Create a new buffer with a fixed capacity, pre-filled with empty strings.
/// Create a new buffer with a fixed capacity.
pub fn new(capacity: usize) -> Self {
let mut buf = VecDeque::with_capacity(capacity);

for _ in 0..capacity {
Comment thread
raymondk marked this conversation as resolved.
buf.push_back(String::new());
}

let buf = VecDeque::with_capacity(capacity);
Self { buf, capacity }
}

Expand Down Expand Up @@ -165,7 +163,7 @@ impl ScriptProgressHandler {

/// Create a channel and start handling script output for progress updates
/// Returns the sender and a join handle for the background receiver task.
pub fn setup_output_handler(&self) -> (mpsc::Sender<String>, JoinHandle<()>) {
pub fn setup_output_handler(&self) -> (mpsc::Sender<String>, JoinHandle<RollingLines>) {
Comment thread
viviveevee marked this conversation as resolved.
Outdated
let (tx, mut rx) = mpsc::channel::<String>(100);

// Shared progress-bar messaging utility
Expand All @@ -180,23 +178,24 @@ impl ScriptProgressHandler {

// Handle logging from script commands
let handle = tokio::spawn(async move {
// Create a rolling buffer to contain last N lines of terminal output
let mut lines = RollingLines::new(4);
// Small rolling buffer to display current output while build is ongoing
let mut rolling = RollingLines::new(4);
// Total output buffer to display full build output later
let mut complete = RollingLines::new(MAX_LINES_PER_STEP); // We need _some_ limit to prevent consuming infinite memory

while let Some(line) = rx.recv().await {
debug!(line);

// Update output buffer
lines.push(line);
rolling.push(line.clone());
complete.push(line);

// Update progress-bar with rolling terminal output
let msg = lines
.iter()
.filter(|s| !s.is_empty())
.map(|s| format!("> {s}"))
.join("\n");
let msg = rolling.iter().map(|s| format!("> {s}")).join("\n");
set_message(msg);
}

complete
});

(tx, handle)
Expand Down
95 changes: 95 additions & 0 deletions crates/icp-cli/tests/build_tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use camino_tempfile::NamedUtf8TempFile as NamedTempFile;
use indoc::formatdoc;
use predicates::{prelude::PredicateBooleanExt, str::contains};

use crate::common::TestContext;
use icp::fs::write_string;
Expand Down Expand Up @@ -79,3 +80,97 @@ fn build_adapter_script_multiple() {
.assert()
.success();
}

#[test]
fn build_adapter_display_failing_build_output() {
let ctx = TestContext::new();

// Setup project
let project_dir = ctx.create_project_dir("icp");

// Project manifest
let pm = r#"
canisters:
- name: my-canister
build:
steps:
- type: script
command: echo "success 1"
- type: script
command: echo "success 2"
- type: script
command: sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1'
- name: unimportant-canister
build:
steps:
- type: script
command: echo "hide this"
- type: script
command: sh -c 'touch "$ICP_WASM_OUTPUT_PATH"'
"#;
Comment thread
viviveevee marked this conversation as resolved.
Outdated

write_string(
&project_dir.join("icp.yaml"), // path
pm, // contents
)
.expect("failed to write project manifest");

// Invoke build
ctx.icp()
.current_dir(project_dir)
.args(["build"])
.assert()
.failure()
.stdout(contains("Build output for canister my-canister"))
.stdout(contains("Building: script (command: echo \"success 1\") 1 of 3"))
.stdout(contains("success 1"))
.stdout(contains("Building: script (command: echo \"success 2\") 2 of 3"))
.stdout(contains("success 2"))
.stdout(contains("Building: script (command: sh -c 'for i in $(seq 1 20); do echo \"failing build step $i\"; done; exit 1') 3 of 3"))
.stdout(contains("failing build step 1"))
.stdout(contains("failing build step 20"))
.stdout(contains("hide this").not());
Comment thread
viviveevee marked this conversation as resolved.
}

#[test]
fn build_adapter_display_failing_prebuilt_output() {
let ctx = TestContext::new();

// Setup project
let project_dir = ctx.create_project_dir("icp");

// Project manifest with a prebuilt step that will fail (non-existent file)
Comment thread
viviveevee marked this conversation as resolved.
let pm = r#"
canisters:
- name: my-canister
build:
steps:
- type: script
command: echo "initial step succeeded"
- type: pre-built
path: /nonexistent/path/to/wasm.wasm
sha256: 0000000000000000000000000000000000000000000000000000000000000000
Comment thread
viviveevee marked this conversation as resolved.
Outdated
"#;

write_string(
&project_dir.join("icp.yaml"), // path
pm, // contents
)
.expect("failed to write project manifest");

// Invoke build
ctx.icp()
.current_dir(project_dir)
.args(["build"])
.assert()
.failure()
.stdout(contains("Build output for canister my-canister"))
.stdout(contains(
"Building: script (command: echo \"initial step succeeded\") 1 of 2",
))
.stdout(contains("initial step succeeded"))
.stdout(contains(
"Building: pre-built (path: /nonexistent/path/to/wasm.wasm, sha: 0000000000000000000000000000000000000000000000000000000000000000) 2 of 2",
))
.stdout(contains("Reading local file: /nonexistent/path/to/wasm.wasm"));
}
12 changes: 9 additions & 3 deletions crates/icp/src/canister/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{fmt, sync::Arc};
use async_trait::async_trait;
use schemars::JsonSchema;
use serde::Deserialize;
use tokio::sync::mpsc::Sender;
use tokio::sync::mpsc::{Sender, error::SendError};

use crate::{
canister::{build, script::ScriptError},
Expand Down Expand Up @@ -66,6 +66,12 @@ pub enum BuildError {

#[error(transparent)]
Unexpected(#[from] anyhow::Error),

#[error("failed to send build output")]
SendOutput(#[from] SendError<String>),

#[error("failed to join futures")]
JoinError(#[from] tokio::task::JoinError),
Comment thread
viviveevee marked this conversation as resolved.
}

#[async_trait]
Expand All @@ -74,7 +80,7 @@ pub trait Build: Sync + Send {
&self,
step: &build::Step,
params: &Params,
stdio: Option<Sender<String>>,
stdio: Sender<String>,
) -> Result<(), BuildError>;
}

Expand All @@ -89,7 +95,7 @@ impl Build for Builder {
&self,
step: &build::Step,
params: &Params,
stdio: Option<Sender<String>>,
stdio: Sender<String>,
) -> Result<(), BuildError> {
match step {
build::Step::Prebuilt(_) => self.prebuilt.build(step, params, stdio).await,
Expand Down
16 changes: 11 additions & 5 deletions crates/icp/src/canister/prebuilt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,18 @@ impl Build for Prebuilt {
&self,
step: &Step,
params: &Params,
_: Option<Sender<String>>,
stdio: Sender<String>,
) -> Result<(), BuildError> {
// Adapter
let adapter = match step {
Step::Prebuilt(v) => v,
_ => panic!("expected prebuilt adapter"),
let Step::Prebuilt(adapter) = step else {
panic!("expected prebuilt adapter");
};

let wasm = match &adapter.source {
// Local path
SourceField::Local(s) => {
stdio
.send(format!("Reading local file: {}", s.path))
.await?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will never actually be displayed to the user, right? Because it will (likely) finish almost instantly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It will not be visible if everything works out fine, but if there is an error it will show up in the full error output

read(&params.path.join(&s.path)).context("failed to read prebuilt canister file")?
}

Expand All @@ -43,6 +44,7 @@ impl Build for Prebuilt {

// Parse Url
let u = Url::from_str(&s.url).context("failed to parse prebuilt canister url")?;
stdio.send(format!("Fetching remote file: {}", u)).await?;

// Construct request
let req = Request::new(
Expand Down Expand Up @@ -73,6 +75,7 @@ impl Build for Prebuilt {

// Verify the checksum if it's provided
if let Some(expected) = &adapter.sha256 {
stdio.send("Verifying checksum".to_string()).await?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above - will it actually get shown, if it completes almost instantly?

// Calculate checksum
let actual = hex::encode({
let mut h = Sha256::new();
Expand All @@ -89,6 +92,9 @@ impl Build for Prebuilt {
}

// Set WASM file
stdio
.send(format!("Writing WASM file: {}", params.output))
.await?;
write(
&params.output, // path
&wasm, // contents
Expand Down
Loading