[rush] Rush returns a zero exit code when build succeeds with warnings.

[iclanton]

Rush is supposed to return a nonzero exit code when a project succeeds with warnings. As per documentation:

If the command writes anything to the stderr stream, Rush will interpret this to mean that at least one error or warning was reported. This will break the build. (This is by design – if you allow people to merge PRs that “cry wolf”, pretty soon you will find that so many warnings have accumulated that nobody even reads them any more.) Some tooling libraries (e.g. Jest) write to stderr as part of their normal operation; you will need to [redirect their output[(https://github.com/Microsoft/web-build-tools/blob/master/core-build/gulp-core-build/src/tasks/JestReporter.ts).

However, Rush currently returns a zero exit code after reporting a warning. It seems like this has been broken for a while, so any fix should be gated behind an option to ignore warnings.

[iclanton]

To address this issue, we've added a property called allowWarningsInSuccessfulBuild to bulk commands in command-line.json.

When invoking shell scripts, Rush uses a heuristic to distinguish errors from warnings:

• If the shell script returns a nonzero process exit code, Rush interprets this as "one or more errors". Error output is displayed in red, and it prevents Rush from attempting to process any downstream projects.
• If the shell script returns a zero process exit code but writes something to its stderr stream, Rush interprets this as "one or more warnings". Warning output is printed in yellow, but does NOT prevent Rush from processing downstream projects.

Thus, warnings do not interfere with local development, but they will cause a CI job to fail, because the Rush process itself returns a nonzero exit code if there are any warnings or errors. This is by design. In an active monorepo, we've found that if you allow any warnings in your master branch, it inadvertently teaches developers to ignore warnings, which quickly leads to a situation where so many "expected" warnings have accumulated that warnings no longer serve any useful purpose.

Sometimes a poorly behaved task will write output to stderr even though its operation was successful. In that case, it's strongly recommended to fix the task. However, as a workaround you can set allowWarningsInSuccessfulBuild=true, which causes Rush to return a nonzero exit code for errors only.

Note: The default value is false. In Rush 5.7.x and earlier, the default value was true.

[Toxaris]

How can I set allowWarningsInSuccessfulBuild=true for the rush rebuild command? I tried:

{
  "commandKind": "bulk",
  "name": "rebuild",
  "allowWarningsInSuccessfulBuild": true
}

But that is rejected by schema validation:

> rush install

Rush Multi-Project Build Tool 5.9.1 - https://rushjs.io

ERROR: JSON validation failed:
...\common\config\rush\command-line.json
Error: #/commands/0
Data does not match any schemas from 'oneOf'
Error: #/commands/0
Missing required property: enableParallelism
Error: #/commands/0
Missing required property: shellCommand

[octogonz]

😯 You would need to provide a complete command definition to pass schema validation. In other words, if you specify rebuild at all, it completely replaces the built-in command, so you would have to give all the fields. (See _addDefaultBuildActions().)

The solution would look like this:

    {
      "commandKind": "bulk",
      "name": "build",
      "summary": "Build all projects that haven't been built, or have changed since they were last built",
      "description": "This command is similar to \"rush rebuild\", except that \"rush build\" performs...",
      "safeForSimultaneousRushProcesses": false,
      "enableParallelism": true,
      "ignoreDependencyOrder": false,
      "ignoreMissingScript": false,
      "allowWarningsInSuccessfulBuild": true
    },
    {
      "commandKind": "bulk",
      "name": "rebuild",
      "summary": "Clean and rebuild the entire set of projects",
      "description": "This command assumes that the package.json file for each project contains...",
      "safeForSimultaneousRushProcesses": false,
      "enableParallelism": true,
      "ignoreDependencyOrder": false,
      "ignoreMissingScript": false,
      "allowWarningsInSuccessfulBuild": true
    },

@iclanton this is... not a very good user experience. I feel like it's a gap we overlooked in the design of the allowWarningsInSuccessfulBuild migration story.

A simple solution would be to add a special option for the built-in rebuild and and build commands. Probably the right design would be for them to merge with the built-in definition rather than replacing it, as @Toxaris intuitively assumed would be the case above.

[Toxaris]

Yes I hoped the default settings for build and rebuild would be merged with my settings from the command-line.json :)

@octogonz I don't understand how the configuration you propose would enable the default rush behavior with respect to build vs. rebuild:

• rush rebuild calls the build script from the package.json but with above configuration, I would expect that rush rebuild searches for a rebuild script instead. Can I pass the commandToRun in the command-line.json?
• rush build can skip some packages and rush rebuild can't, but I don't see that difference reflected in above configuration. Actually, I don't see it reflected in the linked code either, so maybe it is controlled by the command being called "build". In that case I guess it would keep working?

[octogonz]

@Toxaris today rebuild / build have a special relationship due to Rush's support for incremental builds. They are treated specially compared to custom commands.

We want to generalize the incremental build concept to support custom commands as well. This is a deep topic. You'll find a number of Rush issues that propose parts of a design. This year we're hoping to integrate all those pieces together into major new feature focused around faster builds for large repos.

[aphix]

Just hit this as well.

One thing to add is that when implementing the rebuild command (as proposed by @octogonz here) it'll likely mean that all packages' package.json files will need a rebuild task, e.g.

{
 ...snip...
  "scripts": {
    "build": "...some build steps...", // whatever your normal build steps are
    "rebuild": "npm run build" // <- run the regular build task above when calling rebuild
  }
 ...snip...
}

Also, @octogonz, after adding the commands above, should we expect the "non-incremental" part of rush rebuild to remain the same? Or is there something else that must be done to ensure that it's a full rebuild of all projects after we've overridden the normal rebuild command?

Edit: Just tested it, and it's working fine -- it appears to still be doing a full rebuild, even if some projects have already been built successfully.

[octogonz]

This issue is closed, so the discussion here won't be tracked by anybody. If there isn't already another GitHub issue tracking this topic, please open one. Thanks!

[isc30]

What about rush rebuild?

[dtinth]

Workaround

Add 2>&1 to the end of "build" script to redirect stderr to stdout.

[DmacMcgreg]

This completely breaks the build since vue-cli outputs standard informational logging to the stderr.
I understand the reasoning behind this, but it just seems illogical to force this on users and not allow them to turn it off.

using the 2>&1 solution above, although effective, is not at all ideal...

@octogonz @iclanton

[isc30]

Hey @DmacMcgreg this issue is old and there are new features in place to solve it properly in recent versions of Rush.
If your build errors due to the warning, you can add the following snippet to command-line.json that enables allowWarningsInSuccessfulBuild:

"commands": [
    {
      "name": "build",
      "commandKind": "bulk",
      "summary": "Build all projects that haven't been built, or have changed since they were last built",
      "incremental": true,
      "enableParallelism": true,
      "allowWarningsInSuccessfulBuild": true
    },
    {
      "name": "rebuild",
      "commandKind": "bulk",
      "summary": "Rebuild all projects",
      "incremental": false,
      "enableParallelism": true,
      "allowWarningsInSuccessfulBuild": true
    }
]

By default, build/rebuild will fail if anything is outputted to stderr (allowWarningsInSuccessfulBuild is false by default). allowWarningsInSuccessfulBuild was added as optional for every command, not only build/rebuild.

I hope this solves your issue

[aDavidaIsNoOne]

@isc30 That then requires a rebuild script for every package.json in the repo. How is this considered a solution?

[isc30]

@aDavidaIsNoOne if you only need build then just use the build section from the snippet

[wolthers]

@isc30 You mention there are new features in place in newer versions of rush to solve it. Can you elaborate? I can't find any mention of this in the docs, other than patching the libraries that write to stdout. :) Would be great to not have to "overwrite" the build command just to add the flag.

[isc30]

Hey, when I said that the issue was old I meant that at that time you couldn't mark warnings as success. Now you can by redefining build and rebuild commands to pass with warnings (not ideal but works). Sorry for the confusion

[webpro]

Apologies for adding a comment to an old and closed issue, but might be good to add opinions on this matter so that other people can better judge their own situation (I ranted before about this in kadena-community/kadena.js#442 (comment)).

Imho, allowWarningsInSuccessfulBuild: false should not be the default. It goes against standards and is based on the wrong ideas. Sending warnings and diagnostics to stderr and keeping stdout clean for further processing is perfectly fine, and most developers actually do understand the difference between a warning and an error. Dev/prod parity is invaluable.

I believe making this behavior in Rush the default was the wrong solution for a problem most projects don't have, causing confusion instead. The upside is that I learned a lot about going against well-established standards.

Recently a developer joining our team wasted a lot of time wondering why his next build worked locally, but not in CI. Had nothing to do with a "poorly behaved task", it's totally valid logging from whatever program caused it. Now suddenly Rush forced us to work around that in really ugly ways.

I know we can change the default setting. Just making the case here that it's a very poor one.

We should rely on the process exit code for successful operations. That's it. Just like everyone else. Principle of least astonishment.

[dustinlacewell]

I can't believe they just closed this and never looked back heh.