feat(builtin): add silent_on_success option to npm_package_bin

[ptarjan]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature (please, look at the "Scope of the project" section in the README.md file)
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

Issue Number: N/A

nodejs programs which output on stdout and stderr have no way to be silenced on success

What is the new behavior?

A new flag where we can silence binaries that are loud when they are successful.

I couldn't figure out how to write a test here since sh_test can't invoke bazel build commands easily to check the stdout, and my flag doesn't change the behavior of the stdout nor stderr files incase a user wants to still use them.

Without this flag I get:

$ bazel build internal/node/test:loud_binary
...
INFO: From Action internal/node/test/loud_binary:
WARN: Dropping unreachable code [internal/node/test/terser_input_with_diagnostics.js:6,4]
class Greeter{greet(name){return name}}
Target //internal/node/test:loud_binary up-to-date:
  dist/bin/internal/node/test/loud_binary

and then with it I get:

$ bazel build internal/node/test:loud_binary
...
Target //internal/node/test:loud_binary up-to-date:
  dist/bin/internal/node/test/loud_binary

And if I introduce a syntax error into internal/node/test/terser_input_with_diagnostics.js I still see the errors when running:

$ bazel build internal/node/test:loud_binary
...
Parse error at internal/node/test/terser_input_with_diagnostics.js:9,0
ERROR: Unexpected token: eof (undefined)
...
Target //internal/node/test:loud_binary failed to build

Does this PR introduce a breaking change?

• Yes
• No

Other information

[ptarjan]

I debated doing a breaking change here of cating the output even if it is being captured. What do you think?

[mattem]

I think this is fine, conceptually if the output is "captured" then I can see the case for it not making it to stdout. No ones opened an issue so far saying that they'd like to see the output of captured outs.

[mattem]

Discussed a little with Alex, I think we'd rather move towards the breaking change that I think you're trying to work around, rather than introducing an explicit "silence the tool" attribute.

If we instead introduced a new attribute here, include_captured_stdio_in_default_outputs which defaults to True, which you could set False while setting the stderr, stderr attrs, which would then cause these outputs to be placed on a different OutputGroup, rather than be included in the DefaultInfo provider which would then allow you to silence the output, but not have multiple files on the output. We can later discuss flipping the flag to default False on 6.x and removal in 7.x if we want to go that far too.

Does this cover the use case fully?

[alexeagle]

I think it might also be reasonable to have the new include_captured_stdio_in_default_outputs default to False, and call this a "feature" and anyone relying on those being default outputs would have to flip it back to True.

[ptarjan]

Thanks for looking!

If I go the route of include_captured_stdio_in_default_outputs, then to get the bazel semantics of something which is quiet on success and loud of failure I'd have to:

