vpython: Add filesystem operation / test packages.

vpython: Add filesystem operation / test packages.

Adds the "filesystem" vpython package, which includes the filesystem
operations used by the "vpython" tool.

Also includes "filesystem/testfs", which are testing-oriented filesystem
operations.

Both of these will be heavily leveraged in future patch sets.

BUG=chromium:691818
TEST=None

Review-Url: https://codereview.chromium.org/2703793003
Committed: https://github.com/luci/luci-go/commit/091cb1591f7a0516f730228126c04e5574dc7db0

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

Description was changed from

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

vpython: Add filesystem operation / test packages.

Adds the "filesystem" vpython package, which includes the filesystem
operations used by the "vpython" tool.

Also includes "filesystem/testfs", which are testing-oriented filesystem
operations.

Both of these will be heavily leveraged in future patch sets.

BUG=chromium:691818
TEST=None
==========

to

==========
vpython: Add filesystem operation / test packages.

Adds the "filesystem" vpython package, which includes the filesystem
operations used by the "vpython" tool.

Also includes "filesystem/testfs", which are testing-oriented filesystem
operations.

Both of these will be heavily leveraged in future patch sets.

BUG=chromium:691818
TEST=None
==========

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

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

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

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 08:25:09]

lgtm

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
File vpython/filesystem/filesystem.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:28: return errors.Annotate(err).Reason("unable
to resolve absolute path").
did you want to include 'base' in this Reason? or are you only printing full
stacks?

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:51: if err := os.RemoveAll(path); err != nil {
RemoveAll is pretty short: https://golang.org/src/os/path.go?s=1634:1667#L56

Maybe it would be worth implementing a new version of it that integrates the
chmod so that we don't have to recurse the filesystem twice (relatively
expensive on windows, lots of extra stats)

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:87: return errors.Annotate(err).Reason("failed
to set read-only: %(path)s").
read-only? maybe this message needs tweaking

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/b...
File vpython/filesystem/testfs/build.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/b...
vpython/filesystem/testfs/build.go:17: var BuildDir = "\x00"
why not

layout := map[string]string {
  "some/dir/": "",  # this creates "some/dir"
}

this would be fairly unambiguous, I think?

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/t...
File vpython/filesystem/testfs/tempdir.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/t...
vpython/filesystem/testfs/tempdir.go:21: _ = filesystem.RemoveAll(tdir)
worth logging?

[iannucci, 2017-02-21 08:25:38]

also... maybe add some tests?

[dnj, 2017-02-21 18:07:49]

Added tests. Hopefully the timing ones aren't flaky, but they shouldn't be.

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
File vpython/filesystem/filesystem.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:28: return errors.Annotate(err).Reason("unable
to resolve absolute path").
On 2017/02/21 08:25:08, iannucci wrote:
> did you want to include 'base' in this Reason? or are you only printing full
> stacks?

Just chaining it here. I am printing full stacks, so this will be included.

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:51: if err := os.RemoveAll(path); err != nil {
On 2017/02/21 08:25:08, iannucci wrote:
> RemoveAll is pretty short: https://golang.org/src/os/path.go?s=1634:1667#L56
> 
> Maybe it would be worth implementing a new version of it that integrates the
> chmod so that we don't have to recurse the filesystem twice (relatively
> expensive on windows, lots of extra stats)

Done. This is probably not a huge gain, though, since only directories are
stat-ted in RemoveAll.

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/filesyst...
vpython/filesystem/filesystem.go:87: return errors.Annotate(err).Reason("failed
to set read-only: %(path)s").
On 2017/02/21 08:25:08, iannucci wrote:
> read-only? maybe this message needs tweaking

Done.

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/b...
File vpython/filesystem/testfs/build.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/b...
vpython/filesystem/testfs/build.go:17: var BuildDir = "\x00"
On 2017/02/21 08:25:08, iannucci wrote:
> why not
> 
> layout := map[string]string {
>   "some/dir/": "",  # this creates "some/dir"
> }
> 
> this would be fairly unambiguous, I think?

Done.

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/t...
File vpython/filesystem/testfs/tempdir.go (right):

https://codereview.chromium.org/2703793003/diff/1/vpython/filesystem/testfs/t...
vpython/filesystem/testfs/tempdir.go:21: _ = filesystem.RemoveAll(tdir)
On 2017/02/21 08:25:09, iannucci wrote:
> worth logging?

Done.

[dnj, 2017-02-22 06:51:42]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-22 06:51:43]

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/2703793003/#ps40001
(title: "fix")

[commit-bot: I haz the power, 2017-02-22 06:51:50]

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

[dnj, 2017-02-22 06:56:59]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2017-02-22 06:57:51]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-22 06:57:51]

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/2703793003/#ps60001
(title: "up the filesystem timeout for macs")

[commit-bot: I haz the power, 2017-02-22 06:58:00]

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:05:38]

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/347d6e71b290c410)

[dnj, 2017-02-22 07:15:59]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-02-22 07:15:59]

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/2703793003/#ps100001
(title: "test now path")

[commit-bot: I haz the power, 2017-02-22 07:16:09]

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:21:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487747759703910,
"parent_rev": "49cc23e49067b4303d84e86a81e85d25bb27f3ee", "commit_rev":
"091cb1591f7a0516f730228126c04e5574dc7db0"}

[commit-bot: I haz the power, 2017-02-22 07:21:28]

Description was changed from

==========
vpython: Add filesystem operation / test packages.

Adds the "filesystem" vpython package, which includes the filesystem
operations used by the "vpython" tool.

Also includes "filesystem/testfs", which are testing-oriented filesystem
operations.

Both of these will be heavily leveraged in future patch sets.

BUG=chromium:691818
TEST=None
==========

to

==========
vpython: Add filesystem operation / test packages.

Adds the "filesystem" vpython package, which includes the filesystem
operations used by the "vpython" tool.

Also includes "filesystem/testfs", which are testing-oriented filesystem
operations.

Both of these will be heavily leveraged in future patch sets.

BUG=chromium:691818
TEST=None

Review-Url: https://codereview.chromium.org/2703793003
Committed:
https://github.com/luci/luci-go/commit/091cb1591f7a0516f730228126c04e5574dc7db0
==========

[commit-bot: I haz the power, 2017-02-22 07:21:29]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/091cb1591f7a0516f730228126c04e5574dc7db0