[recipe_modules/file] add real file module to engine.

[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org, phajdan.jr@chromium.org
BUG=skia:6628
Review-Url: https://codereview.chromium.org/2918463002
Committed: https://github.com/luci/recipes-py/commit/d878bba594c81c7ba4cc16b8bf078097b00a102d

[iannucci, 2017-05-31 03:35:29]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 03:35:40]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-05-31 03:36:14]

Description was changed from

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org
BUG=
==========

to

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org
BUG=skia:6628
==========

[commit-bot: I haz the power, 2017-05-31 03:41:49]

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

[commit-bot: I haz the power, 2017-05-31 03:41:50]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36756730a91be310)

[iannucci, 2017-05-31 03:52:56]

On 2017/05/31 03:35:28, iannucci wrote:

Oh, yeah, oops. Forgot to add the path tests.

[iannucci, 2017-05-31 04:01:28]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 04:01:40]

Dry run: 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-05-31 04:07:09]

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

[commit-bot: I haz the power, 2017-05-31 04:07:10]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36757c3865f76010)

[iannucci, 2017-05-31 04:21:48]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 04:21:53]

Dry run: 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-05-31 04:27:27]

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

[commit-bot: I haz the power, 2017-05-31 04:27:28]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36758eba9b0ff410)

[iannucci, 2017-05-31 04:29:55]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 04:29:57]

Dry run: 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-05-31 04:35:46]

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

[commit-bot: I haz the power, 2017-05-31 04:35:47]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/367598941fcdec10)

[iannucci, 2017-05-31 06:15:30]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 06:15:46]

Dry run: 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-05-31 06:23:20]

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

[commit-bot: I haz the power, 2017-05-31 06:23:21]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3675f70152e30910)

[iannucci, 2017-05-31 07:02:49]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 07:03:00]

Dry run: 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-05-31 07:06:22]

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

[commit-bot: I haz the power, 2017-05-31 07:06:23]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-05-31 07:11:16]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 07:11:26]

Dry run: 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-05-31 07:14:36]

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

[commit-bot: I haz the power, 2017-05-31 07:14:38]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-05-31 16:45:52]

Description was changed from

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org
BUG=skia:6628
==========

to

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org,
phajdan.jr@chromium.org
BUG=skia:6628
==========

[iannucci, 2017-05-31 16:45:52]

iannucci@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[iannucci, 2017-05-31 16:45:59]

PTAL

[dnj, 2017-05-31 16:50:40]

first round of comments

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api.py
File recipe_modules/file/api.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:37: j = result.json.outputs['main']
Only one output, WDYT about naming default?

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:76: timeout (int) - Maximum number of seconds to
allow this command to run.
nit: Document "symlinks"

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:100: self._run(name, ['move', source, dest],
timeout=5)
This timeout might actually be inadequate if it's cross-filesystem moving large
file?

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:182: timeout (int) - Maximum number of seconds to
allow this command to run.
Is timeout useful here? I can't think of a meaningful way to set it. Maybe
numeric limit instead?

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:184: Returns list[str] - The names of the entries in
the source directory. Note
QQ: why not Path objects?

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
File recipe_modules/file/resources/fileutil.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
recipe_modules/file/resources/fileutil.py:50: for p in (os.path.join(path, x)
for x in os.listdir(path)):
nit: should we chmod 0770 everything we're about to delete? We do it to root
below, but...

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
recipe_modules/file/resources/fileutil.py:83: if not subprocess.call(['cmd.exe',
'/c', 'rd', '/q', '/s', path]):
nit: let's actually define this once and reference it twice.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/tes...
File recipe_modules/file/test_api.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/tes...
recipe_modules/file/test_api.py:42: def glob_paths(self, paths=(),
errno_name=0):
errno_name=0 .. why not None? I like None as a more descriptive and less
committal default. Imagine you're someone wrapping this, now you have to
propagate the "0" default to the wrapper etc. etc. Here and elsewhere.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/tes...
recipe_modules/file/test_api.py:66: This must be e.g. 'EPERM', 'EEXIST', etc.
'must be ... etc.' seems silly - either have some constants and must be one of
them, or "should be"

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/path/api.py
File recipe_modules/path/api.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/path/api...
recipe_modules/path/api.py:60: source, dest = str(source), str(dest)
Needs initialize?

[iannucci, 2017-05-31 22:59:21]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[iannucci, 2017-05-31 22:59:27]

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api.py
File recipe_modules/file/api.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:37: j = result.json.outputs['main']
On 2017/05/31 16:50:40, dnj wrote:
> Only one output, WDYT about naming default?

at first I thought that other things would need their own json, but nothing else
actually does. Done.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:76: timeout (int) - Maximum number of seconds to
allow this command to run.
On 2017/05/31 16:50:40, dnj wrote:
> nit: Document "symlinks"

Done.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:100: self._run(name, ['move', source, dest],
timeout=5)
On 2017/05/31 16:50:40, dnj wrote:
> This timeout might actually be inadequate if it's cross-filesystem moving
large
> file?

blergh, true. Removed.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/api...
recipe_modules/file/api.py:184: Returns list[str] - The names of the entries in
the source directory. Note
On 2017/05/31 16:50:40, dnj wrote:
> QQ: why not Path objects?

Because the usage of listdir frequently deals with just the filenames. We can
revisit this later if stuff tries to Pathify it (I'll see when I convert stuff
to use this)

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
File recipe_modules/file/resources/fileutil.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
recipe_modules/file/resources/fileutil.py:50: for p in (os.path.join(path, x)
for x in os.listdir(path)):
On 2017/05/31 16:50:40, dnj wrote:
> nit: should we chmod 0770 everything we're about to delete? We do it to root
> below, but...

