New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

popen: Do not suppress stderr #3099

Merged
merged 1 commit into from Sep 18, 2017

Conversation

Projects
None yet
4 participants
@sjackman
Contributor

sjackman commented Aug 28, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Suppressing stderr makes troubleshooting failed commands difficult.

@sjackman sjackman referenced this pull request Aug 28, 2017

Merged

trf 4.09 #6214

8 of 8 tasks complete
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 28, 2017

Contributor

One test is failing test/dev-cmd/bottle_spec.rb:2 # brew bottle builds a bottle for the given Formula

expected block to not output to stderr, but output "Error: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\n"

https://travis-ci.org/Homebrew/brew/jobs/269276988#L1745

I'll look into this failure.

Contributor

sjackman commented Aug 28, 2017

One test is failing test/dev-cmd/bottle_spec.rb:2 # brew bottle builds a bottle for the given Formula

expected block to not output to stderr, but output "Error: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\n"

https://travis-ci.org/Homebrew/brew/jobs/269276988#L1745

I'll look into this failure.

@JCount

This comment has been minimized.

Show comment
Hide comment
@JCount

JCount Aug 29, 2017

Member

Would it make sense to guard this with a check to see whether debug is enabled, rather than unilaterally disabling it? Otherwise, this seems to have the potential to dump a lot of confusing output into the average user's stderr, potentially complicating matters.

(If something like this is already in place, feel free to disregard.)

Member

JCount commented Aug 29, 2017

Would it make sense to guard this with a check to see whether debug is enabled, rather than unilaterally disabling it? Otherwise, this seems to have the potential to dump a lot of confusing output into the average user's stderr, potentially complicating matters.

(If something like this is already in place, feel free to disregard.)

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 29, 2017

Contributor

I've addressed the above test failure by silencing the stderr of fgrep. The -s option to silence stderr is portable to both BSD grep and GNU grep.

Contributor

sjackman commented Aug 29, 2017

I've addressed the above test failure by silencing the stderr of fgrep. The -s option to silence stderr is portable to both BSD grep and GNU grep.

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 29, 2017

Contributor

@JCount I feel that ignoring error messages wholesale is a dangerous default. When a particular tool is expected to output error message (like fgrep which caused a test to fail), its stderr should be selectively silenced.

Contributor

sjackman commented Aug 29, 2017

@JCount I feel that ignoring error messages wholesale is a dangerous default. When a particular tool is expected to output error message (like fgrep which caused a test to fail), its stderr should be selectively silenced.

@JCount

This comment has been minimized.

Show comment
Hide comment
@JCount

JCount Aug 29, 2017

Member

I was advocating for selective silencing, not against it. Using the developer mode env or the debug flag were merely a basic example to try to clarify what I meant.

Member

JCount commented Aug 29, 2017

I was advocating for selective silencing, not against it. Using the developer mode env or the debug flag were merely a basic example to try to clarify what I meant.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Aug 29, 2017

Member

I'm not sure about making this change. This is essentially a public formula API at this stage and will make formulae start outputting things they didn't before. I think it's a possibility if, as @JCount suggests, it's guarded on ARGV.debug? and/or ARGV.verbose?. CC @ilovezfs for some thoughts here, too.

Member

MikeMcQuaid commented Aug 29, 2017

I'm not sure about making this change. This is essentially a public formula API at this stage and will make formulae start outputting things they didn't before. I think it's a possibility if, as @JCount suggests, it's guarded on ARGV.debug? and/or ARGV.verbose?. CC @ilovezfs for some thoughts here, too.

@sjackman sjackman changed the title from popen: Do no suppress stderr to popen: Do not suppress stderr Aug 30, 2017

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 30, 2017

Contributor

I've modified the logic to be

$stderr.reopen("/dev/null", "w") unless ARGV.debug? || ARGV.verbose?

I'm still happy to receive comments and change it as desired.

Contributor

sjackman commented Aug 30, 2017

I've modified the logic to be

$stderr.reopen("/dev/null", "w") unless ARGV.debug? || ARGV.verbose?

I'm still happy to receive comments and change it as desired.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Aug 30, 2017

Contributor

So if we do foo = Utils.popen_read("blah") does foo get set to some mixture of stdout and stderr? If so, that is not good.

Contributor

ilovezfs commented Aug 30, 2017

So if we do foo = Utils.popen_read("blah") does foo get set to some mixture of stdout and stderr? If so, that is not good.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs
Contributor

ilovezfs commented Aug 30, 2017

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 30, 2017

Contributor

So if we do foo = Utils.popen_read("blah") does foo get set to some mixture of stdout and stderr?

No, the $stderr of blah does not go to foo. $stderr of the child process goes to $stderr of the parent process, which is typically the console for an interactive brew process, or to a file when the $stderr of brew is being redirected to file.

Contributor

sjackman commented Aug 30, 2017

So if we do foo = Utils.popen_read("blah") does foo get set to some mixture of stdout and stderr?

No, the $stderr of blah does not go to foo. $stderr of the child process goes to $stderr of the parent process, which is typically the console for an interactive brew process, or to a file when the $stderr of brew is being redirected to file.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Aug 30, 2017

Contributor

Why did the fgrep need to be changed if this doesn't affect stdout?

Contributor

ilovezfs commented Aug 30, 2017

Why did the fgrep need to be changed if this doesn't affect stdout?

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 30, 2017

