Makes copy_ex handle directories

Makes copy_ex handle directories

Currently it only handles files. I would like to use it for a project
that needs to copy directories.

R=cjhopman@chromium.org
BUG=none
TEST=none

Committed: https://crrev.com/450cd07835c8e724cf0cc9f2f3dacfe63832b02b
Cr-Commit-Position: refs/heads/master@{#329101}

[cjhopman, 2015-05-08 22:58:55]

Hm. I would really try to avoid copying directories around. It is easy to have
the build rules end up being incorrect because it is difficult to correctly
specify the inputs/outputs (and if you could specify them correctly, then you
could just explicitly copy the files).

For the case where you want to pass directories to other things, it is less
error-prone to zip the contents of the directory and pass the archive around
(though that's a bit more involved on the end that wants to use it).

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.py
File build/android/gyp/copy_ex.py (right):

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
build/android/gyp/copy_ex.py:44: shutil.copytree(f, os.path.join(options.dest,
os.path.basename(f)))
I would require that options.clear be set if any directories are copied.

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
build/android/gyp/copy_ex.py:51: files + build_utils.GetPythonDependencies())
The depfile should list the entire contents of the directories that were copied.

[sky, 2015-05-08 23:38:14]

On 2015/05/08 22:58:55, cjhopman wrote:
> Hm. I would really try to avoid copying directories around. It is easy to have
> the build rules end up being incorrect because it is difficult to correctly
> specify the inputs/outputs (and if you could specify them correctly, then you
> could just explicitly copy the files).

Actually, explicitly listing directories isn't possible right now. For example,
if I list a/b as an input then I end up with 'b', whereas I want 'a/b'. That
said, I still like passing in directories for resources.

> For the case where you want to pass directories to other things, it is less
> error-prone to zip the contents of the directory and pass the archive around
> (though that's a bit more involved on the end that wants to use it).

As you said, this results in more pain on the other end that wants to make use
of the files.

> 
> https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.py
> File build/android/gyp/copy_ex.py (right):
> 
>
https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
> build/android/gyp/copy_ex.py:44: shutil.copytree(f, os.path.join(options.dest,
> os.path.basename(f)))
> I would require that options.clear be set if any directories are copied.
> 
>
https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
> build/android/gyp/copy_ex.py:51: files + build_utils.GetPythonDependencies())
> The depfile should list the entire contents of the directories that were
copied.

[sky, 2015-05-08 23:38:32]

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.py
File build/android/gyp/copy_ex.py (right):

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
build/android/gyp/copy_ex.py:44: shutil.copytree(f, os.path.join(options.dest,
os.path.basename(f)))
On 2015/05/08 22:58:55, cjhopman wrote:
> I would require that options.clear be set if any directories are copied.

Why is it different than with files? The same problem exists in either case, you
may not realize there are stale files around. Seems to me you're arguing for
always clearing, which seems separate from adding support for directories.

I had started adding the warning, but then realized it starts sounding very
weird. Basically it comes down to something like "we don't trust you, use
clear=true when copying directories."

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
build/android/gyp/copy_ex.py:51: files + build_utils.GetPythonDependencies())
On 2015/05/08 22:58:55, cjhopman wrote:
> The depfile should list the entire contents of the directories that were
copied.

I totally missed that. I believe I've fixed it.

[cjhopman, 2015-05-08 23:57:05]

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.py
File build/android/gyp/copy_ex.py (right):

https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
build/android/gyp/copy_ex.py:44: shutil.copytree(f, os.path.join(options.dest,
os.path.basename(f)))
On 2015/05/08 23:38:31, sky wrote:
> On 2015/05/08 22:58:55, cjhopman wrote:
> > I would require that options.clear be set if any directories are copied.
> 
> Why is it different than with files? The same problem exists in either case,
you
> may not realize there are stale files around. Seems to me you're arguing for
> always clearing, which seems separate from adding support for directories.
> 
> I had started adding the warning, but then realized it starts sounding very
> weird. Basically it comes down to something like "we don't trust you, use
> clear=true when copying directories."

Yeah, exactly, I don't trust people (including myself) to do this correctly.

In the case where you are only copying files, I would expect that you will be
able to pass your actual outputs to the consumer (because you were able to tell
copy_ex your actual inputs).

The problem is that if you are copying a directory here, then you are probably
telling whoever uses the outputs to look in a directory (because if you can tell
the consumer the exact files, then why not just tell the copy step the exact
files). If you're doing that, then it's really difficult for the consumer to
detect and ignore stale files.

I would consider requiring options.stamp, too, but that's really only necessary
in gyp, not GN. The issue is that you need to be able to specify some sort of
output that is an input to the consumer so that the build knows the consumer
needs to run. In GN, just having a dependency is enough for that.

[sky, 2015-05-09 14:09:17]

On 2015/05/08 23:57:05, cjhopman wrote:
> https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.py
> File build/android/gyp/copy_ex.py (right):
> 
>
https://codereview.chromium.org/1134813003/diff/1/build/android/gyp/copy_ex.p...
> build/android/gyp/copy_ex.py:44: shutil.copytree(f, os.path.join(options.dest,
> os.path.basename(f)))
> On 2015/05/08 23:38:31, sky wrote:
> > On 2015/05/08 22:58:55, cjhopman wrote:
> > > I would require that options.clear be set if any directories are copied.
> > 
> > Why is it different than with files? The same problem exists in either case,
> you
> > may not realize there are stale files around. Seems to me you're arguing for
> > always clearing, which seems separate from adding support for directories.
> > 
> > I had started adding the warning, but then realized it starts sounding very
> > weird. Basically it comes down to something like "we don't trust you, use
> > clear=true when copying directories."
> 
> Yeah, exactly, I don't trust people (including myself) to do this correctly.
> 
> In the case where you are only copying files, I would expect that you will be
> able to pass your actual outputs to the consumer (because you were able to
tell
> copy_ex your actual inputs).
> 
> The problem is that if you are copying a directory here, then you are probably
> telling whoever uses the outputs to look in a directory (because if you can
tell
> the consumer the exact files, then why not just tell the copy step the exact
> files). If you're doing that, then it's really difficult for the consumer to
> detect and ignore stale files.
> 
> I would consider requiring options.stamp, too, but that's really only
necessary
> in gyp, not GN. The issue is that you need to be able to specify some sort of
> output that is an input to the consumer so that the build knows the consumer
> needs to run. In GN, just having a dependency is enough for that.

Updated to warn if clear not set.

[cjhopman, 2015-05-10 01:51:10]

lgtm

[sky, 2015-05-10 03:09:34]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2015-05-10 03:09:45]

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

[commit-bot: I haz the power, 2015-05-10 07:12:04]

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

[commit-bot: I haz the power, 2015-05-10 07:12:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[sky, 2015-05-10 15:25:18]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2015-05-10 15:26:14]

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

[commit-bot: I haz the power, 2015-05-11 06:54:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-11 06:55:42]

Patchset 3 (id:??) landed as
https://crrev.com/450cd07835c8e724cf0cc9f2f3dacfe63832b02b
Cr-Commit-Position: refs/heads/master@{#329101}