recipe engine: add tempfile and shutil modules

recipe engine: add tempfile and shutil modules

BUG=chromium:583303
Committed: https://github.com/luci/recipes-py/commit/0978755542b27bbb752e3e6150872e0b75e3c068

[Paweł Hajdan Jr., 2016-04-07 10:37:24]

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

[commit-bot: I haz the power, 2016-04-07 10:37:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1866313002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1866313002/1

[commit-bot: I haz the power, 2016-04-07 10:40:56]

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

[commit-bot: I haz the power, 2016-04-07 10:40:56]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Recipes-py%20Presubmit/...)

[Paweł Hajdan Jr., 2016-04-07 10:49:59]

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

[commit-bot: I haz the power, 2016-04-07 10:50:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1866313002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1866313002/20001

[commit-bot: I haz the power, 2016-04-07 10:51:56]

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

[commit-bot: I haz the power, 2016-04-07 10:51:57]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2016-04-07 10:53:43]

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

[Paweł Hajdan Jr., 2016-04-07 10:53:44]

This will be needed for autoroller CL https://codereview.chromium.org/1862383002

[martiniss, 2016-04-07 17:35:22]

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
File recipe_modules/shutil/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
recipe_modules/shutil/api.py:7: class ShutilApi(recipe_api.RecipeApi):
I'm uncomfortable having this, when we have the file recipe module at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...

It would be better to just upstream this, since most of those functions aren't
build dependent. However, several of them depend on depot_tools/build python
scripts to function properly, so they can't be put into the recipe engine. 

I would prefer to just inline this wherever you need this for now; I know
tandrii did for his recipe at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
File recipe_modules/tempfile/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
This (roughly) already exists at
https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py#L198

If you want a context manager, adding one to the path class would probably be
preferable. Although it's a bit strange that the mkdtemp function is part of the
path api module.

[Paweł Hajdan Jr., 2016-04-07 20:39:04]

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
File recipe_modules/shutil/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
recipe_modules/shutil/api.py:7: class ShutilApi(recipe_api.RecipeApi):
On 2016/04/07 at 17:35:22, martiniss wrote:
> I'm uncomfortable having this, when we have the file recipe module at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...

With shutil module here, we can move build-independent methods from file module
here.

Note however that file.rmtree currently depends on chromium_utils, and I'm not
sure if we can change that without breakage.

> It would be better to just upstream this, since most of those functions aren't
build dependent. However, several of them depend on depot_tools/build python
scripts to function properly, so they can't be put into the recipe engine. 
> 
> I would prefer to just inline this wherever you need this for now; I know
tandrii did for his recipe at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...

I'm leaning towards providing shutil here - at least there'd be one place,
rather than several different copies sprinkled throughout codebase.

Is yours a strong opinion? If so, could you provide rationale for it? FWIW mine
is like above - less number of places affected by ugliness, if there is any
ugliness at all.

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
File recipe_modules/tempfile/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
On 2016/04/07 at 17:35:22, martiniss wrote:
> This (roughly) already exists at
https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py#L198

Yes, in fact this method calls path.mkdtemp .

> If you want a context manager, adding one to the path class would probably be
preferable. Although it's a bit strange that the mkdtemp function is part of the
path api module.

That was my first choice. However, then path module would have to depend on
shutil (for rmtree), and that leads to circular dependencies.

To me what's done in this patch makes sense. mkdtemp also registers existence of
the path for testing. Other logic doesn't need that, so can be in a separate
recipe module.

If you have an alternative way to implement it (which is implementable - watch
out for things like circular dependencies I mentioned above), please share.

