Building cronet targets multiple times now reports "no work to do"

Building cronet targets multiple times now reports "no work to do"

BUG=646165
Committed: https://crrev.com/e2173d0f620afa9b75fb1414c4f7cf2095c3164b
Cr-Commit-Position: refs/heads/master@{#418742}

[agrieve, 2016-09-14 19:25:43]

agrieve@chromium.org changed reviewers:
+ xunjieli@chromium.org

[agrieve, 2016-09-14 19:26:14]

Note: tested with same targets the bots build:
ninja cronet_package cronet_perf_test_apk cronet_sample_test_apk
cronet_test_instrumentation_apk cronet_unittests cronet_unittests_apk
net_unittests net_unittests_apk

[agrieve, 2016-09-14 19:27:41]

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

[commit-bot: I haz the power, 2016-09-14 19:28:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-09-14 19:43:42]

Very cool! one question below.

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
File components/cronet/tools/jar_src.py (right):

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
components/cronet/tools/jar_src.py:78: # Srcjar deps already captured in GN
rules (no need to list them here).
Srcjar deps aren't explicitly listed in the GN rule, that's why we do
build_utils.WriteDepfile() here right?
Or are you suggesting we do not need this step at all?

[agrieve, 2016-09-14 19:59:40]

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
File components/cronet/tools/jar_src.py (right):

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
components/cronet/tools/jar_src.py:78: # Srcjar deps already captured in GN
rules (no need to list them here).
On 2016/09/14 19:43:42, xunjieli wrote:
> Srcjar deps aren't explicitly listed in the GN rule, that's why we do
> build_utils.WriteDepfile() here right?
> Or are you suggesting we do not need this step at all?

This change marks the srcjar_deps as inputs to the action():
inputs = invoker.src_jars

[xunjieli, 2016-09-14 20:02:17]

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
File components/cronet/tools/jar_src.py (right):

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
components/cronet/tools/jar_src.py:78: # Srcjar deps already captured in GN
rules (no need to list them here).
On 2016/09/14 19:59:40, agrieve wrote:
> On 2016/09/14 19:43:42, xunjieli wrote:
> > Srcjar deps aren't explicitly listed in the GN rule, that's why we do
> > build_utils.WriteDepfile() here right?
> > Or are you suggesting we do not need this step at all?
> 
> This change marks the srcjar_deps as inputs to the action():
> inputs = invoker.src_jars

If the dependency is added, can we get rid of line 73 - 79 then?

[agrieve, 2016-09-14 20:33:03]

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
File components/cronet/tools/jar_src.py (right):

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
components/cronet/tools/jar_src.py:78: # Srcjar deps already captured in GN
rules (no need to list them here).
On 2016/09/14 20:02:16, xunjieli wrote:
> On 2016/09/14 19:59:40, agrieve wrote:
> > On 2016/09/14 19:43:42, xunjieli wrote:
> > > Srcjar deps aren't explicitly listed in the GN rule, that's why we do
> > > build_utils.WriteDepfile() here right?
> > > Or are you suggesting we do not need this step at all?
> > 
> > This change marks the srcjar_deps as inputs to the action():
> > inputs = invoker.src_jars
> 
> If the dependency is added, can we get rid of line 73 - 79 then?

The dependency in GN is only for generated files. We still need to list the
non-generated files in the depfile (or switch to listing them in GN. Right now
we just pass this script a directory)

[xunjieli, 2016-09-14 20:34:50]

LGTM

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
File components/cronet/tools/jar_src.py (right):

https://codereview.chromium.org/2341903002/diff/1/components/cronet/tools/jar...
components/cronet/tools/jar_src.py:78: # Srcjar deps already captured in GN
rules (no need to list them here).
On 2016/09/14 20:33:03, agrieve wrote:
> On 2016/09/14 20:02:16, xunjieli wrote:
> > On 2016/09/14 19:59:40, agrieve wrote:
> > > On 2016/09/14 19:43:42, xunjieli wrote:
> > > > Srcjar deps aren't explicitly listed in the GN rule, that's why we do
> > > > build_utils.WriteDepfile() here right?
> > > > Or are you suggesting we do not need this step at all?
> > > 
> > > This change marks the srcjar_deps as inputs to the action():
> > > inputs = invoker.src_jars
> > 
> > If the dependency is added, can we get rid of line 73 - 79 then?
> 
> The dependency in GN is only for generated files. We still need to list the
> non-generated files in the depfile (or switch to listing them in GN. Right now
> we just pass this script a directory)

Acknowledged. I see.

[commit-bot: I haz the power, 2016-09-14 20:48:48]

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

[commit-bot: I haz the power, 2016-09-14 20:48:49]

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_...)

[agrieve, 2016-09-15 00:29:07]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-09-15 00:30:44]

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, 2016-09-15 01:16:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-15 01:18:15]

Description was changed from

==========
Building cronet targets multiple times now reports "no work to do"

BUG=646165
==========

to

==========
Building cronet targets multiple times now reports "no work to do"

BUG=646165

Committed: https://crrev.com/e2173d0f620afa9b75fb1414c4f7cf2095c3164b
Cr-Commit-Position: refs/heads/master@{#418742}
==========

[commit-bot: I haz the power, 2016-09-15 01:18:16]

Patchset 1 (id:??) landed as
https://crrev.com/e2173d0f620afa9b75fb1414c4f7cf2095c3164b
Cr-Commit-Position: refs/heads/master@{#418742}