Skip to content
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

util.command_output: return stderr, too #3329

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

zsinskri
Copy link
Contributor

Return a namedtuple CommandOutput(stdout, stderr) instead of just stdout from
util.command_ouput, allowing separate access to stdout and stderr.

This change is required by the ffmpeg replaygain backend (GitHub
PullRequest #3056) as ffmpeg's ebur128 filter outputs only to stderr.

Return a namedtuple CommandOutput(stdout, stderr) instead of just stdout from
util.command_ouput, allowing separate access to stdout and stderr.

This change is required by the ffmpeg replaygain backend (GitHub
PullRequest beetbox#3056) as ffmpeg's ebur128 filter outputs only to stderr.
@zsinskri
Copy link
Contributor Author

I'm kind of lost with this test failure. The exception seems to originate in bpd plugin. It tries to start some server on a used address (which of course fails) an then times out while waiting for that non-existing server.

In this branch neither the bpd plugin, nor the space of available addresses has been changed. My best guess is that this is an existing bug that surfaces now due to unhappy timing (and would disappear when rerunning the tests).

Any help?

@zsinskri
Copy link
Contributor Author

Also interestingly the same error occurred in #3056 after rebasing upon this branch, but in a different run.

Here py27-cov failed and py34-test passed while in #3056 only py34-test failed.

@jackwilsdon
Copy link
Member

Looks like an unstable test - I just re-ran the tests for this branch on Travis and it has passed now 😕

@sampsyo
Copy link
Member

sampsyo commented Jul 14, 2019

Indeed; we do have problems with that test. There's more in #3309.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Would you mind adding a quick note to the changelog under a "for developers" heading indicating that this API has changed, in case any external code is depending on it?

beets/util/__init__.py Outdated Show resolved Hide resolved
After commit `30395911 util.command_output: return stderr, too`,
`command_output` returns a tuple of stdout and stderr. Document that change by
adding a changelog entry and add a usage note to `command_output`'s docstring.
@sampsyo sampsyo merged commit 45aa75a into beetbox:master Jul 15, 2019
sampsyo added a commit that referenced this pull request Jul 15, 2019
util.command_output: return stderr, too
sampsyo added a commit that referenced this pull request Jul 15, 2019
@sampsyo
Copy link
Member

sampsyo commented Jul 15, 2019

Looks perfect. Thanks again for separating this out! Merged. ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants