webkitpy: Remove contents of directory.

webkitpy: Remove contents of directory.

Adding a function which removes the contents of a directory but *not*
the directory itself.

This is used by the merge-layout-test-results script when generating
output into an existing directory.

BUG=721466, 524758
Review-Url: https://codereview.chromium.org/2896423004
Cr-Commit-Position: refs/heads/master@{#475391}
Committed: https://chromium.googlesource.com/chromium/src/+/0f2a25964b96c4333d57da6d7fb38afef4bd02fc

[mithro, 2017-05-25 04:09:45]

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

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

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-25 05:42:11]

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

[commit-bot: I haz the power, 2017-05-25 05:42:12]

Dry run: This issue passed the CQ dry run.

[mithro, 2017-05-25 06:44:21]

tansell@chromium.org changed reviewers:
+ mcgreevy@chromium.org, qyearsley@chromium.org

[mithro, 2017-05-25 06:44:21]

Hi!

While we are working on using different directories in recipes for the merged
output (see https://chromium-review.googlesource.com/c/508449), I still think
having the directory contents removal feature is useful because of the issue on
Windows with removing directories which have stuff running from.

I'd still like to add some tests for this, but wanted to see what you thought
before doing so.

Tim 'mithro' Ansell
(I've put Dirk is an optional reviewer.)

[qyearsley, 2017-05-25 23:32:12]

General background question, when does the merge-layout-test-results script
generate output into an existing directory that we want to clear out?

Initial thoughts when reading this change so far:

 1. I wonder if there's some shorter/simpler alternative to the current version
of remove_contents. I haven't thought about it much, but to remove the contents
of a directory but keep the directory itself as empty directory, would
rmtree(dir) followed by mkdir(dir) work? or might that sometimes not work due to
permissions issues or some other reason?

 2. Somewhat unrelated to this CL: main() in merge-layout-test-results is pretty
long, I wonder if some parts could be extracted out to helper functions, which
could possibly be put in webkitpy.layout_tests.merge_results?

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:36: import
logging
Nit: The general convention in webkitpy is to use

    _log = logging.getLogger(__name__)

And then when logging, use _log.method instead of logging.method.

Example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

The main practical benefit of this is that when tools use these common modules
they have verbose logging modes which print the place from where the message is
being logged. For example:

./webkit-patch auto-rebaseline -v 
webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
webkitpy.common.system.executive: [DEBUG] "git rev-parse --is-inside-work-tree"
took 0.02s
...

If, instead, we directly using "logging.debug" in executive.py, this would
instead print:
webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
root: [DEBUG] "git rev-parse --is-inside-work-tree" took 0.02s

[mcgreevy_g, 2017-05-26 03:02:43]

mcgreevy@google.com changed reviewers:
+ mcgreevy@google.com

[mcgreevy_g, 2017-05-26 03:02:43]

Why do we need this at all if we're going to separate the output directories for
the with and without patch runs of webkit_layout_tests?

[mithro, 2017-05-26 04:04:23]

On 2017/05/25 23:32:12, qyearsley wrote:
> General background question, when does the merge-layout-test-results script
> generate output into an existing directory that we want to clear out?
> 
> Initial thoughts when reading this change so far:
> 
>  1. I wonder if there's some shorter/simpler alternative to the current
version
> of remove_contents. I haven't thought about it much, but to remove the
contents
> of a directory but keep the directory itself as empty directory, would
> rmtree(dir) followed by mkdir(dir) work? or might that sometimes not work due
to
> permissions issues or some other reason?

rmtree(dir) followed by mkdir(dir) was what we were doing previously and fails
in two ways;

 * If dir is the cwd, windows won't let you remove + recreate the directory as a
process is currently using it.
 * Removing and recreating the directory invalidates any open handles on the
directory. This is what caused recipes to start failing because it had a handle
to a directory which was now gone.

Working around these problems is why removing the contents of a directory but
not the directory is useful.

>  2. Somewhat unrelated to this CL: main() in merge-layout-test-results is
pretty
> long, I wonder if some parts could be extracted out to helper functions, which
> could possibly be put in webkitpy.layout_tests.merge_results?

Sure, but let think about that in another CL.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:36: import
logging
On 2017/05/25 23:32:12, qyearsley wrote:
> Nit: The general convention in webkitpy is to use
> 
>     _log = logging.getLogger(__name__)
> 
> And then when logging, use _log.method instead of logging.method.
> 
> Example:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> The main practical benefit of this is that when tools use these common modules
> they have verbose logging modes which print the place from where the message
is
> being logged. For example:
> 
> ./webkit-patch auto-rebaseline -v 
> webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
> webkitpy.common.system.executive: [DEBUG] "git rev-parse
--is-inside-work-tree"
> took 0.02s
> ...
> 
> If, instead, we directly using "logging.debug" in executive.py, this would
> instead print:
> webkitpy.common.system.log_utils: [DEBUG] Debug logging enabled.
> root: [DEBUG] "git rev-parse --is-inside-work-tree" took 0.02s

Done.

[mithro, 2017-05-26 04:09:32]

On 2017/05/26 03:02:43, mcgreevy_g wrote:
> Why do we need this at all if we're going to separate the output directories
for
> the with and without patch runs of webkit_layout_tests?

While we probably won't need it for the merge script on the bots anymore, it
still seems useful enough when running locally. It also gives us an option if
the output directory stuff doesn't turn out to work as expected.

This function will likely end up being used in more then the merge script as for
a bunch of tools it does make sense to remove the existing contents before
generating new contents or for cleanup purposes (like when archive_layout_tests
is migrated).

[mcgreevy_g, 2017-05-26 06:38:01]

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:283: bool:
True when the directory is now empty.
s/when/if/

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:285: # We
try multiple times as on Windows a process which is currently closing
I'd put a comma after "times" and use "because" instead of "as".

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:301: isdir
= True
I think it's odd to intialize this to True when you are unconditionally setting
it on line 303, and you continue in the case of an exception.  I would
initialize it to None here. But I defer to your judgment when it comes to python
idioms.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:316:
self.remove(fullname)
The implementation of remove has a series of retries, but you're calling it from
inside another retry loop here.  So you'll wait up to 5 attempts * 3s = 15s in
the case where you're waiting on a single file.  If you have N files that you're
failing to remove (which never succeed) you'll end up waiting 15*N seconds.

Instead, I would decide on a deadline before line 295. Get rid of the while
attempts < 5 loop, and work through each entry in dirname one by one,
reattempting each one an indefinite number of times until the deadline expires. 
At that point, either give up, or just try to delete the rest of the entries
with no retries.

You will end up allowing equal time for each entry to be deleted (because while
you're waiting on one, you're also waiting on the rest), but the timeout will be
predicable rather than depending on the number of files.

[mithro, 2017-05-29 03:55:58]

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:283: bool:
True when the directory is now empty.
On 2017/05/26 06:38:01, mcgreevy_g wrote:
> s/when/if/

Done.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:285: # We
try multiple times as on Windows a process which is currently closing
On 2017/05/26 06:38:01, mcgreevy_g wrote:
> I'd put a comma after "times" and use "because" instead of "as".

Done.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:301: isdir
= True
On 2017/05/26 06:38:01, mcgreevy_g wrote:
> I think it's odd to intialize this to True when you are unconditionally
setting
> it on line 303, and you continue in the case of an exception.  I would
> initialize it to None here. But I defer to your judgment when it comes to
python
> idioms.

Acknowledged.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:316:
self.remove(fullname)
On 2017/05/26 06:38:01, mcgreevy_g wrote:
> The implementation of remove has a series of retries, but you're calling it
from
> inside another retry loop here.  So you'll wait up to 5 attempts * 3s = 15s in
> the case where you're waiting on a single file.  If you have N files that
you're
> failing to remove (which never succeed) you'll end up waiting 15*N seconds.
> 
> Instead, I would decide on a deadline before line 295. Get rid of the while
> attempts < 5 loop, and work through each entry in dirname one by one,
> reattempting each one an indefinite number of times until the deadline
expires. 
> At that point, either give up, or just try to delete the rest of the entries
> with no retries.
> 
> You will end up allowing equal time for each entry to be deleted (because
while
> you're waiting on one, you're also waiting on the rest), but the timeout will
be
> predicable rather than depending on the number of files.

I added a flag to prevent retry behaviour in the remove function. The desired
behaviour is to attempt to delete the whole directory three times -- I'm not
really after a timeout behaviour.

If a file is failed to be removed, it will cause a kind of cascade where the
parent directories will fail to remove (because they are not empty). This is why
we want to try the whole process again.

[mithro, 2017-05-29 03:56:06]

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

[commit-bot: I haz the power, 2017-05-29 03:56:27]

Dry run: CQ is trying da patch.

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

[mcgreevy_g, 2017-05-29 04:17:56]

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:316:
self.remove(fullname)
> The desired
> behaviour is to attempt to delete the whole directory three times

Why three?

> If a file is failed to be removed, it will cause a kind of cascade where the
> parent directories will fail to remove (because they are not empty). This is
why
> we want to try the whole process again.

But this code operates from the top of the tree, and relies on shutil.rmtree to
traverse down the tree.  So as far as this code is concerned, a failure (of
either "remove" or "rmtree") does not require any special handling of parent
directories.  What am I missing?

[commit-bot: I haz the power, 2017-05-29 05:11:48]

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

[commit-bot: I haz the power, 2017-05-29 05:11:49]

Dry run: This issue passed the CQ dry run.

[mithro, 2017-05-29 06:00:30]

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:316:
self.remove(fullname)
On 2017/05/29 04:17:56, mcgreevy_g wrote:
> > The desired
> > behaviour is to attempt to delete the whole directory three times
> 
> Why three?

Arbitrary number.

> > If a file is failed to be removed, it will cause a kind of cascade where the
> > parent directories will fail to remove (because they are not empty). This is
> why
> > we want to try the whole process again.
> 
> But this code operates from the top of the tree, and relies on shutil.rmtree
to
> traverse down the tree.  So as far as this code is concerned, a failure (of
> either "remove" or "rmtree") does not require any special handling of parent
> directories.  What am I missing?

You are correct. 

The cascade will only happen in the rmtree path. The errors in that case get
handled by the onerror function.

[mithro, 2017-05-29 06:07:37]

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

[commit-bot: I haz the power, 2017-05-29 06:07:51]

Dry run: CQ is trying da patch.

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

[mcgreevy_g, 2017-05-29 07:00:54]

LGTM with nits.

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
(right):

https://codereview.chromium.org/2896423004/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py:316:
self.remove(fullname)
On 2017/05/29 06:00:30, mithro wrote:
> On 2017/05/29 04:17:56, mcgreevy_g wrote:
> > > The desired
> > > behaviour is to attempt to delete the whole directory three times
> > 
> > Why three?
> 
> Arbitrary number.
> 
> > > If a file is failed to be removed, it will cause a kind of cascade where
the
> > > parent directories will fail to remove (because they are not empty). This
is
> > why
> > > we want to try the whole process again.
> > 
> > But this code operates from the top of the tree, and relies on shutil.rmtree
> to
> > traverse down the tree.  So as far as this code is concerned, a failure (of
> > either "remove" or "rmtree") does not require any special handling of parent
> > directories.  What am I missing?
> 
> You are correct. 
> 
> The cascade will only happen in the rmtree path. The errors in that case get
> handled by the onerror function.

I still think setting a deadline would be nicer, but at least with retries
disabled in remove, the running time is now predictable.

https://codereview.chromium.org/2896423004/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/merge-layout-test-results (right):

https://codereview.chromium.org/2896423004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/merge-layout-test-results:170: if
args.remove_existing_output_directory and not
FileSystem().remove_contents(args.output_directory):
It's a bit odd to remove the contents of a directory weven in the case where
you've created it. If you pulled this directory creating/content-removal out
into a function, I think it would be simpler. Something like:


def ensure_empty_dir(directory, allow_existing, remove_existing):
  if not os.path.exists(args.output_directory):
    os.makedirs(args.output_directory)
    return

  logging.warning('Output directory exists %r', args.output_directory)
  if not allow_existing:
      raise IOError(
          ('Output directory %s exists!\n'
           'Use --allow-existing-output-directory to continue') %
args.output_directory)

  if remove_existing and not
FileSystem().remove_contents(args.output_directory):
      raise IOError(
          ('Unable to remove output directory %s contents!\n'
           'See log output for errors.') % args.output_directory)

[mithro, 2017-05-29 07:01:44]

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

[commit-bot: I haz the power, 2017-05-29 07:02:01]

Dry run: CQ is trying da patch.

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

[mithro, 2017-05-29 07:14:54]

https://codereview.chromium.org/2896423004/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/merge-layout-test-results (right):

https://codereview.chromium.org/2896423004/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/merge-layout-test-results:170: if
args.remove_existing_output_directory and not
FileSystem().remove_contents(args.output_directory):
On 2017/05/29 07:00:54, mcgreevy_g wrote:
> It's a bit odd to remove the contents of a directory weven in the case where
> you've created it. If you pulled this directory creating/content-removal out
> into a function, I think it would be simpler. Something like:
> 
> 
> def ensure_empty_dir(directory, allow_existing, remove_existing):
>   if not os.path.exists(args.output_directory):
>     os.makedirs(args.output_directory)
>     return
> 
>   logging.warning('Output directory exists %r', args.output_directory)
>   if not allow_existing:
>       raise IOError(
>           ('Output directory %s exists!\n'
>            'Use --allow-existing-output-directory to continue') %
> args.output_directory)
> 
>   if remove_existing and not
> FileSystem().remove_contents(args.output_directory):
>       raise IOError(
>           ('Unable to remove output directory %s contents!\n'
>            'See log output for errors.') % args.output_directory)
> 
> 

Done.

[mithro, 2017-05-29 07:16:18]

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

[commit-bot: I haz the power, 2017-05-29 07:16: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-29 08:36:34]

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

[commit-bot: I haz the power, 2017-05-29 08:36:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mike Lawther (Google), 2017-05-29 21:05:30]

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

[Mike Lawther (Google), 2017-05-29 21:05:30]

The CQ bit was checked by mikelawther@chromium.org

[Mike Lawther (Google), 2017-05-29 21:05:30]

lgtm

[Mike Lawther (Google), 2017-05-29 21:05:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mcgreevy@google.com
Link to the patchset: https://codereview.chromium.org/2896423004/#ps80001
(title: "Fixing nits")

[commit-bot: I haz the power, 2017-05-29 21:05:45]

CQ is trying da patch.

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

[mithro, 2017-05-29 21:28:26]

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

[commit-bot: I haz the power, 2017-05-29 21:28:43]

Dry run: CQ is trying da patch.

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

[mithro, 2017-05-29 22:29:19]

The CQ bit was unchecked by tansell@chromium.org

[mithro, 2017-05-29 22:29:43]

The CQ bit was checked by tansell@chromium.org

[mithro, 2017-05-29 22:29:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
mikelawther@chromium.org, mcgreevy@google.com
Link to the patchset: https://codereview.chromium.org/2896423004/#ps100001
(title: "Fixing tests.")

[commit-bot: I haz the power, 2017-05-29 22:29:50]

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-29 22:41:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496096983033910,
"parent_rev": "1497017e0dcacd563f483323550e77d32f536d87", "commit_rev":
"0f2a25964b96c4333d57da6d7fb38afef4bd02fc"}

[commit-bot: I haz the power, 2017-05-29 22:41:21]

Description was changed from

==========
webkitpy: Remove contents of directory.

Adding a function which removes the contents of a directory but *not*
the directory itself.

This is used by the merge-layout-test-results script when generating
output into an existing directory.

BUG=721466,524758
==========

to

==========
webkitpy: Remove contents of directory.

Adding a function which removes the contents of a directory but *not*
the directory itself.

This is used by the merge-layout-test-results script when generating
output into an existing directory.

BUG=721466,524758

Review-Url: https://codereview.chromium.org/2896423004
Cr-Commit-Position: refs/heads/master@{#475391}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f2a25964b96c4333d57da6d7fb3...
==========

[commit-bot: I haz the power, 2017-05-29 22:41:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0f2a25964b96c4333d57da6d7fb3...

[qyearsley, 2017-05-30 16:22:16]

lgtm