feat: display output of failed builds#125
Conversation
raymondk
left a comment
There was a problem hiding this comment.
I tried this out in the examples/icp-progress-bar-test-bed example.
[canister-1] ✺
Building: script (command: sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1') 3 of 4
> failing build step 10
> failing build step 11
> failing build step 12
> failing build step 13
[canister-2] ✔ Built successfully
[canister-3] ✔ Built successfully Build output for canister canister-1:
Building: script (command: lorem.sh 5) 1 of 4
Running command: lorem.sh 5
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
Lorem ipsum dolor sit amet, consectetur adipiscing elit.
Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
Building: script (command: lorem.sh 5) 2 of 4
Running command: lorem.sh 5
Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.
Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
Building: script (command: sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1') 3 of 4
Running command: sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1'
failing build step 1
failing build step 2
failing build step 3
failing build step 4
failing build step 5
failing build step 6
failing build step 7
failing build step 8
failing build step 9
failing build step 10
failing build step 11
failing build step 12
[canister-1] ✘ Failed to build canister: command 'sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1'' failed with status code 1
[canister-2] ✔ Built successfully
[canister-3] ✔ Built successfully Error: command 'sh -c 'for i in $(seq 1 20); do echo "failing build step $i"; done; exit 1'' failed with status code 1```
A couple of observations:
- I think the last line might not be necessary since it's alredy under [canister-1]
- It looks like the "built successfully" is duplicated (maybe that is intentional, i think it's nice to have a summary at the bottom)
- The first line `[canister-1] *` still looks like it's in progress.
| 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(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I reworked output display significantly. It's now all handled by the progress bar
| SourceField::Local(s) => { | ||
| stdio | ||
| .send(format!("Reading local file: {}", s.path)) | ||
| .await?; |
There was a problem hiding this comment.
This will never actually be displayed to the user, right? Because it will (likely) finish almost instantly.
There was a problem hiding this comment.
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
|
|
||
| // Verify the checksum if it's provided | ||
| if let Some(expected) = &adapter.sha256 { | ||
| stdio.send("Verifying checksum".to_string()).await?; |
There was a problem hiding this comment.
Same as above - will it actually get shown, if it completes almost instantly?
| // After progress bar is finished, dump the output if build failed | ||
| if let Err(e) = &result { | ||
| pb.dump_output(ctx); | ||
| let _ = ctx | ||
| .term | ||
| .write_line(&format!("Failed to build canister: {e}")); |
There was a problem hiding this comment.
My gut feeling says there's still some way for the output to get messed up because of progress bars, but I don't see anything specific.
rikonor
left a comment
There was a problem hiding this comment.
LGTM, just left one nitpick about the naming of SomeProgressBar.
Currently, when a build fails, the error is not visible to the user. It is only briefly displayed in the rolling 4-line buffer. With this PR, the build step outputs are buffered, and for every build that fails, the output is displayed with a readable report.
Example (remaining) output:
https://dfinity.atlassian.net/browse/SDK-2305