Add nogncheck to gtest/gtest_prod.h includes.

Add nogncheck to gtest/gtest_prod.h includes.

This is necessary to appease "gn check" if gtest_prod.h becomes a part
of the Chromium checkout, instead of a third-party repository brought
over by Chromium's DEPS. The file is already listed in v8's DEPS, but gn
does not use DEPS as an input.

BUG=chromium:630705
Review-Url: https://codereview.chromium.org/2807353002
Cr-Commit-Position: refs/heads/master@{#44604}
Committed: https://chromium.googlesource.com/v8/v8/+/53f13a09ba7553e8e0792cb987fb446b9ac1ddb0

[pwnall, 2017-04-11 00:35:12]

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

[commit-bot: I haz the power, 2017-04-11 00:35:23]

Dry run: 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, 2017-04-11 01:01:13]

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

[commit-bot: I haz the power, 2017-04-11 01:01:13]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-04-11 17:32:19]

pwnall@chromium.org changed reviewers:
+ jochen@chromium.org

[pwnall, 2017-04-11 17:32:20]

PTAL?

[jochen (gone - plz use gerrit), 2017-04-12 09:12:12]

hum, I'm not sure I understand this CL.

Ideally, chromium's gtest and v8's gtest are in sync. Will that still be the
case after your change?

[pwnall, 2017-04-12 09:40:14]

On 2017/04/12 09:12:12, jochen wrote:
> hum, I'm not sure I understand this CL.
> 
> Ideally, chromium's gtest and v8's gtest are in sync. Will that still be the
> case after your change?

This CL just makes it possible to land the CL that changes tests. Without it, I
get the build errors like
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...

I'd like to have this CL in so trybots aren't blocked on V8's headers for the
Chromium-side CL. I am still figuring out the details of how gtest will be
updated. I'd also like V8's gtest to be in sync :) and I will look into that.

[jochen (gone - plz use gerrit), 2017-04-12 09:43:08]

it looks like v8_base should depend on //testing/gtest:gtest?

[pwnall, 2017-04-12 09:46:35]

On 2017/04/12 09:43:08, jochen wrote:
> it looks like v8_base should depend on //testing/gtest:gtest?

Chromium's //base doesn't depend on //testing/gtest:gtest. Since that target has
testonly=true, I don't think it's supposed to go into binaries that we ship.

The only thing that can go into production binaries is gtest_prod.h, which
essentially has macros for declaring friend test classes. checkdeps.py is told
about this exception in the DEPS files, but GN doesn't follow that. Instead,
(IIUC) GN only uses dependencies in BUILD.gn files. However, to avoid issues
with generated files, (I think) GN ignores #includes for files that are not in
the repo.

[jochen (gone - plz use gerrit), 2017-04-12 09:48:08]

On 2017/04/12 at 09:46:35, pwnall wrote:
> On 2017/04/12 09:43:08, jochen wrote:
> > it looks like v8_base should depend on //testing/gtest:gtest?
> 
> Chromium's //base doesn't depend on //testing/gtest:gtest. Since that target
has testonly=true, I don't think it's supposed to go into binaries that we ship.
> 
> The only thing that can go into production binaries is gtest_prod.h, which
essentially has macros for declaring friend test classes. checkdeps.py is told
about this exception in the DEPS files, but GN doesn't follow that. Instead,
(IIUC) GN only uses dependencies in BUILD.gn files. However, to avoid issues
with generated files, (I think) GN ignores #includes for files that are not in
the repo.

does chromium's //base have similar annotations?

[pwnall, 2017-04-12 09:48:36]

On 2017/04/12 09:40:14, pwnall wrote:
> On 2017/04/12 09:12:12, jochen wrote:
> > hum, I'm not sure I understand this CL.
> > 
> > Ideally, chromium's gtest and v8's gtest are in sync. Will that still be the
> > case after your change?
> 
> This CL just makes it possible to land the CL that changes tests. Without it,
I
> get the build errors like
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...
> 
> I'd like to have this CL in so trybots aren't blocked on V8's headers for the
> Chromium-side CL. I am still figuring out the details of how gtest will be
> updated. I'd also like V8's gtest to be in sync :) and I will look into that.