Also let me know if you'd be OK with having an EMEA reviewer stamp the
implementation provided it'd be consistent with some guidelines we can agree on
(so that we don't need so many round-trips).

[martiniss, 2016-04-07 21:34:02]

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
File recipe_modules/shutil/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
recipe_modules/shutil/api.py:7: class ShutilApi(recipe_api.RecipeApi):
On 2016/04/07 at 20:39:04, Paweł Hajdan Jr. wrote:
> On 2016/04/07 at 17:35:22, martiniss wrote:
> > I'm uncomfortable having this, when we have the file recipe module at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...
> 
> With shutil module here, we can move build-independent methods from file
module here.

If you're volunteering, please add a TODO :) I was planning on doing that
eventually, once the recipe roller and tryjobs have stabilized more.

> 
> Note however that file.rmtree currently depends on chromium_utils, and I'm not
sure if we can change that without breakage.
> 
> > It would be better to just upstream this, since most of those functions
aren't build dependent. However, several of them depend on depot_tools/build
python scripts to function properly, so they can't be put into the recipe
engine. 
> > 
> > I would prefer to just inline this wherever you need this for now; I know
tandrii did for his recipe at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...
> 
> I'm leaning towards providing shutil here - at least there'd be one place,
rather than several different copies sprinkled throughout codebase.

Ok, can you change the inline uses of it to use this module then, in a separate
CL in build then?

> 
> Is yours a strong opinion? If so, could you provide rationale for it? FWIW
mine is like above - less number of places affected by ugliness, if there is any
ugliness at all.

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
File recipe_modules/tempfile/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
On 2016/04/07 at 20:39:04, Paweł Hajdan Jr. wrote:
> On 2016/04/07 at 17:35:22, martiniss wrote:
> > This (roughly) already exists at
https://github.com/luci/recipes-py/blob/master/recipe_modules/path/api.py#L198
> 
> Yes, in fact this method calls path.mkdtemp .

I mis-read this function, sorry.

> 
> > If you want a context manager, adding one to the path class would probably
be preferable. Although it's a bit strange that the mkdtemp function is part of
the path api module.
> 
> That was my first choice. However, then path module would have to depend on
shutil (for rmtree), and that leads to circular dependencies.

shutil doesn't depend on anything, though? How would it be circular.

tempfile -> path -> shutil
tempfile -> shutil

correct?

> 
> To me what's done in this patch makes sense. mkdtemp also registers existence
of the path for testing. Other logic doesn't need that, so can be in a separate
recipe module.
> 
> If you have an alternative way to implement it (which is implementable - watch
out for things like circular dependencies I mentioned above), please share.

circular dependencies are allowed in the recipe engine AFAIK, I believe on
purpose. It might not always be the best idea, but it is technically feasible.

> 
> Also let me know if you'd be OK with having an EMEA reviewer stamp the
implementation provided it'd be consistent with some guidelines we can agree on
(so that we don't need so many round-trips).

I would want a recipe engine owner to stamp any CLs which add a recipe module to
the engine itself, so I'm not sure who besides you in EMEA time would be able to
review this. I'm not sure who out there knows a lot about recipes and best
practices around them.

[martiniss, 2016-04-07 21:52:53]

lgtm

[martiniss, 2016-04-07 21:53:46]

On 2016/04/07 at 21:52:53, martiniss wrote:
> lgtm

(provided you have a TODO and fix build :) )

[iannucci, 2016-04-08 00:19:20]

this lgtm for now, but as I mention below, this is the wrong API for temp
directory management (though it's not this CL's fault, it's moving the
implementation of the bad API within the existing system).

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
File recipe_modules/tempfile/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
This is fine for now, but this is really the wrong way to think about tempdirs
in general. Crashes will cause them to stick around, and nothing will clean them
up.

A better way to do this, I think would be to basically implement the
long-standing crbug.com/361343 . Basically, recipe engine manages TEMP/TMP
envvars and does:
  * at startup, allocate a new tempfolder with some recognizable patter with a
.pid file for ourselves inside
  * hunt for any stale temp folders matching that pattern and delete them
  * each step automatically gets it's own subdir of the overall recipe run's
temp folder, with correctly adjusted TEMP/TMP
  * api.path.mkdtemp would generate a random tempdir inside of this engine-wide
tempdir.
  * cleanups are handled by the engine at the end of a scope (duration of
context manager for path.mkdtemp or duration of the step). Ideally we could also
print an optional leaked-files report on the step so we can e.g. catch messy
tests.

[iannucci, 2016-04-08 00:28:37]

On 2016/04/08 at 00:19:20, iannucci wrote:
> this lgtm for now, but as I mention below, this is the wrong API for temp
directory management (though it's not this CL's fault, it's moving the
implementation of the bad API within the existing system).
> 
>
https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
> File recipe_modules/tempfile/api.py (right):
> 
>
https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
> recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
> This is fine for now, but this is really the wrong way to think about tempdirs
in general. Crashes will cause them to stick around, and nothing will clean them
up.
> 
> A better way to do this, I think would be to basically implement the
long-standing crbug.com/361343 . Basically, recipe engine manages TEMP/TMP
envvars and does:
>   * at startup, allocate a new tempfolder with some recognizable patter with a
.pid file for ourselves inside
>   * hunt for any stale temp folders matching that pattern and delete them
>   * each step automatically gets it's own subdir of the overall recipe run's
temp folder, with correctly adjusted TEMP/TMP
>   * api.path.mkdtemp would generate a random tempdir inside of this
engine-wide tempdir.
>   * cleanups are handled by the engine at the end of a scope (duration of
context manager for path.mkdtemp or duration of the step). Ideally we could also
print an optional leaked-files report on the step so we can e.g. catch messy
tests.

(which it looks like sergiyb may take a stab at :))

