Make changes to mini_installer test files trigger tests.

Make changes to mini_installer test files trigger tests.

BUG=413998
R=dpranke@chromium.org

Committed: https://crrev.com/b4bec9b0d5d9b31334946d2f714addffbf2746c6
Cr-Commit-Position: refs/heads/master@{#317667}

[Dirk Pranke, 2015-02-18 03:57:31]

lgtm

[jam, 2015-02-18 08:30:55]

please hold off on landing this, see thread

[grt (UTC plus 2), 2015-02-20 21:06:40]

New patchsets have been uploaded after l-g-t-m from dpranke@chromium.org

[grt (UTC plus 2), 2015-02-20 21:08:48]

Patchset #2 (id:20001) has been deleted

[grt (UTC plus 2), 2015-02-20 21:10:00]

On 2015/02/18 08:30:55, jam wrote:
> please hold off on landing this, see thread

What do you think of the latest patch set (module the missing file header,
comments, etc)? It should rebuild mini_installer when any of the test sources
change, which I think will be enough to make the recipe run the test. Thanks for
taking a look.

[jam, 2015-02-23 03:20:40]

there are not many python files. can we list them in a gypi target instead of
this script? I think in general the less scripts that run for a build, the
better.

[jam, 2015-02-23 03:24:36]

jam@chromium.org changed reviewers:
+ brettw@chromium.org, jam@chromium.org

[jam, 2015-02-23 03:24:37]

+Brett for his thoughts as well

[brettw, 2015-02-23 18:25:21]

https://codereview.chromium.org/935913002/diff/60001/chrome/installer/mini_in...
File chrome/installer/mini_installer.gyp (right):

https://codereview.chromium.org/935913002/diff/60001/chrome/installer/mini_in...
chrome/installer/mini_installer.gyp:32:
'<!@pymod_do_main(list_test_installer_sources ../test/mini_installer)',
I don't think we should be doing filesystem operations in the middle of the
metabuild step. This brings the whole metabuild operation to a halt while some
Python code goes and paws around the disk.

While the GYP runtimes are such a disaster that it won't matter, there's
something to be said for not making it worse. And this type of thing makes a
really big difference in runtime in GN where the metabuild performance problems
are fixed. I've gone through some effort to remove these kind of thing in the GN
build.

If this code seldom changes, I think it's OK to just list the files manually,
and maybe add a README to the directory(s) in question for people that happen to
be adding new files to not forget to update the list.

If this is more frequently changing or you're more paranoid, what I've done is
list the files but then write an action that runs at build time that enumerates
the files and then compares them to the hardcoded list. This action would
produce a stamp file on success, or fail if the lists don't match.

This approach has the advantage that you're spending time during the slow build
step, and then only for people compiling mini installer, rather than taking time
every time any engineer syncs or updates a build file.

[Dirk Pranke, 2015-02-23 19:20:59]

In addition to what Brett said, this is functionally equivalent to putting a
glob into GYP, which is something we've historically been against doing.

If we can make things work by just listing a set of files in GYP/GN, I think
that's the best and easiest thing to do for now.

[grt (UTC plus 2), 2015-02-23 20:29:57]

Patchset #4 (id:80001) has been deleted

[grt (UTC plus 2), 2015-02-23 20:31:36]

Thanks for the feedback. I've switched to listing the test files in a .gypi
alongside them, which is included by mini_installer.gyp. How does this sit?

[Dirk Pranke, 2015-02-23 20:36:35]

this more-or-less lgtm.

[grt (UTC plus 2), 2015-02-23 20:38:51]

On 2015/02/23 20:36:35, Dirk Pranke wrote:
> this more-or-less lgtm.

Do you have suggestions to make it even more lgtm? :-)

[Dirk Pranke, 2015-02-23 20:40:47]

On 2015/02/23 20:38:51, grt wrote:
> On 2015/02/23 20:36:35, Dirk Pranke wrote:
> > this more-or-less lgtm.
> 
> Do you have suggestions to make it even more lgtm? :-)

No, I only wrote that because I'm not enough of a GYP guru to be sure 
that there isn't a better way to do what you want, and of course, I don't 
know what all the list of files in the gypi really should be, so I'm 
rubber-stamping that part.

brettw@ or jam@ might have opinions on the gyp change, though.

[brettw, 2015-02-23 22:06:14]

lgtm

[jam, 2015-02-23 22:11:41]

On 2015/02/23 22:06:14, brettw wrote:
> lgtm

i defer to brett, i assume this does what he suggested

thanks for the iteration to get it to this place; i'll point people towards this
method for other cases like this.

[grt (UTC plus 2), 2015-02-23 22:17:52]

On 2015/02/23 22:11:41, jam wrote:
> On 2015/02/23 22:06:14, brettw wrote:
> > lgtm
> 
> i defer to brett, i assume this does what he suggested
> 
> thanks for the iteration to get it to this place; i'll point people towards
this
> method for other cases like this.

Great, thanks.

[grt (UTC plus 2), 2015-02-23 22:18:00]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-02-23 22:19:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/935913002/100001

[brettw, 2015-02-23 22:38:12]

jam: this patch hardcodes the list and doesn't do the 2nd stage verification.
This is fine with me, but isn't the more complex solution I was suggesting for
more frequently changing code.

[commit-bot: I haz the power, 2015-02-23 22:41:11]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-02-23 22:43:36]

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