• 
      

    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

    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

    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

    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:
    Completed
    Change Summary:
    Pushed to master (aa07d38)