[Paweł Hajdan Jr., 2016-04-08 07:46:16]

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
File recipe_modules/shutil/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/shutil/a...
recipe_modules/shutil/api.py:7: class ShutilApi(recipe_api.RecipeApi):
On 2016/04/07 at 21:34:02, martiniss wrote:
> On 2016/04/07 at 20:39:04, Paweł Hajdan Jr. wrote:
> > On 2016/04/07 at 17:35:22, martiniss wrote:
> > > I'm uncomfortable having this, when we have the file recipe module at
https://chromium.googlesource.com/chromium/tools/build.git/+/master/scripts/s...
> > 
> > With shutil module here, we can move build-independent methods from file
module here.
> 
> If you're volunteering, please add a TODO :) I was planning on doing that
eventually, once the recipe roller and tryjobs have stabilized more.

Added TODO and I plan to address it soon. My recipe roller work depends on this
CL landing.

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
File recipe_modules/tempfile/api.py (right):

https://codereview.chromium.org/1866313002/diff/20001/recipe_modules/tempfile...
recipe_modules/tempfile/api.py:12: def temp_dir(self, prefix):
On 2016/04/07 at 21:34:02, martiniss wrote:
> On 2016/04/07 at 20:39:04, Paweł Hajdan Jr. wrote:
> > On 2016/04/07 at 17:35:22, martiniss wrote:
> > > If you want a context manager, adding one to the path class would probably
be preferable. Although it's a bit strange that the mkdtemp function is part of
the path api module.
> > 
> > That was my first choice. However, then path module would have to depend on
shutil (for rmtree), and that leads to circular dependencies.
> 
> shutil doesn't depend on anything, though? How would it be circular.
> 
> tempfile -> path -> shutil
> tempfile -> shutil
> 
> correct?

The circular dependency was path -> python -> step -> path ; by extracting
shutil and tempfile we get tempfile -> shutil -> python -> step -> path (no
cycle).

> > To me what's done in this patch makes sense. mkdtemp also registers
existence of the path for testing. Other logic doesn't need that, so can be in a
separate recipe module.
> > 
> > If you have an alternative way to implement it (which is implementable -
watch out for things like circular dependencies I mentioned above), please
share.
> 
> circular dependencies are allowed in the recipe engine AFAIK, I believe on
purpose. It might not always be the best idea, but it is technically feasible.

That's new to me. I got an actual error with above circular dependency, and
that's in recipe engine.

[Paweł Hajdan Jr., 2016-04-08 07:46:25]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-04-08 07:46:26]

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

[commit-bot: I haz the power, 2016-04-08 07:46:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1866313002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1866313002/40001

[commit-bot: I haz the power, 2016-04-08 07:48:35]

Description was changed from

==========
recipe engine: add tempfile and shutil modules

BUG=chromium:583303
==========

to

==========
recipe engine: add tempfile and shutil modules

BUG=chromium:583303

Committed:
https://github.com/luci/recipes-py/commit/0978755542b27bbb752e3e6150872e0b75e...
==========

[commit-bot: I haz the power, 2016-04-08 07:48:35]

Committed patchset #3 (id:40001) as
https://github.com/luci/recipes-py/commit/0978755542b27bbb752e3e6150872e0b75e...