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)