microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.82k stars 593 forks source link

[rush] rush build error logs no longer include stdout #4882

Open relm923 opened 1 month ago

relm923 commented 1 month ago

Summary

rush build error logs no longer include STDOUT since 5.124.4.

Repro steps

Using https://github.com/microsoft/rush-example

  1. Add TS error in package
  2. rush build
  3. Review generated logs

Expected result: (v5.124.4)

==[ FAILURE: 1 operation ]=====================================================

--[ FAILURE: my-toolchain ]----------------------------------[ 0.77 seconds ]--

Invoking: rimraf ./lib/ && tsc 
src/startCommand.ts(9,1): error TS2552: Cannot find name 'consoleX'. Did you mean 'console'?

Operations failed.

rush build (0.79 seconds)

Actual result: (since v5.124.4)

==[ FAILURE: 1 operation ]=====================================================

--[ FAILURE: my-toolchain ]----------------------------------[ 0.76 seconds ]--

Returned error code: 2

Operations failed.

rush build (0.77 seconds)

Details

Potentially introduced by https://github.com/microsoft/rushstack/pull/4717

Zulip Thread

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question Answer
@microsoft/rush globally installed version?
rushVersion from rush.json? 5.124.4 vs 5.124.5
useWorkspaces from rush.json? true
Operating system? Mac
Would you consider contributing a PR? Yes
Node.js version (node -v)? 20.11.0
iclanton commented 1 month ago

@dmichon-msft, would you mind taking a look at this?

dmichon-msft commented 1 month ago

StdioSummarizer will automatically discard stdout if there is any content to stderr. I think the solution to this may well be that we should abandon that behavior and instead do as BuildXL does, i.e. if the operation fails, dump the entire log.

The change in my PR was to handle issues where operations were failing and not logging anything.

relm923 commented 3 weeks ago

@dmichon-msft Makes sense but the current implementation is particularly painful for build commands using tsc as all error messages are thrown away.

Thoughts on either reverting your change or prioritizing a forward fix?