• set include_captured_stdio_in_default_outputs = False
• do `stdout = "stdout", stderr = "stderr", exit_code_output = "exit_code"
• have a genrule (is that right?) which checks the "exit_code" and if it is not 0, fails and prints stdout and stderr

All that would be possible today without the extra flag, right? Since I'd need the genrule to output the output anyways?

I guess we are smashing the JS-world into the bazel world and I'm hoping the rule feels more bazel-y since the consumers of it are other bazel rules.

[mattem]

Yes, correct. The part that breaks at rh is that the rule consuming the output of the loud tool is only expecting a single file as its input, so we need the include_captured_stdio_in_default_outputs = False to remove these from the DefaultInfo provider.

The genrule will also have to provide some output file that something depends on too, otherwise it'll never be run.

[ptarjan]

Thank you. Given that, couldn't that genrule just filter out the stdout and we wouldn't need any upstream changes? I guess I'm saying that either I need all the extra machinery downstream of this rule, or I need this rule to behave like `bazel` expects, but doing `include_captured_stdio_in_default_outputs` doesn't really do either.

[mattem]

I guess it could? The genrule would have to be able to tell which was the "main" output file (expanding all of them via $locations), then based on the value in exit_code either forward that to the output, or cat whatever stdout or stderr is needed and exit with exit_code.

[ptarjan]

I'm working on something like that, and it is somewhat working, but genrules can't output directories (whereas this one can) so I have to concatenate all the files which might be wrong for some node programs. Maybe I can iterate the outputs from the binary and transpose them to be outs of the genrule?

[ptarjan]

I updated to add the same option to rollup_bundle. Again, I don't know how to test bazel from bazel but on the cli it is clean:

$ bazel build //packages/rollup/test/silent/...
INFO: Invocation ID: 1dd3eb6b-fe0d-488c-9ea0-f63019b10b25
DEBUG: /Users/paul.tarjan/robinhood/opensource/rules_nodejs/index.bzl:122:10: WARNING: check_rules_nodejs_version has been removed. This is a no-op, please remove the call.
DEBUG: /Users/paul.tarjan/robinhood/opensource/rules_nodejs/index.bzl:65:14: node_repository attribute not set and no repository named 'nodejs_toolchains' exists; installing default node
DEBUG: /private/var/tmp/_bazel_paul.tarjan/a3791b6c5e41b3bfe90e0b384724f03d/external/bazel_toolchains/rules/rbe_repo/version_check.bzl:68:14:
Current running Bazel is ahead of bazel-toolchains repo. Please update your pin to bazel-toolchains repo in your WORKSPACE file.
DEBUG: /private/var/tmp/_bazel_paul.tarjan/a3791b6c5e41b3bfe90e0b384724f03d/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:125:14: buildkite_config not using checked in configs; Bazel version 5.0.0 was picked/selected but no checked in config was found in map {"0.20.0": ["8.0.0"], "0.21.0": ["8.0.0"], "0.22.0": ["8.0.0", "9.0.0"], "0.23.0": ["8.0.0", "9.0.0"], "0.23.1": ["8.0.0", "9.0.0"], "0.23.2": ["9.0.0"], "0.24.0": ["9.0.0"], "0.24.1": ["9.0.0"], "0.25.0": ["9.0.0"], "0.25.1": ["9.0.0"], "0.25.2": ["9.0.0"], "0.26.0": ["9.0.0"], "0.26.1": ["9.0.0"], "0.27.0": ["9.0.0"], "0.27.1": ["9.0.0"], "0.28.0": ["9.0.0"], "0.28.1": ["9.0.0"], "0.29.0": ["9.0.0"], "0.29.1": ["9.0.0", "10.0.0"], "1.0.0": ["9.0.0", "10.0.0"], "1.0.1": ["10.0.0"], "1.1.0": ["10.0.0"], "1.2.0": ["10.0.0"], "1.2.1": ["10.0.0"], "2.0.0": ["10.0.0"], "2.1.0": ["10.0.0"], "2.1.1": ["10.0.0", "11.0.0"], "2.2.0": ["11.0.0"], "3.0.0": ["11.0.0"], "3.1.0": ["11.0.0"], "3.2.0": ["11.0.0"], "3.3.0": ["11.0.0"], "3.3.1": ["11.0.0"], "3.4.1": ["11.0.0"], "3.5.0": ["11.0.0"], "3.5.1": ["11.0.0"], "3.6.0": ["11.0.0"], "3.7.0": ["11.0.0"], "3.7.1": ["11.0.0"], "3.7.2": ["11.0.0"], "4.0.0": ["11.0.0"]}
DEBUG: /private/var/tmp/_bazel_paul.tarjan/a3791b6c5e41b3bfe90e0b384724f03d/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:125:14: rbe_default not using checked in configs; Bazel version 5.0.0 was picked/selected but no checked in config was found in map {"0.20.0": ["8.0.0"], "0.21.0": ["8.0.0"], "0.22.0": ["8.0.0", "9.0.0"], "0.23.0": ["8.0.0", "9.0.0"], "0.23.1": ["8.0.0", "9.0.0"], "0.23.2": ["9.0.0"], "0.24.0": ["9.0.0"], "0.24.1": ["9.0.0"], "0.25.0": ["9.0.0"], "0.25.1": ["9.0.0"], "0.25.2": ["9.0.0"], "0.26.0": ["9.0.0"], "0.26.1": ["9.0.0"], "0.27.0": ["9.0.0"], "0.27.1": ["9.0.0"], "0.28.0": ["9.0.0"], "0.28.1": ["9.0.0"], "0.29.0": ["9.0.0"], "0.29.1": ["9.0.0", "10.0.0"], "1.0.0": ["9.0.0", "10.0.0"], "1.0.1": ["10.0.0"], "1.1.0": ["10.0.0"], "1.2.0": ["10.0.0"], "1.2.1": ["10.0.0"], "2.0.0": ["10.0.0"], "2.1.0": ["10.0.0"], "2.1.1": ["10.0.0", "11.0.0"], "2.2.0": ["11.0.0"], "3.0.0": ["11.0.0"], "3.1.0": ["11.0.0"], "3.2.0": ["11.0.0"], "3.3.0": ["11.0.0"], "3.3.1": ["11.0.0"], "3.4.1": ["11.0.0"], "3.5.0": ["11.0.0"], "3.5.1": ["11.0.0"], "3.6.0": ["11.0.0"], "3.7.0": ["11.0.0"], "3.7.1": ["11.0.0"], "3.7.2": ["11.0.0"], "4.0.0": ["11.0.0"]}
INFO: Analyzed 2 targets (66 packages loaded, 612 targets configured).
INFO: Found 2 targets...
INFO: Elapsed time: 9.351s, Critical Path: 0.43s
INFO: 4 processes: 2 internal, 2 darwin-sandbox.
INFO: Build completed successfully, 4 total actions

[mattem]

Windows failure looks like it's the same at HEAD.
lgtm

[mattem]

I think this is fine, conceptually if the output is "captured" then I can see the case for it not making it to stdout. No ones opened an issue so far saying that they'd like to see the output of captured outs.

[mattem]

I'll open a tracking issue for if we want to flip this to True in 6.x