Contributor
expected block to not output to stderr, but output "Error: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\n"

#3099 (comment)

It caused a brew tests failure that checked that the output of $stderr was empty when installing a bottle with a broken symlink.

Contributor

sjackman commented Aug 30, 2017

expected block to not output to stderr, but output "Error: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\nError: No such file or directory - /usr/bin/fgrep\n"

#3099 (comment)

It caused a brew tests failure that checked that the output of $stderr was empty when installing a bottle with a broken symlink.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Aug 31, 2017

Contributor

codecov/patch — 0% of diff hit (target 50.49%)

@sjackman do you mind trying to raise that percent?

Contributor

ilovezfs commented Aug 31, 2017

codecov/patch — 0% of diff hit (target 50.49%)

@sjackman do you mind trying to raise that percent?

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 31, 2017

Contributor

Both lines of this diff are hit by test/dev-cmd/bottle_spec.rb:20. I know because that test failed without adding the -s option to grep.
https://travis-ci.org/Homebrew/brew/jobs/269276988#L1745
#3099 (comment)

Doesn't that mean that 100% of the diff is covered, or have I misunderstood?

Contributor

sjackman commented Aug 31, 2017

Both lines of this diff are hit by test/dev-cmd/bottle_spec.rb:20. I know because that test failed without adding the -s option to grep.
https://travis-ci.org/Homebrew/brew/jobs/269276988#L1745
#3099 (comment)

Doesn't that mean that 100% of the diff is covered, or have I misunderstood?

@ilovezfs ilovezfs closed this Aug 31, 2017

@ilovezfs ilovezfs reopened this Aug 31, 2017

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Aug 31, 2017

Contributor

I know because that test failed without adding the -s option to grep.

was that before or after you scoped it to debug and verbose?

Contributor

ilovezfs commented Aug 31, 2017

I know because that test failed without adding the -s option to grep.

was that before or after you scoped it to debug and verbose?

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 31, 2017

Contributor

This line is definitely being run.

Utils.popen_read("/usr/bin/fgrep", "-s", recursive_fgrep_args, string, to_s) do |io|

https://github.com/Homebrew/brew/pull/3099/files#diff-26acfe09641ad4e7cf4f22179625981eR99

Contributor

sjackman commented Aug 31, 2017

This line is definitely being run.

Utils.popen_read("/usr/bin/fgrep", "-s", recursive_fgrep_args, string, to_s) do |io|

https://github.com/Homebrew/brew/pull/3099/files#diff-26acfe09641ad4e7cf4f22179625981eR99

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Aug 31, 2017

Contributor
$stderr.reopen("/dev/null", "w") if !ARGV.debug? && !ARGV.verbose?

https://github.com/Homebrew/brew/pull/3099/files#diff-83e8c13ed518191365f4edaa7339293dR16
This line is run too. I believe ARGV.debug? and ARGV.verbose? are both false when Jenkins runs, so only the one code path is being tested, where !ARGV.debug? && !ARGV.verbose? is false and $stderr is closed.

Contributor

sjackman commented Aug 31, 2017

$stderr.reopen("/dev/null", "w") if !ARGV.debug? && !ARGV.verbose?

https://github.com/Homebrew/brew/pull/3099/files#diff-83e8c13ed518191365f4edaa7339293dR16
This line is run too. I believe ARGV.debug? and ARGV.verbose? are both false when Jenkins runs, so only the one code path is being tested, where !ARGV.debug? && !ARGV.verbose? is false and $stderr is closed.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 3, 2017

Member

@sjackman It's unclear to me: are you saying the CI fails without that change being made?

Member

MikeMcQuaid commented Sep 3, 2017

@sjackman It's unclear to me: are you saying the CI fails without that change being made?

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Sep 5, 2017

Contributor

CI passes. The fgrep -s change is required if brew tests is run with either ARGV.debug? or ARGV.verbose? enabled, but that doesn't seem to be the case for CI brew test-bot.

Contributor

sjackman commented Sep 5, 2017

CI passes. The fgrep -s change is required if brew tests is run with either ARGV.debug? or ARGV.verbose? enabled, but that doesn't seem to be the case for CI brew test-bot.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 5, 2017

Member

The fgrep -s change is required if brew tests is run with either ARGV.debug? or ARGV.verbose? enabled, but that doesn't seem to be the case for CI brew test-bot.

🆒, you can remove that change then, thanks.

Member

MikeMcQuaid commented Sep 5, 2017

The fgrep -s change is required if brew tests is run with either ARGV.debug? or ARGV.verbose? enabled, but that doesn't seem to be the case for CI brew test-bot.

🆒, you can remove that change then, thanks.

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Sep 5, 2017

Contributor

Done!

Contributor

sjackman commented Sep 5, 2017

Done!

@MikeMcQuaid MikeMcQuaid merged commit 6bde1de into Homebrew:master Sep 18, 2017

1 of 3 checks passed

codecov/patch 0% of diff hit (target 49.92%)
Details
codecov/project 49.56% (-0.37%) compared to 4ef0322
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Sep 18, 2017

Member

Thanks again @sjackman!

Member

MikeMcQuaid commented Sep 18, 2017

Thanks again @sjackman!

@sjackman sjackman deleted the sjackman:popen_read_stderr branch Sep 19, 2017

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Sep 19, 2017

Contributor

Thanks for merging, Mike!

Contributor

sjackman commented Sep 19, 2017

Thanks for merging, Mike!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.