Changes to Chrome resource files don't get picked up by ninja

[akalin]

Repro steps:

1. In a Chrome checkout, edit chrome/browser/resources/sync_internals/sync_index.html (add a comment or something)
2. Do ninja -j100 chrome

Expected results:
stuff in chrome/chrome_extra_resources.ninja gets built, and chrome is rebuilt with the updated resources.

Actual results:
ninja does nothing

[evmar]

Lols. After lots of head scratching I find this comment I wrote to myself:
# XXX need to add extra dependencies on rule inputs
# (e.g. if generator program changes, we need to rerun)

[evmar]

Fixed in the current gyp.patch.

[akalin]

This seems to have regressed. Following the original repro steps, ninja does stuff, but ninja/resources.pak isn't changed unless I explicitly say 'ninja -j100 packed_extra_resources':

(edit sync_index.html to add a comment at the top)

$ ninja -j100 -v chrome 
ninja: WARNING: multiple rules generate ninja/gen/libraries.o. build will not be correct; continuing anyway
ninja: WARNING: multiple rules generate ninja/gen/experimental-libraries.o. build will not be correct; continuing anyway
ACTION Generating resources from browser/resources/sync_internals_resources.grd
STAMP ninja/obj/chrome_extra_resources/actions.stamp
STAMP ninja/obj/chrome_extra_resources/chrome/chrome_extra_resources.stamp
STAMP ninja/obj/browser/predepends.stamp
CXX ninja/obj/browser/chrome/browser/ui/webui/sync_internals_html_source.o
AR ninja/obj/browser/chrome/libbrowser.a
LINK ninja/chrome

(opening about:sync and viewing source doesn't show the added comment)

$ ninja -j100 -v packed_extra_resources
ninja: WARNING: multiple rules generate ninja/gen/libraries.o. build will not be correct; continuing anyway
ninja: WARNING: multiple rules generate ninja/gen/experimental-libraries.o. build will not be correct; continuing anyway
touch ninja/obj/packed_extra_resources/predepends.stamp
cd chrome; python ../tools/data_pack/repack.py "../ninja//resources.pak" "../ninja/gen/chrome/component_extension_resources.pak" "../ninja/gen/chrome/devtools_resources.pak" "../ninja/gen/chrome/net_internals_resources.pak" "../ninja/gen/chrome/shared_resources.pak" "../ninja/gen/chrome/sync_internals_resources.pak"
touch ninja/obj/packed_extra_resources/actions.stamp
touch ninja/obj/packed_extra_resources/chrome/packed_extra_resources.stamp

(now it does)

[evmar]

After some fiddling, I have gotten the world to this state:

$ touch chrome/browser/resources/sync_internals/sync_index.html
$ n chrome | cat
[1/10] ACTION Generating resources from browser/resources/sync_internals_resources.grd
[2/10] STAMP ninja/obj/chrome/chrome_extra_resources_actions_rules_copies.stamp
[3/10] STAMP ninja/obj/chrome/chrome_extra_resources_chrome_extra_resources.stamp
[4/10] STAMP ninja/obj/chrome/packed_extra_resources_predepends.stamp
[5/10] STAMP ninja/obj/chrome/browser_predepends.stamp
[6/10] ACTION packed_extra_resources: repack_resources
[7/10] CXX ninja/obj/chrome/browser/ui/webui/browser_sync_internals_html_source.o
[8/10] STAMP ninja/obj/chrome/packed_extra_resources_packed_extra_resources.stamp
[9/10] AR ninja/obj/chrome/browser_libbrowser.a
[10/10] LINK ninja/chrome

I believe the reason it does all the extra work (including relinking) is that grit touches the header file used by that internals_html_source, which causes it to recompile, which causes all the relinking downstream.

What do you think, is that desirable behavior?

[akalin]

It's a shame that grit ends up touching the header file, triggering a link. But I think that's a problem with grit and not ninja.

Thanks for looking into this!

[evmar]

If grit didn't touch the header file, then we would always think the header file is out of date and would re-run grit every time you build.

[evmar]

I believe this actually works already with ninja trunk, due to the modification to the header.

evmar:/work/chrome/src$ n chrome | cat
real 1.17
user 0.99
sys 0.17
evmar:/work/chrome/src$ touch chrome/browser/resources/sync_internals/sync_index.html 
evmar:/work/chrome/src$ n chrome | cat
[1/7] ACTION Generating resources from browser/resources/sync_internals_resources.grd
[2/7] STAMP ninja/obj/chrome/chrome_extra_resources_actions_rules_copies.stamp
[3/7] STAMP ninja/obj/chrome/chrome_extra_resources_chrome_extra_resources.stamp
[4/7] STAMP ninja/obj/chrome/browser_predepends.stamp
[5/7] CXX ninja/obj/chrome/browser/ui/webui/browser_sync_internals_html_source.o
[6/7] AR ninja/obj/chrome/browser_libbrowser.a
[7/7] LINK ninja/chrome
real 10.97

This makes it difficult for me to judge whether my more complicated "fix" is warranted.

[evmar]

Wait, I take that back -- it's not rebuilding the pak file.

[evmar]

And I've confirmed my patch does fix this. Now to just convince myself it doesn't regress anything... :\

[evmar]

Fix pushed.