Based on a bit of skimming, it seems to me that V8 mostly mirrors Chromium's
//testing setup. I propose that once the Chromium-side CL is close to the end of
the review cycle, I prepare a V8-side CL that essentially mirrors the approach
taken by the Chromium-side CL. Does this seem reasonable?

[jochen (gone - plz use gerrit), 2017-04-12 09:50:15]

On 2017/04/12 at 09:48:36, pwnall wrote:
> On 2017/04/12 09:40:14, pwnall wrote:
> > On 2017/04/12 09:12:12, jochen wrote:
> > > hum, I'm not sure I understand this CL.
> > > 
> > > Ideally, chromium's gtest and v8's gtest are in sync. Will that still be
the
> > > case after your change?
> > 
> > This CL just makes it possible to land the CL that changes tests. Without
it, I
> > get the build errors like
> >
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...
> > 
> > I'd like to have this CL in so trybots aren't blocked on V8's headers for
the
> > Chromium-side CL. I am still figuring out the details of how gtest will be
> > updated. I'd also like V8's gtest to be in sync :) and I will look into
that.
> 
> Based on a bit of skimming, it seems to me that V8 mostly mirrors Chromium's
//testing setup. I propose that once the Chromium-side CL is close to the end of
the review cycle, I prepare a V8-side CL that essentially mirrors the approach
taken by the Chromium-side CL. Does this seem reasonable?

yes, in fact, v8 has to be able to compile when you check it out standalone, and
we pull in gtest in v8/testing, and when v8 is checked out as part of chromium,
and we will use chromium's testing/ directory.

[pwnall, 2017-04-12 09:52:54]

On 2017/04/12 09:48:08, jochen wrote:
> On 2017/04/12 at 09:46:35, pwnall wrote:
> > On 2017/04/12 09:43:08, jochen wrote:
> > > it looks like v8_base should depend on //testing/gtest:gtest?
> > 
> > Chromium's //base doesn't depend on //testing/gtest:gtest. Since that target
> has testonly=true, I don't think it's supposed to go into binaries that we
ship.
> > 
> > The only thing that can go into production binaries is gtest_prod.h, which
> essentially has macros for declaring friend test classes. checkdeps.py is told
> about this exception in the DEPS files, but GN doesn't follow that. Instead,
> (IIUC) GN only uses dependencies in BUILD.gn files. However, to avoid issues
> with generated files, (I think) GN ignores #includes for files that are not in
> the repo.
> 
> does chromium's //base have similar annotations?

My Chromium-side CL adds the annotations. thakis@ said that the general approach
seems OK. We're still discussing exact locations, but it seems fairly certain to
me that forwarding headers will be required, at least temporarily, and
forwarding headers require this cop-out.

If the annotations bother your deeply, I can create a gtest_prod target that
contains the gtest_prod.h file. The target can then be used as a dependency for
targets that need to import gtest_prod.h. My initial instinct was to maintain
the current structure as much as possible, but I can see the benefits of being
more explicit and taking better advantage of GN's dependency-checking.

[jochen (gone - plz use gerrit), 2017-04-12 09:53:53]

On 2017/04/12 at 09:52:54, pwnall wrote:
> On 2017/04/12 09:48:08, jochen wrote:
> > On 2017/04/12 at 09:46:35, pwnall wrote:
> > > On 2017/04/12 09:43:08, jochen wrote:
> > > > it looks like v8_base should depend on //testing/gtest:gtest?
> > > 
> > > Chromium's //base doesn't depend on //testing/gtest:gtest. Since that
target
> > has testonly=true, I don't think it's supposed to go into binaries that we
ship.
> > > 
> > > The only thing that can go into production binaries is gtest_prod.h, which
> > essentially has macros for declaring friend test classes. checkdeps.py is
told
> > about this exception in the DEPS files, but GN doesn't follow that. Instead,
> > (IIUC) GN only uses dependencies in BUILD.gn files. However, to avoid issues
> > with generated files, (I think) GN ignores #includes for files that are not
in
> > the repo.
> > 
> > does chromium's //base have similar annotations?
> 
> My Chromium-side CL adds the annotations. thakis@ said that the general
approach seems OK. We're still discussing exact locations, but it seems fairly
certain to me that forwarding headers will be required, at least temporarily,
and forwarding headers require this cop-out.
> 
> If the annotations bother your deeply, I can create a gtest_prod target that
contains the gtest_prod.h file. The target can then be used as a dependency for
targets that need to import gtest_prod.h. My initial instinct was to maintain
the current structure as much as possible, but I can see the benefits of being
more explicit and taking better advantage of GN's dependency-checking.

