fix(vendor-gems): redirect bundler stdout to stderr #11479
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gibfahn
commented
Jun 2, 2021
Library/Homebrew/utils/gems.rb
Outdated
Comment on lines
147
to
152
| unless system_redirect_stdout_to_stderr bundle, "install" | ||
| message = <<~EOS | ||
| failed to run `#{bundle} install`! | ||
| EOS |
There was a problem hiding this comment.
I erred on the side of making the minimal changes here, but I believe if we changed this to:
Suggested change
| unless system_redirect_stdout_to_stderr bundle, "install" | |
| message = <<~EOS | |
| failed to run `#{bundle} install`! | |
| EOS | |
| begin | |
| safe_system_redirect_stdout_to_stderr bundle, "install" | |
| rescue ErrorDuringExecution => | |
| message = <<~EOS | |
| failed to run `#{bundle} install`! | |
| EOS |
then we could get rid of the system_redirect_stdout_to_stderr and just use the safe_ one everywhere. Not sure if this is preferable.
gibfahn
force-pushed
the
redirect_stdout
branch
2 times, most recently
from
b514c8f
to
11b3266
Compare
MikeMcQuaid
reviewed
Jun 2, 2021
| @@ -40,7 +40,7 @@ def vendor_gems | |||
| end | |||
|
|
|||
| ohai "bundle install --standalone" | |||
| safe_system "bundle", "install", "--standalone" | |||
| safe_system_redirect_stdout_to_stderr "bundle", "install", "--standalone" | |||
There was a problem hiding this comment.
I'm not sure about using these methods just to be used in a single place each (unless you have other ideals). I'd be tempted to just inline the methods?
There was a problem hiding this comment.
Yeah agreed that two single-use methods are overkill. I went with combining into one method, that is now used in both places. I suspect there are other invocations of bundler or other tools that would need this, so having a function for it seems probably worth it, what do you think?
When running brew commands and interpreting the output, e.g. running `brew livecheck --json`, it's necessary to stop other programs Homebrew happens to execute from writing logging output to stdout. Most programs don't do this, but `bundle install` does seem to. To reproduce the issue you can run: ```shell git -C "$(brew --prefix)" clean -ffdx Library/Homebrew/vendor stdout=$(HOMEBREW_FORCE_VENDOR_RUBY=1 brew livecheck --newer-only --json --cask $(brew --repo homebrew/cask)/Casks/grid.rb) echo "^^^ was stderr, >>> is stdout: $stdout" ``` If you run it without this change it will print a bunch of output like this to the stdout before printing out the livecheck JSON output: ```text Using bundler 1.17.3 Fetching byebug 11.1.3 Fetching coderay 1.1.3 Installing byebug 11.1.3 with native extensions Installing coderay 1.1.3 Fetching colorize 0.8.1 Installing colorize 0.8.1 [ # Contents of the JSON block. ] ``` With this change the stdout from `bundle install` will be redirected to brew's stderr, meaning only the JSON goes to stdout, and the rest goes to stderr.
MikeMcQuaid
approved these changes
Jun 3, 2021
|
Looks good, nice work here @gibfahn! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?I didn't add a test for this as I wasn't sure we wanted to be running a full bundle install for a single test, let me know if there's a good way to test this.
Commits (oldest to newest)
11b3266fd fix(vendor-gems): redirect bundler stdout to stderr
When running brew commands and interpreting the output, e.g. running
brew livecheck --json, it's necessary to stop other programs Homebrewhappens to execute from writing logging output to stdout. Most programs
don't do this, but
bundle installdoes seem to.To reproduce the issue you can run:
If you run it without this change it will print a bunch of output like
this to the stdout before printing out the livecheck JSON output:
With this change the stdout from
bundle installwill be redirected tobrew's stderr, meaning only the JSON goes to stdout, and the rest goes
to stderr.