Done.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/file/res...
recipe_modules/file/resources/fileutil.py:83: if not subprocess.call(['cmd.exe',
'/c', 'rd', '/q', '/s', path]):
On 2017/05/31 16:50:40, dnj wrote:
> nit: let's actually define this once and reference it twice.

Done.

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/path/api.py
File recipe_modules/path/api.py (right):

https://codereview.chromium.org/2918463002/diff/80001/recipe_modules/path/api...
recipe_modules/path/api.py:60: source, dest = str(source), str(dest)
On 2017/05/31 16:50:40, dnj wrote:
> Needs initialize?

Done.

[commit-bot: I haz the power, 2017-05-31 22:59:28]

Dry run: 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-05-31 23:02:18]

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

[commit-bot: I haz the power, 2017-05-31 23:02:19]

Dry run: This issue passed the CQ dry run.

[dnj, 2017-05-31 23:58:58]

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
File recipe_modules/file/api.py (right):

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
recipe_modules/file/api.py:31: '--json-output', self.m.json.output()
Still think this should be "add_json_log=False". I don't think the JSON output
is too useful, and big dirs etc. could be yuge.

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
recipe_modules/file/api.py:189: return map(self.m.path.abs_to_path,
ret.stdout.splitlines())
If the process returns abspath, the test data will be hard to form. IMO have the
process still return just names, and join them against "source" in recipe.

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/re...
File recipe_modules/file/resources/fileutil.py (right):

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/re...
recipe_modules/file/resources/fileutil.py:150: source = os.path.abspath(source)
(As mentioned above, I'd still keep this to names).

[iannucci, 2017-06-01 00:31:03]

PTAnL

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
File recipe_modules/file/api.py (right):

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
recipe_modules/file/api.py:31: '--json-output', self.m.json.output()
On 2017/05/31 23:58:58, dnj wrote:
> Still think this should be "add_json_log=False". I don't think the JSON output
> is too useful, and big dirs etc. could be yuge.

changed to step_text only when it fails

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/ap...
recipe_modules/file/api.py:189: return map(self.m.path.abs_to_path,
ret.stdout.splitlines())
On 2017/05/31 23:58:58, dnj wrote:
> If the process returns abspath, the test data will be hard to form. IMO have
the
> process still return just names, and join them against "source" in recipe.

Done.

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/re...
File recipe_modules/file/resources/fileutil.py (right):

https://codereview.chromium.org/2918463002/diff/140001/recipe_modules/file/re...
recipe_modules/file/resources/fileutil.py:150: source = os.path.abspath(source)
On 2017/05/31 23:58:58, dnj wrote:
> (As mentioned above, I'd still keep this to names).

Done.

[iannucci, 2017-06-01 00:50:04]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 00:50:16]

Dry run: 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-06-01 00:53:25]

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

[commit-bot: I haz the power, 2017-06-01 00:53:26]

Dry run: This issue passed the CQ dry run.

[dnj, 2017-06-01 03:01:59]

nice lgtm

[iannucci, 2017-06-01 03:05:03]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 03:05:12]

Dry run: CQ is trying da patch.

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

[iannucci, 2017-06-01 03:06:22]

The CQ bit was unchecked by iannucci@chromium.org

[iannucci, 2017-06-01 03:06:26]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2017-06-01 03:06:26]

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

[commit-bot: I haz the power, 2017-06-01 03:06:32]

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-06-01 03:10:46]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496286386022760,
"parent_rev": "8ff178b2ec0c099d49ecb1df1c411f487c6bc657", "commit_rev":
"d878bba594c81c7ba4cc16b8bf078097b00a102d"}

[commit-bot: I haz the power, 2017-06-01 03:10:50]

Description was changed from

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org,
phajdan.jr@chromium.org
BUG=skia:6628
==========

to

==========
[recipe_modules/file] add real file module to engine.

Including docs, mocks and more :)

R=borenet@chromium.org, dnj@chromium.org, nodir@chromium.org,
phajdan.jr@chromium.org
BUG=skia:6628

Review-Url: https://codereview.chromium.org/2918463002
Committed:
https://github.com/luci/recipes-py/commit/d878bba594c81c7ba4cc16b8bf078097b00...
==========

[commit-bot: I haz the power, 2017-06-01 03:10:52]

Committed patchset #10 (id:180001) as
https://github.com/luci/recipes-py/commit/d878bba594c81c7ba4cc16b8bf078097b00...

[Lei Zhang, 2017-06-27 05:03:04]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2017-06-27 05:03:05]

https://codereview.chromium.org/2918463002/diff/180001/recipe_modules/file/ap...
File recipe_modules/file/api.py (right):

https://codereview.chromium.org/2918463002/diff/180001/recipe_modules/file/ap...
recipe_modules/file/api.py:82: prefix = ['--symlinks'] if symlinks else []
On
https://build.chromium.org/p/chromium.infra.cron/builders/publish_tarball/bui...
the copytree step is purple.

The log says:

 cmd: ['python', '-u',
'/b/rr/tmpuuLFXu/rw/recipes/.recipe_deps/recipe_engine/recipe_modules/file/resources/fileutil.py',
'--json-output', '/b/rr/tmpuuLFXu/t/tmp0SqfWO.json', '--symlinks', 'copytree',
'/b/rr/tmpuuLFXu/w/src', '/b/rr/tmpuuLFXu/t/rt/tmp6cvLbA/src']


usage: fileutil.py [-h] --json-output JSON_OUTPUT
                  
{rmtree,rmcontents,rmglob,copy,copytree,move,glob,remove,listdir,ensure-directory}
                   ...
fileutil.py: error: unrecognized arguments: --symlinks
step returned non-zero exit code: 2