Avoid too long command lines.

Avoid too long command lines.

The hash is calculated for the list of the resource files to determine if the
files were changed (added, removed, renamed, etc). The current way of doing it
may explode the command line length over the system limit. To avoid it, the list
of the input files is written to a temporary file on disk and the file is used
to calculate the checksum.

BUG=177552
R=cjhopman@chromium.org, newt@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213849

[whywhat, 2013-07-25 14:04:28]

PTaL

[cjhopman, 2013-07-25 15:35:17]

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
I think this file is written into the source tree, does .gitignore need any
entries for it? I didn't see any in a quick look for the usage of this gyp thing
in WebKit so maybe its not needed.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
Since inputs contains things that are late expanded (use > instead of <), both
the expansions here probably need to be late.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode186
build/java.gypi:186: '--ignore=>!(cat <(inputs_list_file) | md5sum)',
inputs_list_file might need to be late expanded like above.

Also, md5sum can take the file as its argument instead of cat-ing the file and
piping it.

[newt (away), 2013-07-25 17:55:13]

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 15:35:17, cjhopman wrote:
> I think this file is written into the source tree, does .gitignore need any
> entries for it? I didn't see any in a quick look for the usage of this gyp
thing
> in WebKit so maybe its not needed.

Could we put this in the out/ folder? in
<(intermediate_dir)/process_resources_inputs or something similar.

[cjhopman, 2013-07-25 18:05:25]

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 17:55:13, newt wrote:
> On 2013/07/25 15:35:17, cjhopman wrote:
> > I think this file is written into the source tree, does .gitignore need any
> > entries for it? I didn't see any in a quick look for the usage of this gyp
> thing
> > in WebKit so maybe its not needed.
> 
> Could we put this in the out/ folder? in
> <(intermediate_dir)/process_resources_inputs or something similar.

You can't write to the output directory at gyp time (it's not defined until
build time). I don't know if you even have access to some root of the output
directories.

[newt (away), 2013-07-25 18:11:39]

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 18:05:26, cjhopman wrote:
> On 2013/07/25 17:55:13, newt wrote:
> > On 2013/07/25 15:35:17, cjhopman wrote:
> > > I think this file is written into the source tree, does .gitignore need
any
> > > entries for it? I didn't see any in a quick look for the usage of this gyp
> > thing
> > > in WebKit so maybe its not needed.
> > 
> > Could we put this in the out/ folder? in
> > <(intermediate_dir)/process_resources_inputs or something similar.
> 
> You can't write to the output directory at gyp time (it's not defined until
> build time). I don't know if you even have access to some root of the output
> directories.
> 

what happens if you try to write to the output directory at gyp time?
<intermediate_dir) has a valid value...

in any case, I imagine we need to write to separate files to keep parallel gyp
from exploding.

[cjhopman, 2013-07-25 18:17:49]

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 18:11:40, newt wrote:
> On 2013/07/25 18:05:26, cjhopman wrote:
> > On 2013/07/25 17:55:13, newt wrote:
> > > On 2013/07/25 15:35:17, cjhopman wrote:
> > > > I think this file is written into the source tree, does .gitignore need
> any
> > > > entries for it? I didn't see any in a quick look for the usage of this
gyp
> > > thing
> > > > in WebKit so maybe its not needed.
> > > 
> > > Could we put this in the out/ folder? in
> > > <(intermediate_dir)/process_resources_inputs or something similar.
> > 
> > You can't write to the output directory at gyp time (it's not defined until
> > build time). I don't know if you even have access to some root of the output
> > directories.
> > 
> 
> what happens if you try to write to the output directory at gyp time?
> <intermediate_dir) has a valid value...
> 
> in any case, I imagine we need to write to separate files to keep parallel gyp
> from exploding.

What happens is generator dependent, on ninja it expands to something like:
$!PRODUCT_DIR/chrome_java

But, this makes me notice that the filename here should definitely incorporate
the target_name otherwise multiple java targets in the same gyp file (or with
gyp files in the same directory) will be writing to the same inputs list file.

[whywhat, 2013-07-25 20:01:48]

PTAL

What's the way to properly test this locally?
The generated .tmp files seem to have the list indeed.
android_gyp doesn't fail and chrome is building fine. android_clang_dbg trybot
also passes.

How do I verify that checksum does what it needs to? Should I just remove some
resource file and observe that after android_gyp the resources are processed
again?

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

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 15:35:17, cjhopman wrote:
> I think this file is written into the source tree, does .gitignore need any
> entries for it? I didn't see any in a quick look for the usage of this gyp
thing
> in WebKit so maybe its not needed.

.tmp files were in the untracked section of my git status.
Added a .gitignore entry for *.tmp

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 15:35:17, cjhopman wrote:
> Since inputs contains things that are late expanded (use > instead of <), both
> the expansions here probably need to be late.

