[iOS/Mac/GN] Update timestamp when copying a tree.

[iOS/Mac/GN] Update timestamp when copying a tree.

shutil.copytree does copy the source tree timestamp which confuses
ninja, so stop using shutil.copytree and do the recursive copy in
the script itself.

Use "plutil" to convert the .strings file instead of CoreFoundation
as it looks like it is not necessary to load the file in memory to
check the format.

Remove the "rm -rf" before invoking "compile_xcassets.py" and move
the file deletion into the script.

Add a --version-for-gn parameter with a single supported value and
a descriptive comment to force rebuild when the script changes.

BUG=620950

[sdefresne, 2016-06-17 11:58:43]

sdefresne@chromium.org changed reviewers:
+ rsesek@chromium.org

[sdefresne, 2016-06-17 11:58:43]

This is another version of your CL, WDYT?

[sdefresne, 2016-06-17 13:02:12]

Description was changed from

==========
[iOS/Mac/GN] Update timestamp when copying a tree.

shutil.copytree does copy the source tree timestamp which confuses
ninja, so stop using shutil.copytree and do the recursive copy in
the script itself.

Use "plutil" to convert the .strings file instead of CoreFoundation
as it looks like it is not necessary to load the file in memory to
check the format.

Remove the "rm -rf" before invoking "compile_xcassets.py" and move
the file deletion into the script.

Add a --version-for-gn parameter with a single supported value and
a descriptive comment to force rebuild when the script changes.

BUG=None
==========

to

==========
[iOS/Mac/GN] Update timestamp when copying a tree.

shutil.copytree does copy the source tree timestamp which confuses
ninja, so stop using shutil.copytree and do the recursive copy in
the script itself.

Use "plutil" to convert the .strings file instead of CoreFoundation
as it looks like it is not necessary to load the file in memory to
check the format.

Remove the "rm -rf" before invoking "compile_xcassets.py" and move
the file deletion into the script.

Add a --version-for-gn parameter with a single supported value and
a descriptive comment to force rebuild when the script changes.

BUG=620950
==========

[Robert Sesek, 2016-06-17 17:03:31]

This looks like it takes two different approaches to 
https://codereview.chromium.org/2075703002/ and
https://codereview.chromium.org/2072713005/.

For the tool versioning, I wrote on the CL why I think using the file mtime is
better than a manual version. Let's continue that discussion with Brett on that
CL, since I also add the version to the other scripts. (We are also considering
expressing it as a proper dependency).

For the copying via a manual Python algorithm vs. rsync, I don't have a strong
opinion. I went with rsync rather than re-implementing shutil.copytree because
1) we already use rsync in a few other build scripts, and 2) it may be a little
bit faster.

[sdefresne, 2016-06-17 17:16:37]

On 2016/06/17 17:03:31, Robert Sesek wrote:
> This looks like it takes two different approaches to 
> https://codereview.chromium.org/2075703002/ and
> https://codereview.chromium.org/2072713005/.
> 
> For the tool versioning, I wrote on the CL why I think using the file mtime is
> better than a manual version. Let's continue that discussion with Brett on
that
> CL, since I also add the version to the other scripts. (We are also
considering
> expressing it as a proper dependency).

Ack.

> For the copying via a manual Python algorithm vs. rsync, I don't have a strong
> opinion. I went with rsync rather than re-implementing shutil.copytree because
> 1) we already use rsync in a few other build scripts, and 2) it may be a
little
> bit faster.

I did try to use rsync and I've failed multiple time to pass the correct
options, and this is why I've decided to recode shutil.copy. I see in your CL
that you strip the trailing / in the source path, but I'm unsure whether this is
correct or not.

For example, in iOS, we want to copy the today_extension.appex bundle generated
by a create_bundle target into Chrome.app/PlugIns. So we list
"$root_out_dir/today_extension.appex" in the bundle_data for "chrome" target,
which generate the following ninja rule:

build Chrome.app/PlugIns/today_extension.appex: copy_bundle_data
today_extension.appex

This ends up invoking the script with source = "today_extension.appex" and
output "Chrome.app/PlugIns/today_extension.appex". Looking at your CL, this will
then invoke rsync as this:

rsync --recursive --perms --links today_extension.appex
Chrome.app/PlugIns/today_extension.appex

But if I run this command manually, I end up with the following:

