Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168983002:
Propagate include directories of gtest and gmock to all dependent target (Closed)
DescriptionPropagate include directories of gtest and gmock to all dependent target
GN and GYP have different setup of include directories of gmock and
gtest. GN propagate include_dir of gtest and gmock to all deps, while
GYP doesn't. This CL changes GYP setting to propagate the setting to all
deps for parity to GN.
BUG=630299
Committed: https://crrev.com/d805a40eb7b1ef6b3069bd94be804793cbff5000
Cr-Commit-Position: refs/heads/master@{#407026}
Patch Set 1 #
Total comments: 2
Patch Set 2 : -"gtest" inclusion #
Depends on Patchset: Messages
Total messages: 23 (14 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG= ========== to ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG=630299 ==========
tzik@chromium.org changed reviewers: + dpranke@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + brettw@chromium.org
If we haven't needed this before, why do we need this now? Also, generally, using all_dependent_settings/all_dependent_configs should be avoided, so I'm kinda wondering why we don't just change the GN files, but maybe there's something special about gtest and gmock? punting this to brettw@ who wrote these.
On 2016/07/21 17:13:36, Dirk Pranke wrote: > If we haven't needed this before, why do we need this now? This is a preparation of C++11 switch of gmock. Since we are using slightly old libstdc++ on Linux, we need to inject a fixup header to gmock, that overrides "gmock/internal/custom/gmock-port.h". To do that, we need to put an include path for header injection before gmock include path. However, some modules (e.g. third_party/angle) have an indirect dep to gtest and gmock with manual include path setup on GYP build. This CL unifies the include path setup into test/*.gyp rather than distributing the hack for include path setup. > > Also, generally, using all_dependent_settings/all_dependent_configs should be > avoided, so I'm kinda wondering why we don't just change the GN files, but maybe > there's something special about gtest and gmock? > > punting this to brettw@ who wrote these.
https://codereview.chromium.org/2168983002/diff/1/testing/gtest.gyp File testing/gtest.gyp (right): https://codereview.chromium.org/2168983002/diff/1/testing/gtest.gyp#newcode33 testing/gtest.gyp:33: 'gtest', You added this one. I don't see this in the GN build at all. It seems like these should match. Does the GN build need updating or is this unnecessary?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2168983002/diff/1/testing/gtest.gyp File testing/gtest.gyp (right): https://codereview.chromium.org/2168983002/diff/1/testing/gtest.gyp#newcode33 testing/gtest.gyp:33: 'gtest', On 2016/07/21 19:38:27, brettw (ping on IM after 24h) wrote: > You added this one. I don't see this in the GN build at all. It seems like these > should match. Does the GN build need updating or is this unnecessary? Oops, thanks. A local mod contaminated the path as this. Reverted.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
It's sad that GN is more permissive in this respect, I can't remember why I did it like that. Ideally we would change the GN build not to use all*. But by itself, this patch seems fine. LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG=630299 ========== to ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG=630299 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG=630299 ========== to ========== Propagate include directories of gtest and gmock to all dependent target GN and GYP have different setup of include directories of gmock and gtest. GN propagate include_dir of gtest and gmock to all deps, while GYP doesn't. This CL changes GYP setting to propagate the setting to all deps for parity to GN. BUG=630299 Committed: https://crrev.com/d805a40eb7b1ef6b3069bd94be804793cbff5000 Cr-Commit-Position: refs/heads/master@{#407026} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d805a40eb7b1ef6b3069bd94be804793cbff5000 Cr-Commit-Position: refs/heads/master@{#407026} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
