vpython: Add Python interpreter handling package.

vpython: Add Python interpreter handling package.

Add a package to "vpython" to locate and manipulate the system Python
interpreter. This includes location, version resolution, and invocation
functionality.

This package includes opportunistic tests, which will run and assert
based on which interpreter(s) are currently installed on the system.

BUG=chromium:691818
TEST=unit

Review-Url: https://codereview.chromium.org/2701073002
Committed: https://github.com/luci/luci-go/commit/7e123ef96d5a134b8864271e2db68bcbad3e0036

[dnj, 2017-02-17 23:13:54]

Description was changed from

==========
Updated patchset dependency

vpython: Add Python interpreter handling package.

Add a package to "vpython" to locate and manipulate the system Python
interpreter. This includes location, version resolution, and invocation
functionality.

This package includes opportunistic tests, which will run and assert
based on which interpreter(s) are currently installed on the system.

BUG=chromium:691818
TEST=unit
==========

to

==========
vpython: Add Python interpreter handling package.

Add a package to "vpython" to locate and manipulate the system Python
interpreter. This includes location, version resolution, and invocation
functionality.

This package includes opportunistic tests, which will run and assert
based on which interpreter(s) are currently installed on the system.

BUG=chromium:691818
TEST=unit
==========

[dnj, 2017-02-17 23:17:35]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2017-02-17 23:17:36]

PTAL! This is part of a larger "vpython" CL set for the tool. More information
can be found in the bug, or in the linked design doc:
https://docs.google.com/document/d/1GKN7rtW6-006RbG_Ryh6vr4TpUxSxJlPSCKFLq2nO...

Total CL set is:
https://codereview.chromium.org/2703463006/ (README, protobufs)
https://codereview.chromium.org/2703793003/ (filesystem)
https://codereview.chromium.org/2700273002/ (wheel)
https://codereview.chromium.org/2705623003/ (environment spec)
https://codereview.chromium.org/2701073002/ (Python interpreter management)
https://codereview.chromium.org/2699063004/ (main VirtualEnv package)
https://codereview.chromium.org/2699223002/ (CIPD support)
https://codereview.chromium.org/2702873002/ (package entry point)
https://codereview.chromium.org/2705593003/ (main application)
https://chromium-review.googlesource.com/c/444040/ (Infra entry point)

[iannucci, 2017-02-21 10:08:17]

lgtm