Done.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 18:17:50, cjhopman wrote:
> On 2013/07/25 18:11:40, newt wrote:
> > On 2013/07/25 18:05:26, cjhopman wrote:
> > > On 2013/07/25 17:55:13, newt wrote:
> > > > On 2013/07/25 15:35:17, cjhopman wrote:
> > > > > I think this file is written into the source tree, does .gitignore
need
> > any
> > > > > entries for it? I didn't see any in a quick look for the usage of this
> gyp
> > > > thing
> > > > > in WebKit so maybe its not needed.
> > > > 
> > > > Could we put this in the out/ folder? in
> > > > <(intermediate_dir)/process_resources_inputs or something similar.
> > > 
> > > You can't write to the output directory at gyp time (it's not defined
until
> > > build time). I don't know if you even have access to some root of the
output
> > > directories.
> > > 
> > 
> > what happens if you try to write to the output directory at gyp time?
> > <intermediate_dir) has a valid value...
> > 
> > in any case, I imagine we need to write to separate files to keep parallel
gyp
> > from exploding.
> 
> What happens is generator dependent, on ninja it expands to something like:
> $!PRODUCT_DIR/chrome_java
> 
> But, this makes me notice that the filename here should definitely incorporate
> the target_name otherwise multiple java targets in the same gyp file (or with
> gyp files in the same directory) will be writing to the same inputs list file.
> 

Done.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
On 2013/07/25 18:11:40, newt wrote:
> On 2013/07/25 18:05:26, cjhopman wrote:
> > On 2013/07/25 17:55:13, newt wrote:
> > > On 2013/07/25 15:35:17, cjhopman wrote:
> > > > I think this file is written into the source tree, does .gitignore need
> any
> > > > entries for it? I didn't see any in a quick look for the usage of this
gyp
> > > thing
> > > > in WebKit so maybe its not needed.
> > > 
> > > Could we put this in the out/ folder? in
> > > <(intermediate_dir)/process_resources_inputs or something similar.
> > 
> > You can't write to the output directory at gyp time (it's not defined until
> > build time). I don't know if you even have access to some root of the output
> > directories.
> > 
> 
> what happens if you try to write to the output directory at gyp time?
> <intermediate_dir) has a valid value...
> 
> in any case, I imagine we need to write to separate files to keep parallel gyp
> from exploding.

Done.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
Ack

On 2013/07/25 18:05:26, cjhopman wrote:
> On 2013/07/25 17:55:13, newt wrote:
> > On 2013/07/25 15:35:17, cjhopman wrote:
> > > I think this file is written into the source tree, does .gitignore need
any
> > > entries for it? I didn't see any in a quick look for the usage of this gyp
> > thing
> > > in WebKit so maybe its not needed.
> > 
> > Could we put this in the out/ folder? in
> > <(intermediate_dir)/process_resources_inputs or something similar.
> 
> You can't write to the output directory at gyp time (it's not defined until
> build time). I don't know if you even have access to some root of the output
> directories.
>

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode159
build/java.gypi:159: 'inputs_list_file': '<|(inputs_list.tmp <@(_inputs))'
Ack

On 2013/07/25 17:55:13, newt wrote:
> On 2013/07/25 15:35:17, cjhopman wrote:
> > I think this file is written into the source tree, does .gitignore need any
> > entries for it? I didn't see any in a quick look for the usage of this gyp
> thing
> > in WebKit so maybe its not needed.
> 
> Could we put this in the out/ folder? in
> <(intermediate_dir)/process_resources_inputs or something similar.

https://codereview.chromium.org/20313002/diff/1/build/java.gypi#newcode186
build/java.gypi:186: '--ignore=>!(cat <(inputs_list_file) | md5sum)',
On 2013/07/25 15:35:17, cjhopman wrote:
> inputs_list_file might need to be late expanded like above.
> 
> Also, md5sum can take the file as its argument instead of cat-ing the file and
> piping it.

Done.

[newt (away), 2013-07-25 20:09:27]

You can test this by gyping and building, then removing a resource file,
re-gyping and re-building. Be sure that process resources is run when
re-building. As a sanity check, write a constant value (e.g. "foo" instead of
">@(_inputs)") to the inputs_list file and verify that process resources is
*not* run when re-building.

[whywhat, 2013-07-25 21:26:35]

Thanks, I tested and it works on my machine. I've changed the file extension to
.gypcmd since it pleases the .gitignore gods.
Good to commit? :)

[newt (away), 2013-07-25 21:30:55]

lgtm

[cjhopman, 2013-07-25 21:33:09]

lgtm

[commit-bot: I haz the power, 2013-07-26 03:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avayvod@chromium.org/20313002/18001

[whywhat, 2013-07-26 11:46:47]

Committed patchset #3 manually as r213849 (presubmit successful).