• 
      

    Centralize cleartool invocation and fix up windows shell issues.

    Review Request #11298 — Created Nov. 23, 2020 and submitted

    Information

    Review Board
    release-3.0.x

    Reviewers

    The ClearCase SCM backend invokes cleartool a lot, and every time was
    assembling a big Popen call and duplicating error handling. This change
    centralizes all of that into a single method, while also improving
    unicode/bytes compatibility, logging, and console shell handling for
    windows.

    The windows issue is the most interesting one here. Some windows
    applications are marked as requiring a console (as opposed to using
    windows), and invoking them would always pop up a console window,
    regardless of whether input and output was actually possible.
    cleartool is one such executable. An existing workaround had been
    added to use shell=True, but this was only enabled when using Python
    2.7 and Windows 7 (the initial patch was not written with
    future-proofing in mind), and relying on the shell opens up a variety of
    other potential issues, including security concerns. The startupinfo
    flag to Popen allows actually controlling this at a lower level.

    • Ran unit tests.
    • Tested that startupinfo changes to Popen invocation did the correct
      thing for Windows hosts.
    Summary ID
    Centralize cleartool invocation and fix up windows shell issues.
    The ClearCase SCM backend invokes `cleartool` a lot, and every time was assembling a big Popen call and duplicating error handling. This change centralizes all of that into a single method, while also improving unicode/bytes compatibility, logging, and console shell handling for windows. The windows issue is the most interesting one here. Some windows applications are marked as requiring a console (as opposed to using windows), and invoking them would always pop up a console window, regardless of whether input and output was actually possible. `cleartool` is one such executable. An existing workaround had been added to use `shell=True`, but this was only enabled when using Python 2.7 and Windows 7 (the initial patch was not written with future-proofing in mind), and relying on the shell opens up a variety of other potential issues, including security concerns. The `startupinfo` flag to `Popen` allows actually controlling this at a lower level. Testing Done: - Ran unit tests. - Tested that startupinfo changes to Popen invocation did the correct thing for Windows hosts.
    c5aa7241d5998e1c04201940c8194bcae3f6ebbd
    Description From Last Updated

    "Returns" goes before "Raises".

    chipx86chipx86

    F821 undefined name 'six'

    reviewbotreviewbot

    Review Bot has its complaint, but you can also just have this be: if sys.version_info[:2] >= (3, 6): ... which …

    chipx86chipx86

    "Returns" before "Raises".

    chipx86chipx86

    "Returns" before "Raises".

    chipx86chipx86

    "Returns" before "Raises". Same with others below.

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

    flake8

    chipx86
    1. 
        
    2. reviewboard/scmtools/clearcase.py (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      "Returns" goes before "Raises".

    3. reviewboard/scmtools/clearcase.py (Diff revision 1)
       
       
      Show all issues

      Review Bot has its complaint, but you can also just have this be:

      if sys.version_info[:2] >= (3, 6):
        ...
      

      which is probably more clear.

    4. reviewboard/scmtools/clearcase.py (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      "Returns" before "Raises".

    5. reviewboard/scmtools/clearcase.py (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      "Returns" before "Raises".

    6. reviewboard/scmtools/clearcase.py (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      "Returns" before "Raises".

      Same with others below.

    7. 
        
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (6ee4835)