https://codereview.chromium.org/2701073002/diff/1/vpython/python/find.go
File vpython/python/find.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/find.go#newc...
vpython/python/find.go:53: return nil, errors.Annotate(err).Reason("failed to
get veriso for: %(interp)q").
versio

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter.go
File vpython/python/interpreter.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:22: // Interpreter is a configured Python
interpreter.
maybe

Interpreter is a runnable Python interpreter binary found on the system. It can
be queried for it's version. Obtain one using the Find method.

The current comment isn't useful.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:24: // Python is the path to the Python
interpreter.
absolute?

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:27: // WorkDir is the working directory to use.
use for what?

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:30: // ConnectSTDIN, if true, says that STDIN
should also be connected.
ConnectSTDIN will cause stdin to be passed through. Otherwise it will be closed.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:34: // it from local system modification.
Like what? "Local system modification"? From the code, I would say this should
be something like


Isolated should be set to avoid picking up environmental queues (e.g.
PYTHONPATH, site.py), and avoid modifying the local filesystem (e.g. writing
.pyo files)

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:42: runner runnerFunc
lets prefix all testing members with 'testing'

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:47: cachedVersionMu sync.Mutex
sync.Once?

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:75: // Run runs the configured Interpreter with
the supplied arguments.
Run executes the python interpreter with the supplied arguments.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:84: return errors.New("a Python interpreter must
be supplied")
please avoid passive voice everywhere

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:147: rf := i.getRunnerFunc()
why not `i.run(cmd, capture)`? I think that might be clearer than returning a
function which you immediately invoke once.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/version.go
File vpython/python/version.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/version.go#n...
vpython/python/version.go:45: var err error
named return values would simplify this code I think

[dnj, 2017-02-21 23:21:35]

https://codereview.chromium.org/2701073002/diff/1/vpython/python/find.go
File vpython/python/find.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/find.go#newc...
vpython/python/find.go:53: return nil, errors.Annotate(err).Reason("failed to
get veriso for: %(interp)q").
On 2017/02/21 10:08:16, iannucci wrote:
> versio

Vers I/O

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter.go
File vpython/python/interpreter.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:22: // Interpreter is a configured Python
interpreter.
On 2017/02/21 10:08:16, iannucci wrote:
> maybe
> 
> Interpreter is a runnable Python interpreter binary found on the system. It
can
> be queried for it's version. Obtain one using the Find method.
> 
> The current comment isn't useful.

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:24: // Python is the path to the Python
interpreter.
On 2017/02/21 10:08:16, iannucci wrote:
> absolute?

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:27: // WorkDir is the working directory to use.
On 2017/02/21 10:08:16, iannucci wrote:
> use for what?

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:30: // ConnectSTDIN, if true, says that STDIN
should also be connected.
On 2017/02/21 10:08:16, iannucci wrote:
> ConnectSTDIN will cause stdin to be passed through. Otherwise it will be
closed.

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:34: // it from local system modification.
On 2017/02/21 10:08:16, iannucci wrote:
> Like what? "Local system modification"? From the code, I would say this should
> be something like
> 
> 
> Isolated should be set to avoid picking up environmental queues (e.g.
> PYTHONPATH, site.py), and avoid modifying the local filesystem (e.g. writing
> .pyo files)

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:42: runner runnerFunc
On 2017/02/21 10:08:16, iannucci wrote:
> lets prefix all testing members with 'testing'

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:47: cachedVersionMu sync.Mutex
On 2017/02/21 10:08:16, iannucci wrote:
> sync.Once?

Because there can be an error return code, I would prefer to use a Mutex.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:75: // Run runs the configured Interpreter with
the supplied arguments.
On 2017/02/21 10:08:16, iannucci wrote:
> Run executes the python interpreter with the supplied arguments.

I split this off into "Command()" (returns a bound command) and "Run()" (which
runs the configured command).

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:84: return errors.New("a Python interpreter must
be supplied")
On 2017/02/21 10:08:16, iannucci wrote:
> please avoid passive voice everywhere

Done.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/interpreter....
vpython/python/interpreter.go:147: rf := i.getRunnerFunc()
On 2017/02/21 10:08:16, iannucci wrote:
> why not `i.run(cmd, capture)`? I think that might be clearer than returning a
> function which you immediately invoke once.

Changed to a Command struct.

https://codereview.chromium.org/2701073002/diff/1/vpython/python/version.go
File vpython/python/version.go (right):

https://codereview.chromium.org/2701073002/diff/1/vpython/python/version.go#n...
vpython/python/version.go:45: var err error
On 2017/02/21 10:08:16, iannucci wrote:
> named return values would simplify this code I think

I dunno, i don't really mind it as-is.

[dnj, 2017-02-22 07:44:17]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-22 07:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2701073002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-22 07:44:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 07:49:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487749457267070,
"parent_rev": "fa008f62168a77f43978a9da9dc6aa22de82bbc5", "commit_rev":
"7e123ef96d5a134b8864271e2db68bcbad3e0036"}

[commit-bot: I haz the power, 2017-02-22 07:49:42]

Description was changed from

==========
vpython: Add Python interpreter handling package.

Add a package to "vpython" to locate and manipulate the system Python
interpreter. This includes location, version resolution, and invocation
functionality.

This package includes opportunistic tests, which will run and assert
based on which interpreter(s) are currently installed on the system.

BUG=chromium:691818
TEST=unit
==========

to

==========
vpython: Add Python interpreter handling package.

Add a package to "vpython" to locate and manipulate the system Python
interpreter. This includes location, version resolution, and invocation
functionality.

This package includes opportunistic tests, which will run and assert
based on which interpreter(s) are currently installed on the system.

BUG=chromium:691818
TEST=unit

Review-Url: https://codereview.chromium.org/2701073002
Committed:
https://github.com/luci/luci-go/commit/7e123ef96d5a134b8864271e2db68bcbad3e0036
==========

[commit-bot: I haz the power, 2017-02-22 07:49:43]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/7e123ef96d5a134b8864271e2db68bcbad3e0036