[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

Since timestamp of directory is not always updated when the content
is changed (e.g. when a file content is changed, ...) listing just
the directory lead to flaky incremental builds and bot failures.

Explicitly list all test data files to fix those flakes in net.gypi
and use those list in both gyp and gn. Document how the list have
been generated inline in net.gypi.

BUG=459705
Committed: https://crrev.com/04f9114d3168d9e59830a22d68547e06dbf9409e
Cr-Commit-Position: refs/heads/master@{#388725}

[sdefresne, 2016-04-18 17:14:41]

sdefresne@chromium.org changed reviewers:
+ davidben@chromium.org

[sdefresne, 2016-04-18 17:14:41]

Please take a look.

See https://codereview.chromium.org/1889483003 for explanation of why it is
required to explicitly list individual files instead of listing the containing
directory.

[davidben, 2016-04-18 18:02:04]

davidben@chromium.org changed reviewers:
+ eroman@chromium.org, rsleevi@chromium.org

[davidben, 2016-04-18 18:02:04]

Hrmf, that's a nuisance. Why does GN even support depending on directories if it
doesn't work right? Isolate files, in contrast, allow globbing.

+rsleevi,eroman who are normally the ones adding to those directories. If this
is how it is to work, that seems fine to me, but I'll leave it to them.

[Ryan Sleevi, 2016-04-18 18:07:49]

I didn't see an explanation why we couldn't/shouldn't fix GN. Did I miss it?

[eroman, 2016-04-18 18:20:54]

+1

If this is specifically an implementation issue with GN + iOS rather than an
architectural decision by GN to disallow data directory dependencies, an iOS
specific hack like copying the entire directory always seems like a better fit.

[sdefresne, 2016-04-19 08:27:38]

sdefresne@chromium.org changed reviewers:
+ brettw@chromium.org

[sdefresne, 2016-04-19 08:27:38]

bundle_data does support listing a directory. This ends up generating a ninja
build rule that a directory as source. When running the build, ninja has no
knowledge of directories and just check the timestamp that is not updated when
content in sub-folders is updated, causing flake during the build. So a fix
should go into "ninja" not "gn".

The isolate files work differently. They are used post build by script that do
not optimise for not duplicating file copy. They are also run after every build
on the bots, so they can use globbing as the file they list won't change before
the list is used.

It is possible to use exec_script in BUILD.gn to list the content of net/data
and to avoid listing all the files, but this will still leave the opportunity
for flakyness. The exec_script will only collect the list of file when running
"gn gen", not during the build. So, if a developer adds a new data file, change
a source file then it will not be found in the generated application bundle on
iOS (as "gn gen" is only automatically re-run by ninja if a BUILD.gn file is
modified). Said developer may be surprised that the new file is not picked up.
If the files have to be listed individually, then there is no such surprise.

Using exec_script may cause less failures than using a directory as input though
as 1. BUILD.gn frequently changes so "gn gen" will eventually be run on
developer checkout, 2. I think the bot unconditionally run "gn gen" as part of
the build.

+brettw: what do you think about using exec_script to collect the list of data
files to copy in the application bundle on iOS versus explicitly listing them in
BUILD.gn?

[brettw, 2016-04-19 18:16:12]

On 2016/04/19 08:27:38, sdefresne wrote:
> bundle_data does support listing a directory. This ends up generating a ninja
> build rule that a directory as source. When running the build, ninja has no
> knowledge of directories and just check the timestamp that is not updated when
> content in sub-folders is updated, causing flake during the build. So a fix
> should go into "ninja" not "gn".
> 
> The isolate files work differently. They are used post build by script that do
> not optimise for not duplicating file copy. They are also run after every
build
> on the bots, so they can use globbing as the file they list won't change
before
> the list is used.
> 
> It is possible to use exec_script in BUILD.gn to list the content of net/data
> and to avoid listing all the files, but this will still leave the opportunity
> for flakyness. The exec_script will only collect the list of file when running
> "gn gen", not during the build. So, if a developer adds a new data file,
change
> a source file then it will not be found in the generated application bundle on
> iOS (as "gn gen" is only automatically re-run by ninja if a BUILD.gn file is
> modified). Said developer may be surprised that the new file is not picked up.
> If the files have to be listed individually, then there is no such surprise.
> 
> Using exec_script may cause less failures than using a directory as input
though
> as 1. BUILD.gn frequently changes so "gn gen" will eventually be run on
> developer checkout, 2. I think the bot unconditionally run "gn gen" as part of
> the build.
> 
> +brettw: what do you think about using exec_script to collect the list of data
> files to copy in the application bundle on iOS versus explicitly listing them
in
> BUILD.gn?

Right, the only way to guarantee correctness (in both GYP and GN) is to list the
files out in the build and pass them to a command line to an action or
something. In all other cases one can generate a situation where something
unexpected can happen (which often translate into test flakes).

I am strongly opposed to exec_script calls in general. These were abused to no
end in the GYP build which is why the GN build has a whitelist for force a
toplevel owners review for new files to use it. They're also slow (especially on
Windows) and doing globs can be mysterious (you can get unversioned files in
random directories and they end up in the build accidentally). You could
certainly argue that adding one call won't be much slower and the way you use
the data will be unlikely to have mysterious errors. But once we start doing
this kind of thing, running GN will actually get much slower and such mysterious
errors do actually happen in practice. As an example of what can go wrong, the
angle team was using globs for *source files* until I made them stop.

[sdefresne, 2016-04-19 19:01:27]

davidben/eroman/rsleevi: what do you think about this CL given the extra
information?

[eroman, 2016-04-19 19:21:10]

Previously "data/ssl/certificates" was a data dependency of //net:test_support,
whereas now it is just a data dependency of //net:net_unittests.

There are targets outside of //net that depend on //net:test_support -- have you
confirmed that nothing in //net:test_support was actually depending on those
data files?

[sdefresne, 2016-04-20 07:17:25]

On 2016/04/19 at 19:21:10, eroman wrote:
> Previously "data/ssl/certificates" was a data dependency of
//net:test_support, whereas now it is just a data dependency of
//net:net_unittests.
> 
> There are targets outside of //net that depend on //net:test_support -- have
you confirmed that nothing in //net:test_support was actually depending on those
data files?

No source file in //net:test_support directly load any data from
//net/data/ssl/certificates. They are only loaded by tests in net_unittests and
one test in components_unittests. Since the individual unit test have to list
the data filename in their source code, I figured that it was better to also
have the individual test target list the files they use (for
components_unittests, this is already done, see
https://codereview.chromium.org/1903443002/diff/80001/components/security_sta...).

[Ryan Sleevi, 2016-04-20 08:01:21]

On 2016/04/20 07:17:25, sdefresne wrote:
> No source file in //net:test_support directly load any data from
> //net/data/ssl/certificates. They are only loaded by tests in net_unittests
and
> one test in components_unittests. Since the individual unit test have to list
> the data filename in their source code, I figured that it was better to also
> have the individual test target list the files they use (for
> components_unittests, this is already done, see
>
https://codereview.chromium.org/1903443002/diff/80001/components/security_sta...).

My gut/first reaction to that is that it feels 'wrong'. We'd previously treated
//net/data/ssl/certificates conceptually the same as a 'shared library' - that
is, if you depend on a target 'foo', you have access to all the symbols in
'foo', and the encapsulation of 'foo' is handled by the 'foo' library authors.

This feels like embedding individual files from arbitrary targets into static
libraries. That is, rather than a logical 'net_test_support', reembedding source
files in test executables (since the ODR rule)

That's my gut reaction - that this feels worse/less maintainable/more confusing.

[sdefresne, 2016-04-20 08:11:23]

On 2016/04/20 at 08:01:21, rsleevi wrote:
> On 2016/04/20 07:17:25, sdefresne wrote:
> > No source file in //net:test_support directly load any data from
> > //net/data/ssl/certificates. They are only loaded by tests in net_unittests
and
> > one test in components_unittests. Since the individual unit test have to
list
> > the data filename in their source code, I figured that it was better to also
> > have the individual test target list the files they use (for
> > components_unittests, this is already done, see
> >
https://codereview.chromium.org/1903443002/diff/80001/components/security_sta...).
> 
> My gut/first reaction to that is that it feels 'wrong'. We'd previously
treated //net/data/ssl/certificates conceptually the same as a 'shared library'
- that is, if you depend on a target 'foo', you have access to all the symbols
in 'foo', and the encapsulation of 'foo' is handled by the 'foo' library
authors.
> 
> This feels like embedding individual files from arbitrary targets into static
libraries. That is, rather than a logical 'net_test_support', reembedding source
files in test executables (since the ODR rule)
> 
> That's my gut reaction - that this feels worse/less maintainable/more
confusing.

This makes sense. I've moved net/data/ssl/certificates to new target
//net:test_support_bundle_data that is a dependency of //net:test_support. WDYT?

[sdefresne, 2016-04-20 10:15:03]

Please take another look.

[sdefresne, 2016-04-20 13:18:32]

Description was changed from

==========
[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

Since timestamp of directory is not always updated when the content
is changed (e.g. when a file content is changed, ...) listing just
the directory lead to flaky incremental builds and bot failures.

Explicitly list all test data files to fix those flakes.

BUG=459705
==========

to

==========
[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

Since timestamp of directory is not always updated when the content
is changed (e.g. when a file content is changed, ...) listing just
the directory lead to flaky incremental builds and bot failures.

Explicitly list all test data files to fix those flakes in net.gypi
and use those list in both gyp and gn. Document how the list have
been generated inline in net.gypi.

BUG=459705
==========

[eroman, 2016-04-20 18:04:05]

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn#newcode1451
net/BUILD.gn:1451: "data/",
Why are these not a problem?

The current CL enumerates some data dependencies, but not all of the data
dependencies of net_unittests. Here, and in the following lines, there are other
data directory dependencies.

I am conceptually fine with individually listing each of the data dependencies,
if that is what we need to do.

However I would like to make sure we are using a consistent approach throughout
so it is clear when adding new test data where it goes and how to update. And
have a consistent failure mode when forgetting to enumerate new test files
(rather than it works some places but breaks mysteriously on other bots).

Apologies if I am being dense, but can you explain to me why we need to
enumerate some directories but not others?

[eroman, 2016-04-20 18:11:01]

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn#newcode1451
net/BUILD.gn:1451: "data/",
On 2016/04/20 18:04:05, eroman wrote:
> Why are these not a problem?
> 
> The current CL enumerates some data dependencies, but not all of the data
> dependencies of net_unittests. Here, and in the following lines, there are
other
> data directory dependencies.
> 
> I am conceptually fine with individually listing each of the data
dependencies,
> if that is what we need to do.
> 
> However I would like to make sure we are using a consistent approach
throughout
> so it is clear when adding new test data where it goes and how to update. And
> have a consistent failure mode when forgetting to enumerate new test files
> (rather than it works some places but breaks mysteriously on other bots).
> 
> Apologies if I am being dense, but can you explain to me why we need to
> enumerate some directories but not others?

Let me re-word:

I think my confusion is in understanding the end-goal -- do we want to
consistently itemize all data files individually, or do we only want to bother
itemizing individually the data files that may be used by test run on iOS.

The subset of data chosen from //net/data/* by this CL I believe is just the
latter. There are large swaths of data dependencies included by directory as
evidenced by the contents of //net/data, as well as the rest of this build file.

This in-between state is hard to understand. Thanks!

[sdefresne, 2016-04-20 18:13:51]

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn#newcode1451
net/BUILD.gn:1451: "data/",
On 2016/04/20 at 18:04:05, eroman wrote:
> Why are these not a problem?
> 
> The current CL enumerates some data dependencies, but not all of the data
dependencies of net_unittests. Here, and in the following lines, there are other
data directory dependencies.
> 
> I am conceptually fine with individually listing each of the data
dependencies, if that is what we need to do.
> 
> However I would like to make sure we are using a consistent approach
throughout so it is clear when adding new test data where it goes and how to
update. And have a consistent failure mode when forgetting to enumerate new test
files (rather than it works some places but breaks mysteriously on other bots).
> 
> Apologies if I am being dense, but can you explain to me why we need to
enumerate some directories but not others?

The "data" property of "test" target is not used on iOS (per design, as "data"
semantic is too weak to be used on iOS, see
https://docs.google.com/document/d/1bKh57hg6TSBsEmeh0zWpOO5SVGA2INu-D3FGgsyIC...).
The "data" is used by script that are run post-build (i.e. they use the same
mechanism as the isolate files). Since they do not optimise for no-operation is
there is no change to the source, they do not have to list individual files.

We are trying to converge on a single way to express the runtime dependencies
with gn but each platform still has its peculiarity. See
https://groups.google.com/a/chromium.org/d/msg/gn-dev/sHpsuzw501o/mJjG8VYjCQAJ
for the current discussion (still unresolved). It should not be a problem to use
the same file list for both variables though, I was just trying to fix the iOS
build and did want to risk breaking other platforms by changing how they build
the app and ship it to the bots.

[sdefresne, 2016-04-20 18:21:02]

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/1896903002/diff/60001/net/BUILD.gn#newcode1451
net/BUILD.gn:1451: "data/",
On 2016/04/20 at 18:11:00, eroman wrote:
> On 2016/04/20 18:04:05, eroman wrote:
> > Why are these not a problem?
> > 
> > The current CL enumerates some data dependencies, but not all of the data
> > dependencies of net_unittests. Here, and in the following lines, there are
other
> > data directory dependencies.
> > 
> > I am conceptually fine with individually listing each of the data
dependencies,
> > if that is what we need to do.
> > 
> > However I would like to make sure we are using a consistent approach
throughout
> > so it is clear when adding new test data where it goes and how to update.
And
> > have a consistent failure mode when forgetting to enumerate new test files
> > (rather than it works some places but breaks mysteriously on other bots).
> > 
> > Apologies if I am being dense, but can you explain to me why we need to
> > enumerate some directories but not others?
> 
> Let me re-word:
> 
> I think my confusion is in understanding the end-goal -- do we want to
consistently itemize all data files individually, or do we only want to bother
itemizing individually the data files that may be used by test run on iOS.

Initially I wanted to try to list only individual files required on iOS but his
proved too hard (I spent almost a day running the app, watching the test fails,
finding which file was missing, adding it to bundle_data, rebuilding in a
cycle). Since we never bothered selecting which files to copy with gyp (we
listed the directory, which was flaky but copied all the files), I eventually
decided to copy all the files by individually listing them.

> The subset of data chosen from //net/data/* by this CL I believe is just the
latter. There are large swaths of data dependencies included by directory as
evidenced by the contents of //net/data, as well as the rest of this build file.
> 
> This in-between state is hard to understand. Thanks!

So, if I try to summarize, my CL tries to fix the flakyness of the build on iOS
(that is the only flaky platform due to its peculiar build system) without
affecting other platforms. I'd like to keep this change independent of removing
any duplication between iOS and the other platforms if possible.

Note that there already existed duplication before between "data" properties of
"test" targets" and "sources" properties of "bundle_data" targets.

[eroman, 2016-04-20 21:55:50]

Thanks for explaining.

LGTM then, to fix iOS build.

Hopefully we can unify this into something more comprehensible as part of
https://bugs.chromium.org/p/chromium/issues/detail?id=599319.

[sdefresne, 2016-04-21 08:37:44]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-04-21 08:37:45]

Thank you for the review.

[commit-bot: I haz the power, 2016-04-21 08:38:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1896903002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1896903002/60001

[commit-bot: I haz the power, 2016-04-21 08:42:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-22 19:32:47]

Description was changed from

==========
[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

Since timestamp of directory is not always updated when the content
is changed (e.g. when a file content is changed, ...) listing just
the directory lead to flaky incremental builds and bot failures.

Explicitly list all test data files to fix those flakes in net.gypi
and use those list in both gyp and gn. Document how the list have
been generated inline in net.gypi.

BUG=459705
==========

to

==========
[GN/iOS] Explicitly list test data in net_unittests_bundle_data.

Since timestamp of directory is not always updated when the content
is changed (e.g. when a file content is changed, ...) listing just
the directory lead to flaky incremental builds and bot failures.

Explicitly list all test data files to fix those flakes in net.gypi
and use those list in both gyp and gn. Document how the list have
been generated inline in net.gypi.

BUG=459705

Committed: https://crrev.com/04f9114d3168d9e59830a22d68547e06dbf9409e
Cr-Commit-Position: refs/heads/master@{#388725}
==========

[commit-bot: I haz the power, 2016-04-22 19:32:48]

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

[eroman, 2016-04-22 20:07:11]

https://codereview.chromium.org/1896903002/diff/60001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/1896903002/diff/60001/net/net.gypi#newcode2209
net/net.gypi:2209: 'data/certificate_policies_unittest/generate_policies.py',
FYI: Didn't notice this earlier, but the script should also exclude *.py, *.pyc,
*.sh.

I can follow-up on this..

[sdefresne, 2016-04-27 08:59:14]

On 2016/04/22 at 20:07:11, eroman wrote:
> https://codereview.chromium.org/1896903002/diff/60001/net/net.gypi
> File net/net.gypi (right):
> 
> https://codereview.chromium.org/1896903002/diff/60001/net/net.gypi#newcode2209
> net/net.gypi:2209: 'data/certificate_policies_unittest/generate_policies.py',
> FYI: Didn't notice this earlier, but the script should also exclude *.py,
*.pyc, *.sh.
> 
> I can follow-up on this..

Done with https://codereview.chromium.org/1923203002.