Send stdout and stderr to the same stream (build and generate) #1525
Conversation
To ensure ordering of stdout and stderr in build and generate phases, send the stdout and stderr of the executing command to the same stream (stdout). This will have an outside effect of anyone consuming `lifecycle` as what was previously in stderr is now in stdout. Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
Signed-off-by: Jesse Brown <jabrown85@gmail.com>
|
Thank you for this! I agree that (a) combining both output streams makes the most sense to solve this, (b) picking stdout (rather than stderr) for the combined output is best. (Internally we've previously had some discussion about making the buildpack logging utility helpers all default to stderr to work around this, but I think the unix "stderr is for humans" best practice really only applies when the output from stdout is expected to be machine consumable/easily piped, and that doesn't really apply to plain logs. In the future if |
|
Seems good to standardize. Thanks! We had some buildpacks with output to stdout/stderr indicating different things (like they used stderr to indicate only warnings and wanted to assert no warnings). Merging them to one stream provides more consistency. The I have a vauge desire that Ed hinted at to introduce some more meaningful machine-usable outputs from pack. Like being able to retrieve the input image name or other metadata about the build itself. I don't have a strong need today, but imagine something like ruby/rubygems#8728 for pack to reduce/remove the need of people to grep unstructured data would be nice (once we have some strong use-cases). |
`lifecycle` now emits all buildpack output to stdout in order to fix this stdout/stderr output interleaving bug: buildpacks/pack#2330 buildpacks/lifecycle#1525 As such, the `libcnb-test` integration test assertions need updating to use stdout in a number of places. This also allows the test assertions to correctly assert the placement of warning in-between other non-warning content. (And in the process has revealed a couple of places where whitespace could be tweaked, which will be handled separately as part of the future build output style change work). GUS-W-19337364.
`lifecycle` now emits all buildpack output to stdout in order to fix this stdout/stderr output interleaving bug: buildpacks/pack#2330 buildpacks/lifecycle#1525 As such, the `libcnb-test` integration test assertions need updating to use stdout in a number of places. This also allows the test assertions to correctly assert the placement of warning in-between other non-warning content. (And in the process has revealed a couple of places where whitespace could be tweaked, which will be handled separately as part of the future build output style change work). GUS-W-19337364.
CNB builder images v0.20.13 (heroku/cnb-builder-images#784) includes the fix for the interleaved buildpack stdout/stderr (buildpacks/lifecycle#1525), however, that will mean some CNB integration tests that test context.pack_stdout and context.pack_stderr will need updating. The fix is to use stdout instead of stderr as lifecycle is now redirecting the output.
* Fix clippy for Rust 1.89 ``` error: item in documentation is missing backticks --> commons/src/layer/fixtures/metadata_migration_example.md:1:11 | 1 | ## Setup DiffMigrateLayer for new layer Metadata | ^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown = note: `-D clippy::doc-markdown` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::doc_markdown)]` help: try | 1 - ## Setup DiffMigrateLayer for new layer Metadata 1 + ## Setup `DiffMigrateLayer` for new layer Metadata | Checking libcnb-test v0.29.0 error: item in documentation is missing backticks --> commons/src/layer/fixtures/metadata_migration_example.md:108:13 | 108 | - Takes a bullet_stream printer for maximal printing consistency | ^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown help: try | 108 - - Takes a bullet_stream printer for maximal printing consistency 108 + - Takes a `bullet_stream` printer for maximal printing consistency | error: could not compile `commons` (lib) due to 2 previous errors ``` ``` warning: infallible TryFrom impl; consider implementing From, instead --> buildpacks/ruby/src/layers/bundle_install_layer.rs:145:1 | 145 | impl TryFrom<MetadataV3> for MetadataV4 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 146 | type Error = std::convert::Infallible; | ------------------------ infallible error type | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#infallible_try_from = note: `#[warn(clippy::infallible_try_from)]` on by default ``` Close #448 * Fix integration tests to read stdout CNB builder images v0.20.13 (heroku/cnb-builder-images#784) includes the fix for the interleaved buildpack stdout/stderr (buildpacks/lifecycle#1525), however, that will mean some CNB integration tests that test context.pack_stdout and context.pack_stderr will need updating. The fix is to use stdout instead of stderr as lifecycle is now redirecting the output.
4 participants
Summary
To ensure ordering of stdout and stderr in build and generate phases, send the stdout and stderr of the executing command to the same stream (stdout).
This will have an outside effect of anyone consuming
lifecycleas what was previously in stderr is now in stdout.Release notes
lifecycle/builderandlifecycle/generatewill no longer send buildpack stderr to stderr, but instead stdout to preserve ordering.Related
Resolves buildpacks/pack#2330
Context
I looked into and tried a few other options, but ultimately didn't find a path that allowed for us to keep the streams separate and also guarantee ordering. I believe ordering of buildpack produced logs to be more important to more users. We already use a buffer for
detectorbut for different reasons (hidden by default).