Pass resources to dependents as zip files instead of directories

Pass resources to dependents as zip files instead of directories

This makes all temporary work be done in temporary directories.

This change is particularly helpful for 2 reasons: first, it makes it
more difficult to accidentally include stale, unwanted files during an
incremental build. Second, it is easier to trigger dependent actions
(zip file timestamps should be updated when their contents change, while
the same is not true for directories).

Makes the output of build/java_strings_grd.gypi be a zipfile containing
the resources.

BUG=359249, 375431
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276226

[cjhopman, 2014-06-06 18:12:07]

newt: *

[newt (away), 2014-06-09 18:47:32]

A couple small comments. Also, what's the impact of this change on build times?
Zipping without compression and with a warm cache should be pretty quick, but
it's worth checking.

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/process_re...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/process_re...
build/android/gyp/process_resources.py:38: parser.add_option('--resource-dirs',
help='DO NOT COMMIT')
DO NOT COMMIT
?

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/util/build...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/util/build...
build/android/gyp/util/build_utils.py:184: outfile.write(f, os.path.relpath(f,
base_dir))
it's probably a good idea to check here that f lives somewhere inside base_dir.
Perhaps:

    rel_path = os.path.relpath(f, base_dir)
    if rel_path != os.path.normpath(rel_path):
        # problemo!

https://codereview.chromium.org/321463002/diff/1/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode128
build/java.gypi:128: # Dependent jars include this target's R.java file via
update comment

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode137
build/java.gypi:137: # additional_res_dirs, additional_res_packages, and
update comment

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode170
build/java.gypi:170: '<(R_stamp)',
do we still need this stamp file?

https://codereview.chromium.org/321463002/diff/1/build/java_strings_grd.gypi
File build/java_strings_grd.gypi (right):

https://codereview.chromium.org/321463002/diff/1/build/java_strings_grd.gypi#...
build/java_strings_grd.gypi:29: 'grit_stamp':
'<(res_grit_dir)/values/android_strings.xml',
unused?

[cjhopman, 2014-06-09 21:59:38]

Worst case for both zipping+unzipping steps(in a couple quick tests on an ssd),
this adds ~5-8% to process_resources steps. That's a total of ~.6 seconds over
all such steps.

Maybe in the future we will want to get that time back, but there are much lower
hanging fruit and it isn't really that much of the full build (36 seconds). In
gyp, all the process_resources steps are on the critical path (in the case of
just editing a resource), in GN they won't be.

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/process_re...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/process_re...
build/android/gyp/process_resources.py:38: parser.add_option('--resource-dirs',
help='DO NOT COMMIT')
On 2014/06/09 18:47:31, newt wrote:
> DO NOT COMMIT
> ?

Done.

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/util/build...
File build/android/gyp/util/build_utils.py (right):

https://codereview.chromium.org/321463002/diff/1/build/android/gyp/util/build...
build/android/gyp/util/build_utils.py:184: outfile.write(f, os.path.relpath(f,
base_dir))
On 2014/06/09 18:47:31, newt wrote:
> it's probably a good idea to check here that f lives somewhere inside
base_dir.
> Perhaps:
> 
>     rel_path = os.path.relpath(f, base_dir)
>     if rel_path != os.path.normpath(rel_path):
>         # problemo!

Done.

https://codereview.chromium.org/321463002/diff/1/build/java.gypi
File build/java.gypi (right):

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode128
build/java.gypi:128: # Dependent jars include this target's R.java file via
On 2014/06/09 18:47:31, newt wrote:
> update comment

Done.

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode137
build/java.gypi:137: # additional_res_dirs, additional_res_packages, and
On 2014/06/09 18:47:31, newt wrote:
> update comment

Done.

https://codereview.chromium.org/321463002/diff/1/build/java.gypi#newcode170
build/java.gypi:170: '<(R_stamp)',
On 2014/06/09 18:47:31, newt wrote:
> do we still need this stamp file?

Done.

https://codereview.chromium.org/321463002/diff/1/build/java_strings_grd.gypi
File build/java_strings_grd.gypi (right):

https://codereview.chromium.org/321463002/diff/1/build/java_strings_grd.gypi#...
build/java_strings_grd.gypi:29: 'grit_stamp':
'<(res_grit_dir)/values/android_strings.xml',
On 2014/06/09 18:47:31, newt wrote:
> unused?

Done.

[newt (away), 2014-06-09 23:38:07]

One comment, then lgtm.

Timing impact seems reasonable. Glad to hear that GN will be able to parallelize
these steps :)

https://codereview.chromium.org/321463002/diff/20001/build/android/gyp/proces...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/321463002/diff/20001/build/android/gyp/proces...
build/android/gyp/process_resources.py:39: help='Directories containing
resources of this document.')
"this document"?

[cjhopman, 2014-06-10 15:55:12]

https://codereview.chromium.org/321463002/diff/20001/build/android/gyp/proces...
File build/android/gyp/process_resources.py (right):

https://codereview.chromium.org/321463002/diff/20001/build/android/gyp/proces...
build/android/gyp/process_resources.py:39: help='Directories containing
resources of this document.')
On 2014/06/09 23:38:07, newt wrote:
> "this document"?

Done.

[cjhopman, 2014-06-10 15:55:14]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:59:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cjhopman@chromium.org/321463002/40001

[commit-bot: I haz the power, 2014-06-10 23:38:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)

[commit-bot: I haz the power, 2014-06-11 01:20:58]

Change committed as 276226