Summary

Work around OS X 10.11 bug with check_install()

Review Request #8058 — Created March 15, 2016 and submitted April 29, 2016, 10:30 a.m.

Information

Owner

elatt

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

28be9b8...

Reviewers

Groups

rbtools

People

Description

This has been fixed upstream (http://bugs.python.org/issue26083) but who
knows when Apple will update their version of Python they distribute so
this change simply works around the issue by treating a ValueError to be
the same as an OSError. This fix seems smaller/safer then switching to 
the subprocess32 package as it adds another external dependency an the 
documentation states that it hasn't been tested on Windows.

Testing Done

Before my fix, when running rbt post, I get:
  ValueError: insecure string pickle
(I don't have tfs installed)

After my fix, running rbt post works fine.

Tool: Pyflakes
Processed Files:
    rbtools/utils/checks.py
    contrib/internal/release.py
    rbtools/commands/alias.py
    rbtools/commands/main.py
    rbtools/utils/console.py
    rbtools/clients/perforce.py
    rbtools/hooks/common.py
    setup.py
    rbtools/utils/process.py
    rbtools/utils/aliases.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/checks.py
    contrib/internal/release.py
    rbtools/commands/alias.py
    rbtools/commands/main.py
    rbtools/utils/console.py
    rbtools/clients/perforce.py
    rbtools/hooks/common.py
    setup.py
    rbtools/utils/process.py
    rbtools/utils/aliases.py

Hmm. It really scares me to switch to a third party library that advertises that it "has not been tested on Windows."

elatt March 15, 2016, 3:40 p.m.

Fair enough. I was just going off the fact that Gregory P. Smith is both the maintainer of the stdlib subprocess module and the author of the subprocess32 package and recommended the switch but I'm not a Windows user so I don't know how robust it is.

I can attempt to make the fix more surgical and continue using the stdlib subprocess module by simply catching either the bogus ValueError or a CalledProcessError in the section where rbt tries to detect all the SCM tools that are installed.

chipx86 March 15, 2016, 3:44 p.m.

If we can fix this without introducing a new dependency, I'd definitely prefer that. We know that the stdlib one works on Windows, and won't have to worry about any surprises there.

Change Summary:

No longer require subprocess32.

Summary:

-	Switch from using stdlib subprocess module to external subprocess32 package
+	Work around OS X 10.11 bug with check_install()

Description:

~		The subprocess32 package is better maintained on the Python 2.x branches
~		and specifically fixes a bug seen when running rbt on OS X 10.11.
~		See: http://bugs.python.org/issue26083
	~	This has been fixed upstream (http://bugs.python.org/issue26083) but who
	~	knows when Apple will update their version of Python they distribute so
	~	this change simply works around the issue by treating a ValueError to be
	+	the same as an OSError. This fix seems smaller/safer then switching to
	+	the subprocess32 package as it adds another external dependency an the
	+	documentation states that it hasn't been tested on Windows.

Commit:

-	25b0427627dc0453eb8a6780962e891c26d80d08
+	28be9b893efa7e8de53ab0eb82c5656f6ceeaf8e

Diff:

Revision 2 (+5 -1)

Show changes

rbtools/utils/checks.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/checks.py



Tool: Pyflakes
Processed Files:
    rbtools/utils/checks.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (5865146)