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

capture stderr as well as stdout when diagnosing #126

Merged
merged 1 commit into from
Feb 9, 2015

Conversation

lepistone
Copy link
Member

This fixes #125 because in some Ubuntu systems
lsb_release outputs to stderr as well as stdout. This message will be
printed to console, not logged nor shown among server infomation. That
way the message is lost and only pollutes unit tests output.

This fixes OCA#125 because in some Ubuntu systems
lsb_release outputs to stderr as well as stdout. This message will be
printed to console, not logged nor shown among server infomation. That
way the message is lost and only pollutes unit tests output.
@pedrobaeza
Copy link
Member

Why not subprocess.STDERR?

@lepistone
Copy link
Member Author

@pedrobaeza I gave a look at the doc at https://docs.python.org/2/library/subprocess.html and subprocess.STDERR it is not a choice you can make.

In the line after, with communicate()[0] we return stdout. If we did stderr=PIPE then stderr would go to communicate()[1] but then we wouldn't use it, so it would be discarded.

What I did is to return stderr together with stdout so that is displayed by the module. The other option would be to throw it away with 2> /dev/null.

@pedrobaeza
Copy link
Member

OK, thanks for the explanation

👍

@yvaucher
Copy link
Member

yvaucher commented Feb 6, 2015

👍

1 similar comment
@nbessi
Copy link

nbessi commented Feb 9, 2015

👍

nbessi added a commit that referenced this pull request Feb 9, 2015
capture stderr as well as stdout when diagnosing
@nbessi nbessi merged commit d0f1e0e into OCA:8.0 Feb 9, 2015
@lepistone lepistone deleted the fix-lsb-error-to-console branch February 9, 2015 08:52
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.

4 participants