no, if the chromium side gets the annotations, I'm happy to have the annotations
in v8 as well.

lgtm

[pwnall, 2017-04-12 09:55:18]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2017-04-12 09:55:21]

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

[pwnall, 2017-04-12 09:55:47]

On 2017/04/12 09:53:53, jochen wrote:
> On 2017/04/12 at 09:52:54, pwnall wrote:
> > On 2017/04/12 09:48:08, jochen wrote:
> > > On 2017/04/12 at 09:46:35, pwnall wrote:
> > > > On 2017/04/12 09:43:08, jochen wrote:
> > > > > it looks like v8_base should depend on //testing/gtest:gtest?
> > > > 
> > > > Chromium's //base doesn't depend on //testing/gtest:gtest. Since that
> target
> > > has testonly=true, I don't think it's supposed to go into binaries that we
> ship.
> > > > 
> > > > The only thing that can go into production binaries is gtest_prod.h,
which
> > > essentially has macros for declaring friend test classes. checkdeps.py is
> told
> > > about this exception in the DEPS files, but GN doesn't follow that.
Instead,
> > > (IIUC) GN only uses dependencies in BUILD.gn files. However, to avoid
issues
> > > with generated files, (I think) GN ignores #includes for files that are
not
> in
> > > the repo.
> > > 
> > > does chromium's //base have similar annotations?
> > 
> > My Chromium-side CL adds the annotations. thakis@ said that the general
> approach seems OK. We're still discussing exact locations, but it seems fairly
> certain to me that forwarding headers will be required, at least temporarily,
> and forwarding headers require this cop-out.
> > 
> > If the annotations bother your deeply, I can create a gtest_prod target that
> contains the gtest_prod.h file. The target can then be used as a dependency
for
> targets that need to import gtest_prod.h. My initial instinct was to maintain
> the current structure as much as possible, but I can see the benefits of being
> more explicit and taking better advantage of GN's dependency-checking.
> 
> no, if the chromium side gets the annotations, I'm happy to have the
annotations
> in v8 as well.
> 
> lgtm

Thank you very much for the review and for taking time to explain and discuss
concerns!

[commit-bot: I haz the power, 2017-04-12 10:24:24]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491990918625410, "parent_rev":
"8c0c5e8117a0c935d6f2e5f6e540674d46753a87", "commit_rev":
"53f13a09ba7553e8e0792cb987fb446b9ac1ddb0"}

[commit-bot: I haz the power, 2017-04-12 10:24:31]

Description was changed from

==========
Add nogncheck to gtest/gtest_prod.h includes.

This is necessary to appease "gn check" if gtest_prod.h becomes a part
of the Chromium checkout, instead of a third-party repository brought
over by Chromium's DEPS. The file is already listed in v8's DEPS, but gn
does not use DEPS as an input.

BUG=chromium:630705
==========

to

==========
Add nogncheck to gtest/gtest_prod.h includes.

This is necessary to appease "gn check" if gtest_prod.h becomes a part
of the Chromium checkout, instead of a third-party repository brought
over by Chromium's DEPS. The file is already listed in v8's DEPS, but gn
does not use DEPS as an input.

BUG=chromium:630705

Review-Url: https://codereview.chromium.org/2807353002
Cr-Commit-Position: refs/heads/master@{#44604}
Committed:
https://chromium.googlesource.com/v8/v8/+/53f13a09ba7553e8e0792cb987fb446b9ac...
==========

[commit-bot: I haz the power, 2017-04-12 10:24:32]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/v8/v8/+/53f13a09ba7553e8e0792cb987fb446b9ac...