[rush-lib] Add experiment flag to allow skipping build with warning

[zecheng01]

Summary

The problem: currently a build operation with warning is not allowed to use the incremental build (build skipping) feature. Consequently, the package-deps_build.json is not saved to disk which prevents further change detection in future runs. Preventing this is understandably a good default for discouraging build with warning, however, there are cases in which frameworks like Angular write to stderr for logs, which creates the warning. This make even newly generated projects non-skippable by default.

Just like the existing buildCacheWithAllowWarningsInSuccessfulBuild flag, it would be great to provide this flexibility to projects that conciously accept the risk or is hard to alter the default behaviours.

The proposed change:

• add a flag to support skipping build with status sucess with warning
• persist package-deps_build.json when the flag is enabled and the commands are configured to allow warnings.

How it was tested

• test default experiments.json file
  • buildSkipWithAllowWarningsInSuccessfulBuild is disabled
  • build an angular project for 3 times, none of them skipped build
• test with flag on
  • configure a custom command with allowWarningsInSuccessfulBuild and incremental on
  • enable buildSkipWithAllowWarningsInSuccessfulBuild
  • build an angular project for 3 times, warm runs skipped build
  • modify the project, the next build is not skipped
  • package-deps_build.json was updated
  • subsequent runs are skipped again

Impacted documentation[

https://rushjs.io/pages/advanced/incremental_builds/

[octogonz-dev]

Some thoughts about this:

1. 👍 We could have a valid conversation about "whether Rush should cache build results with errors and/or warnings." The reason why we don't do that today, is because often errors/warnings are nondeterministic, and it is safer to always retry. However, Cobuilds already forced us to question this design decision or at least relax it a bit, because it uses the cache to communicate console output between machines.

2. 👍 We could also have a valid conversation about "whether Rush could support other models for representing warnings/errors besides our STDERR heuristic." The STDERR heuristic is simple and toolchain agnostic, but it can sometimes be a bit of work to retrofit onto certain toolchains. In the case of Angular, we would strongly recommend to do the work to ensure that Angular commands write all errors/warnings to STDERR and all other output to STDOUT. Usually this is just a simple wrapper or configuration, but if some other communication mechanism would be easier to implement (e.g. writing warnings into a temporary file), Rush could consider supporting that.

3. 👎 In a professional monorepo, we would strongly discourage a model where Warnings/errors are printed during a "successful" build. This is what buildCacheWithAllowWarningsInSuccessfulBuild provides, but it undermines the definition of the word "warning." A "warning" is something that the engineer needs to fix or suppress before they can merge. (An "error" is something the engineer must fix immediately.) Whereas, if I go to build main and see a wall of yellow "warning" text that does not break the build, I will ignore it as irrelevant noise, and therefore I will also ignore any new warnings caused by my own work. As a result, the entire distinction of Warning: becomes meaningless. They are semantically equivalent to info logs, and we have lost the ability to get the engineer's attention about potential problems.

[Faithfinder]

At the very least Docker writes progress reports to stderr :( Have to use the ignore warnings flag in our "build-docker" command because of this :(

[zecheng01]

@octogonz-proxy thanks for your insights, and I fully agree with the reasoning of having a strict mode to keep engineers concious about the existence of warnings outlined in (3). From an end user's perspective, I think the issue is more about providing an escape hatch that allows user to opt into the feature while giving some frictions in configuration so the risk is acknowledged when they use it. With the current design, user need to enable the flags both at experiment.json level and command level, which I think is enough friction to make user concious about their choice.

For (2), this is the biggest challenge we are facing in terms of fully embracing the STDERR heuristic. Being strict about not allowing warning output is very feasible for greenfield projects. But sometimes we are working with repos with some history. It's very hard to tell at this point downstream what is real warning in stderr and filter them out properly. From existing PRs opened against Angular and it's ecosystem, this is a common practice and not configurable. They output progress and info to stderr, so is warning. Having an flag that we can turn on is probably the most reasonable choice in our use case, as production build output is pretty stable without real warnings, and most if not all warnings are caught in dev mode without caching/skipping

For (1), for caching behaviour with warning, I think it might be helpful, as there is no gurantee a successful build is deterministic either and it's a communication mechanism in some cases. The problem lies in educating newcomers about what is cachable and what is not, and what is skippable and what are the risks. In the incremental build case, our current repo is full of angular projects, but the build skipping behaviour has stopped working long ago, but almost no one is even aware of it. Consider the incremental build is still the default behaviour on repo init, I think it might be beneficial to additionally provide some hint in CLI output to indicate why skipping did not happen due to warning.

[dmichon-msft]

A flag you can turn on:
Add the following to the npm script definition if you don't want stderr to cause warnings: 2>&1 (i.e. redirect stderr to stdout).

For more granular control, wrap the invocation in another process (can be a bash script or something else) that filters the output.

[zecheng01]

@dmichon-msft Thanks for tips! We have implemented similar redirection for scripts in the past. It created some inconviniences in other situations like logging in CI, where stderr and stdout are treated differently. Well in this case, I don't think there is a perfect solution, just hoping for an escape hatch similar to buildCacheWithAllowWarningsInSuccessfulBuild for the build caching feature. Eventually, we have opted for build caching.