$ find Chrome.app/PlugIns -type d|head
Chrome.app/PlugIns
Chrome.app/PlugIns/today_extension.appex
Chrome.app/PlugIns/today_extension.appex/today_extension.appex
Chrome.app/PlugIns/today_extension.appex/today_extension.appex/_CodeSignature
Chrome.app/PlugIns/today_extension.appex/today_extension.appex/ar.lproj
...

Which is not what is expected. The expected output is:

$ find Chrome.app/PlugIns -type d|head
Chrome.app/PlugIns
Chrome.app/PlugIns/today_extension.appex
Chrome.app/PlugIns/today_extension.appex/_CodeSignature
Chrome.app/PlugIns/today_extension.appex/ar.lproj

The correct rsync command needs the trailing / I think but requires that the
destination directory exists.

$  rsync --recursive --perms --links today_extension.appex/
Chrome.app/PlugIns/today_extension.appex
rsync: mkdir "Chrome.app/PlugIns/today_extension.appex" failed: No such file or
directory (2)
rsync error: error in file IO (code 11) at
/BuildRoot/Library/Caches/com.apple.xbs/Sources/rsync/rsync-47/rsync/main.c(544)
[receiver=2.6.9]
rsync: connection unexpectedly closed (8 bytes received so far) [sender]
rsync error: error in rsync protocol data stream (code 12) at
/BuildRoot/Library/Caches/com.apple.xbs/Sources/rsync/rsync-47/rsync/io.c(453)
[sender=2.6.9]
$ mkdir -p Chrome.app/PlugIns/today_extension.appex
$  rsync --recursive --perms --links today_extension.appex/
Chrome.app/PlugIns/today_extension.appex
$  find Chrome.app/PlugIns -type d|head
Chrome.app/PlugIns
Chrome.app/PlugIns/today_extension.appex
Chrome.app/PlugIns/today_extension.appex/_CodeSignature
Chrome.app/PlugIns/today_extension.appex/ar.lproj

All that makes me a bit scarred of using rsync.

[Robert Sesek, 2016-06-17 17:33:04]

On 2016/06/17 17:16:37, sdefresne wrote:
> On 2016/06/17 17:03:31, Robert Sesek wrote:
> > opinion. I went with rsync rather than re-implementing shutil.copytree
because
> > 1) we already use rsync in a few other build scripts, and 2) it may be a
> little
> > bit faster.
> 
> I did try to use rsync and I've failed multiple time to pass the correct
> options, and this is why I've decided to recode shutil.copy. I see in your CL
> that you strip the trailing / in the source path, but I'm unsure whether this
is
> correct or not.
> 

It is not correct, you're right. We should actually be *adding* trailing slash
not stripping it (I got that swapped in my head for the CL). I haven't uploaded
a new patch set yet because I want to send the broken patch to the trybots after
the versioning change lands to verify that it would be caught in the future.

> The correct rsync command needs the trailing / I think but requires that the
> destination directory exists.

Yup. Totally reversed the correct thing in the CL.

> $  rsync --recursive --perms --links today_extension.appex/
> Chrome.app/PlugIns/today_extension.appex
> rsync: mkdir "Chrome.app/PlugIns/today_extension.appex" failed: No such file
or
> directory (2)
> rsync error: error in file IO (code 11) at
>
/BuildRoot/Library/Caches/com.apple.xbs/Sources/rsync/rsync-47/rsync/main.c(544)
> [receiver=2.6.9]
> rsync: connection unexpectedly closed (8 bytes received so far) [sender]
> rsync error: error in rsync protocol data stream (code 12) at
> /BuildRoot/Library/Caches/com.apple.xbs/Sources/rsync/rsync-47/rsync/io.c(453)
> [sender=2.6.9]
> $ mkdir -p Chrome.app/PlugIns/today_extension.appex
> $  rsync --recursive --perms --links today_extension.appex/
> Chrome.app/PlugIns/today_extension.appex
> $  find Chrome.app/PlugIns -type d|head
> Chrome.app/PlugIns
> Chrome.app/PlugIns/today_extension.appex
> Chrome.app/PlugIns/today_extension.appex/_CodeSignature
> Chrome.app/PlugIns/today_extension.appex/ar.lproj
> 
> All that makes me a bit scarred of using rsync.

Hmm, is that because PlugIns doesn't exist?

[sdefresne, 2016-06-18 07:28:29]

Closing as your CLs landed. Thank you.