Summary

Centralize cleartool invocation and fix up windows shell issues.

Review Request #11298 — Created Nov. 23, 2020 and submitted Nov. 27, 2020, 2:22 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Commits

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

Summary

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

Issues

Description	From	Last Updated
"Returns" goes before "Raises".	chipx86	Nov. 24, 2020, 11:46 a.m.
F821 undefined name 'six'	reviewbot	Nov. 24, 2020, 11:46 a.m.
Review Bot has its complaint, but you can also just have this be: if sys.version_info[:2] >= (3, 6): ... which …	chipx86	Nov. 24, 2020, 11:46 a.m.
"Returns" before "Raises".	chipx86	Nov. 24, 2020, 11:46 a.m.
"Returns" before "Raises".	chipx86	Nov. 24, 2020, 11:47 a.m.
"Returns" before "Raises". Same with others below.	chipx86	Nov. 24, 2020, 11:47 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'six'
```

reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Returns" goes before "Raises".
```

reviewboard/scmtools/clearcase.py (Diff revision 1)

The issue has been resolved. 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.

reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Returns" before "Raises".
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Returns" before "Raises".
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Returns" before "Raises".
Same with others below.
```

Testing Done:

		Ran unit tests.
~		startupinfo changes haven't yet been tested because I don't have a windows environment set up at the moment, but I'll make sure to verify its correctness before landing this.
	~	Tested that startupinfo changes to Popen invocation did the correct thing for Windows hosts.

Commits:

	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. - startupinfo changes haven't yet been tested because I don't have a windows environment set up at the moment, but I'll make sure to verify its correctness before landing this.	c62c899160d09387a1da2487d61a49510f31cb1a
	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

Diff:

Revision 2 (+254 -204)

Show changes

reviewboard/scmtools/clearcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (6ee4835)