Added check for return code when checking for bzr.
Review Request #11196 — Created Sept. 21, 2020 and submitted
When pyenv is in use and the needed executable is only available in a
different Python version, pyenv intercepts the call and will print
an error/return 127.When running bzr help under these conditions, we see this:
pyenv: bzr: command not found
The `bzr' command exists in these Python versions:
2.7.14
Tested 'rbt post' manually in my local integrations repo with the changes.
Summary | ID | Author |
---|---|---|
34fcb7055bfe916b3fd74c45fa1bb18a169572c4 | ruonan |
Description | From | Last Updated |
---|---|---|
So the generic "error code 127" might indicate a lot of different things. I think we have a couple options … |
david | |
For description and testing done, please make sure these wrap to 70 columns. We also want to make sure that … |
david | |
I think we need to add some more explanation in the description as well as some comments in the code … |
david | |
Your summary/description text is OK but please fix capitalization and punctuation. It might also be nice to put the error … |
david | |
F841 local variable 'streamdata' is assigned to but never used |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (90 > 79 characters) |
reviewbot | |
E501 line too long (86 > 79 characters) |
reviewbot | |
E501 line too long (90 > 79 characters) |
reviewbot | |
E501 line too long (86 > 79 characters) |
reviewbot | |
Please make sure that sentences start with capital letters and end in periods. This can also be wrapped a little … |
david | |
The args to Popen need to be indented 4 more spaces to account for the new text you added. |
david | |
Instead of decoding to a unicode object, we can just look for the binary text. Please also use single quotes … |
david |
- Commits:
-
Summary ID Author c3fe47e5062e4168308a18a6e0e92c80705d3f70 ruonan 4096df7da070fd70303057ad7be8b4eb392a2f73 ruonan - Diff:
-
Revision 2 (+14 -4)
- Commits:
-
Summary ID Author 4096df7da070fd70303057ad7be8b4eb392a2f73 ruonan b052f5ce078d8de7b363d79065c15c05b31a5f4c ruonan - Diff:
-
Revision 3 (+14 -4)
Checks run (2 succeeded)
-
-
So the generic "error code 127" might indicate a lot of different things. I think we have a couple options here:
- Treat any positive error code as an error. Theoretically this should be fine, but we would need to look at the various callers of
check_install
and verify that none of the things we're executing can return non-zero errors when it's actually a success case. - Keep the check for error 127, but additionally check the stderr output from the command to look for "command not found".
- Treat any positive error code as an error. Theoretically this should be fine, but we would need to look at the various callers of
-
For description and testing done, please make sure these wrap to 70 columns. We also want to make sure that capitalization and punctuation is correct and consistent. We always use either "RBTools" or "rbtools" (in most cases the former is best when talking about it as a generic product and the latter when referring to the repo or modules in the code).
-
I think we need to add some more explanation in the description as well as some comments in the code explaining what's going on--I had to go back to your messages in Slack to realize what the problem was because the idea that
bzr
was simultaneously not installed and also supposedly executing with an error was very confusing.Let's explain that when pyenv is in use, and the needed executable is only available in a different Python version, pyenv intercepts the call and will print an error/return 127. Please include this example output of the error in the description:
pyenv: bzr: command not found The `bzr' command exists in these Python versions: 2.7.14
-
A few comments on this code, just FYI since I think it'll change a fair bit if you're adding the check for the output.
First, you had changed the
communicate
line to avoid storing the output, but it still has a[0]
at the end--that could be removed and just have it bep.communicate()
.Second, when we have blocks, we like to put a blank line around them to make the code just a bit easier to read. Like so:
rc = p.returncode if rc === 127: ...
That said, any time there's a "if X return True else return False", it can be simplified into a one-liner:
return (p.returncode != 127)
- Commits:
-
Summary ID Author b052f5ce078d8de7b363d79065c15c05b31a5f4c ruonan 931fe641ac5fc74dd397c36fb3869a2940402a4f ruonan bf05dc72ccb71a2f2cabcd24b801b23077db2567 ruonan 1be08c9d4b8681bc48bb0743941ea9535928ee64 ruonan - Diff:
-
Revision 4 (+27 -11)
- Commits:
-
Summary ID Author 931fe641ac5fc74dd397c36fb3869a2940402a4f ruonan bf05dc72ccb71a2f2cabcd24b801b23077db2567 ruonan 1be08c9d4b8681bc48bb0743941ea9535928ee64 ruonan b16eeab341e6f2b8338b4e1093149adef907aeb2 ruonan - Diff:
-
Revision 5 (+20 -4)
- Commits:
-
Summary ID Author b16eeab341e6f2b8338b4e1093149adef907aeb2 ruonan 419d1a61a4634080d28b1e5b3676098866584505 ruonan - Diff:
-
Revision 6 (+22 -4)
Checks run (2 succeeded)
- Summary:
-
added check for return code when running bzr helpadded check for return code when checking for bzr
- Description:
-
~ 'bzr help' runs and returns without OSError in pyenv even when bzr does not exist. Rbtools assumes that it ran properly and thinks that it is a bazaar repo. This additional check checks return code in pyenv so in the case that bzr does not exist, rbtools does not label it as a bazaar repo.
~ when pyenv is in use, and the needed executable is only available in a different Python version, pyenv intercepts the call and will print an error/return 127
+ + When running bzr help under these conditions, we see this:
+ pyenv: bzr: command not found + + The `bzr' command exists in these Python versions:
+ 2.7.14
- Description:
-
~ when pyenv is in use, and the needed executable is only available in a different Python version, pyenv intercepts the call and will print an error/return 127
~ when pyenv is in use, and the needed executable is only available in a
+ different Python version, pyenv intercepts the call and will print + an error/return 127 When running bzr help under these conditions, we see this:
pyenv: bzr: command not found The `bzr' command exists in these Python versions:
2.7.14
-
-
Your summary/description text is OK but please fix capitalization and punctuation. It might also be nice to put the error output there in a code block (triple backticks around it)
-
Please make sure that sentences start with capital letters and end in periods. This can also be wrapped a little bit tighter to the textwidth.
- Description:
-
~ when pyenv is in use, and the needed executable is only available in a
~ When pyenv is in use, and the needed executable is only available in a
different Python version, pyenv intercepts the call and will print ~ an error/return 127 ~ an error/return 127. When running bzr help under these conditions, we see this:
~ pyenv: bzr: command not found ~ pyenv: bzr: command not found
The `bzr' command exists in these Python versions:
2.7.14
- Description:
-
~ When pyenv is in use, and the needed executable is only available in a
~ When pyenv is in use and the needed executable is only available in a
different Python version, pyenv intercepts the call and will print an error/return 127. When running bzr help under these conditions, we see this:
pyenv: bzr: command not found
The `bzr' command exists in these Python versions:
2.7.14
- Commits:
-
Summary ID Author 419d1a61a4634080d28b1e5b3676098866584505 ruonan 419d1a61a4634080d28b1e5b3676098866584505 ruonan cd3f516d5bfbc81933ec5a26ce2b6529a76a71ac ruonan - Diff:
-
Revision 7 (+25 -7)
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 419d1a61a4634080d28b1e5b3676098866584505 ruonan cd3f516d5bfbc81933ec5a26ce2b6529a76a71ac ruonan 34fcb7055bfe916b3fd74c45fa1bb18a169572c4 ruonan - Diff:
-
Revision 8 (+38 -10)