Added check for return code when checking for bzr.

Review Request #11196 — Created Sept. 21, 2020 and submitted

Information

RBTools
master

Reviewers

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
Added check for return code and stderr when running bzr help
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 …

daviddavid

For description and testing done, please make sure these wrap to 70 columns. We also want to make sure that …

daviddavid

I think we need to add some more explanation in the description as well as some comments in the code …

daviddavid

Your summary/description text is OK but please fix capitalization and punctuation. It might also be nice to put the error …

daviddavid

F841 local variable 'streamdata' is assigned to but never used

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

Please make sure that sentences start with capital letters and end in periods. This can also be wrapped a little …

daviddavid

The args to Popen need to be indented 4 more spaces to account for the new text you added.

daviddavid

Instead of decoding to a unicode object, we can just look for the binary text. Please also use single quotes …

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

ruonanjia
Review request changed

Commits:

Summary ID Author
added check for return code when running bzr help
c3fe47e5062e4168308a18a6e0e92c80705d3f70 ruonan
added check for return code when running bzr help
4096df7da070fd70303057ad7be8b4eb392a2f73 ruonan

Diff:

Revision 2 (+14 -4)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
david
  1. 
      
  2. Show all issues

    So the generic "error code 127" might indicate a lot of different things. I think we have a couple options here:

    1. 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.
    2. Keep the check for error 127, but additionally check the stderr output from the command to look for "command not found".
    1. added additional "command not found" check

  3. Show all issues

    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).

  4. Show all issues

    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
    
  5. rbtools/utils/checks.py (Diff revision 3)
     
     
     
     
     
     
     

    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 be p.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)
    
  6. 
      
ruonanjia
Review request changed

Commits:

Summary ID Author
added check for return code when running bzr help
b052f5ce078d8de7b363d79065c15c05b31a5f4c ruonan
added check for return code and stderr when running bzr help
931fe641ac5fc74dd397c36fb3869a2940402a4f ruonan
added check for return code and stderr when running bzr help
bf05dc72ccb71a2f2cabcd24b801b23077db2567 ruonan
added check for return code and stderr when running bzr help
1be08c9d4b8681bc48bb0743941ea9535928ee64 ruonan

Diff:

Revision 4 (+27 -11)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
Review request changed

Commits:

Summary ID Author
added check for return code and stderr when running bzr help
931fe641ac5fc74dd397c36fb3869a2940402a4f ruonan
added check for return code and stderr when running bzr help
bf05dc72ccb71a2f2cabcd24b801b23077db2567 ruonan
added check for return code and stderr when running bzr help
1be08c9d4b8681bc48bb0743941ea9535928ee64 ruonan
added check for return code and stderr when running bzr help
b16eeab341e6f2b8338b4e1093149adef907aeb2 ruonan

Diff:

Revision 5 (+20 -4)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
ruonanjia
ruonanjia
david
  1. 
      
  2. Show all issues

    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)

  3. rbtools/utils/checks.py (Diff revision 6)
     
     
     
     
    Show all issues

    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.

  4. 
      
ruonanjia
ruonanjia
ruonanjia
ruonanjia
ruonanjia
ruonanjia
david
  1. 
      
  2. rbtools/utils/checks.py (Diff revision 7)
     
     
     
     
     
    Show all issues

    The args to Popen need to be indented 4 more spaces to account for the new text you added.

  3. rbtools/utils/checks.py (Diff revision 7)
     
     
    Show all issues

    Instead of decoding to a unicode object, we can just look for the binary text. Please also use single quotes here:

    if b'command not found' in stderr:
    
  4. 
      
ruonanjia
david
  1. Making some tweaks and getting this landed. Thanks!

  2. 
      
david
  1. Ship It!
  2. 
      
ruonanjia
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (aa07d38)
Loading...