Added check for return code when checking for bzr.

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

ruonanjia
RBTools
master
rbtools, students

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 Author
Added check for return code and stderr when running bzr help
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 Author
-
added check for return code when running bzr help
ruonan
+
added check for return code when running bzr help
ruonan

Diff:

Revision 2 (+14 -4)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
david
  1. 
      
  2. 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. 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. 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 Author
-
added check for return code when running bzr help
ruonan
+
added check for return code and stderr when running bzr help
ruonan
+
added check for return code and stderr when running bzr help
ruonan
+
added check for return code and stderr when running bzr help
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 Author
-
added check for return code and stderr when running bzr help
ruonan
-
added check for return code and stderr when running bzr help
ruonan
-
added check for return code and stderr when running bzr help
ruonan
+
added check for return code and stderr when running bzr help
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. 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)
     
     
     
     

    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)
     
     
     
     
     

    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)
     
     

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