|
|
DescriptionMove gtest_prod.h into its own target so it can be correctly set as a dependent target for base and reused as a dependency in gtest.
R=brettw
BUG=105855
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112701
Patch Set 1 : add missing dependency #Patch Set 2 : gtest: create a small project containing one header which is used from base #
Total comments: 2
Patch Set 3 : gtest: create a small project containing one header which is used from base #Patch Set 4 : gtest: create a small project containing one header which is used from base #Messages
Total messages: 35 (0 generated)
Base shouldn't depend on gtest. That's an error if it is the case.
On 2011/11/07 16:28:20, Marc-Antoine Ruel wrote: > Base shouldn't depend on gtest. That's an error if it is the case. There are 127 files under base doing #include "testing/gtest/include/gtest/gtest.h"
On 2011/11/07 17:07:06, ensonic wrote: > On 2011/11/07 16:28:20, Marc-Antoine Ruel wrote: > > Base shouldn't depend on gtest. That's an error if it is the case. > > There are 127 files under base doing > #include "testing/gtest/include/gtest/gtest.h" > git grep --count "gtest.h" | grep -v unittest files/file_path_watcher_browsertest.cc:1 test/perf_test_suite.cc:1 test/scoped_locale.cc:1 test/test_reg_util_win.cc:1 test/test_suite.cc:1 And all of them are in base_unittests or test_support_base? Please verify.
On 2011/11/07 17:53:20, Marc-Antoine Ruel wrote: > On 2011/11/07 17:07:06, ensonic wrote: > > On 2011/11/07 16:28:20, Marc-Antoine Ruel wrote: > > > Base shouldn't depend on gtest. That's an error if it is the case. > > > > There are 127 files under base doing > > #include "testing/gtest/include/gtest/gtest.h" > > > git grep --count "gtest.h" | grep -v unittest > files/file_path_watcher_browsertest.cc:1 > test/perf_test_suite.cc:1 > test/scoped_locale.cc:1 > test/test_reg_util_win.cc:1 > test/test_suite.cc:1 > > And all of them are in base_unittests or test_support_base? > > Please verify. > find base -type f -exec grep -l "gtest/gtest.h" {} \; | grep -v "unittest" base/test/scoped_locale.cc base/test/test_suite.cc base/test/perf_test_suite.cc base/test/test_reg_util_win.cc base/files/file_path_watcher_browsertest.cc verified. Then tehre is base/gtest_prod_util.h #include "testing/gtest/include/gtest/gtest_prod.h" and still all the unittests that depend on gtest. I'll see if I can move the dependency to other targets under base.
Without the patch compilation fails: From //base:base (cc-compile-static) (base/metrics/histogram.cc): In file included from ./base/metrics/histogram.h:51:0, from base/metrics/histogram.cc:10: ./base/gtest_prod_util.h:9:52: fatal error: testing/gtest/include/gtest/gtest_prod.h: No such file or directory compilation terminated.
On 2011/11/08 10:20:17, ensonic wrote: > Without the patch compilation fails: > > From //base:base (cc-compile-static) (base/metrics/histogram.cc): > In file included from ./base/metrics/histogram.h:51:0, > from base/metrics/histogram.cc:10: > ./base/gtest_prod_util.h:9:52: fatal error: > testing/gtest/include/gtest/gtest_prod.h: No such file or directory > compilation terminated. Ah! Ok, gtest_prod.h is a specific file to be used in production without linking to gtest. You can look at it, it defines a single macro. So I'd simply add gtest/include/gtest/gtest_prod.h to base.
On 2011/11/08 13:48:30, Marc-Antoine Ruel wrote: > On 2011/11/08 10:20:17, ensonic wrote: > > Without the patch compilation fails: > > > > From //base:base (cc-compile-static) (base/metrics/histogram.cc): > > In file included from ./base/metrics/histogram.h:51:0, > > from base/metrics/histogram.cc:10: > > ./base/gtest_prod_util.h:9:52: fatal error: > > testing/gtest/include/gtest/gtest_prod.h: No such file or directory > > compilation terminated. > > Ah! > > Ok, gtest_prod.h is a specific file to be used in production without linking to > gtest. You can look at it, it defines a single macro. So I'd simply add > gtest/include/gtest/gtest_prod.h to base. Adding headers from different packages is not possible. What would work, is to move gtest_prod_util.h to testing/gtest/include/gtest/ instead. If that sounds okay, I can try a new CL.
On 2011/11/09 13:08:11, ensonic wrote: > Adding headers from different packages is not possible. What would work, is to > move gtest_prod_util.h to testing/gtest/include/gtest/ instead. If that sounds > okay, I can try a new CL. IMHO, the best option to upstream the macro. Zhanyong, are you fine with adding the macro FRIEND_TEST_ALL_PREFIXES() defined at #1 into #2 ? #1 http://src.chromium.org/viewvc/chrome/trunk/src/base/gtest_prod_util.h?view=m... #2 http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest_p...
+Greg, the new owner of gtest. On Wed, Nov 9, 2011 at 6:23 AM, <maruel@chromium.org> wrote: > On 2011/11/09 13:08:11, ensonic wrote: >> >> Adding headers from different packages is not possible. What would work, >> is to >> move gtest_prod_util.h to testing/gtest/include/gtest/ instead. If that >> sounds >> okay, I can try a new CL. > > IMHO, the best option to upstream the macro. Zhanyong, are you fine with > adding > the macro FRIEND_TEST_ALL_PREFIXES() defined at #1 into #2 ? > > #1 > http://src.chromium.org/viewvc/chrome/trunk/src/base/gtest_prod_util.h?view=m... > > #2 > http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest_p... > > http://codereview.chromium.org/8201001/ > -- Zhanyong
gtest itself only knows about the DISABLED_ prefix. The others are conventions used by Chromium only. Unless we want to standardize the conventions (which I doubt), they don't seem to belong to gtest. 2011/11/9 Zhanyong Wan (λx.x x) <wan@google.com>: > +Greg, the new owner of gtest. > > On Wed, Nov 9, 2011 at 6:23 AM, <maruel@chromium.org> wrote: >> On 2011/11/09 13:08:11, ensonic wrote: >>> >>> Adding headers from different packages is not possible. What would work, >>> is to >>> move gtest_prod_util.h to testing/gtest/include/gtest/ instead. If that >>> sounds >>> okay, I can try a new CL. >> >> IMHO, the best option to upstream the macro. Zhanyong, are you fine with >> adding >> the macro FRIEND_TEST_ALL_PREFIXES() defined at #1 into #2 ? >> >> #1 >> http://src.chromium.org/viewvc/chrome/trunk/src/base/gtest_prod_util.h?view=m... >> >> #2 >> http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest_p... >> >> http://codereview.chromium.org/8201001/ >> > > > > -- > Zhanyong > -- Zhanyong
On 2011/11/09 15:13:22, wan wrote: > gtest itself only knows about the DISABLED_ prefix. The others are > conventions used by Chromium only. Unless we want to standardize the > conventions (which I doubt), they don't seem to belong to gtest. Ok fine, so Stefan otherwise you would prefer to have a gtest_prod pseudo project that includes only gtest_prod.h?
On 2011/11/09 15:17:11, Marc-Antoine Ruel wrote: > On 2011/11/09 15:13:22, wan wrote: > > gtest itself only knows about the DISABLED_ prefix. The others are > > conventions used by Chromium only. Unless we want to standardize the > > conventions (which I doubt), they don't seem to belong to gtest. > > Ok fine, so Stefan otherwise you would prefer to have a gtest_prod pseudo > project that includes only gtest_prod.h? I think moving "base/gtest_prod_util.h" to "testing/gtest_prod_util.h" would work. For the 4 files under base inlcuding it ./base/version.h ./base/pickle.h ./base/metrics/histogram.h ./base/metrics/field_trial.h actually only pickle.h is using the macro. I can remove the obsolete includes at the same time.
On 2011/11/09 16:42:56, ensonic wrote: > On 2011/11/09 15:17:11, Marc-Antoine Ruel wrote: > > Ok fine, so Stefan otherwise you would prefer to have a gtest_prod pseudo > > project that includes only gtest_prod.h? > > I think moving "base/gtest_prod_util.h" to "testing/gtest_prod_util.h" would > work. > > For the 4 files under base inlcuding it > ./base/version.h > ./base/pickle.h > ./base/metrics/histogram.h > ./base/metrics/field_trial.h > actually only pickle.h is using the macro. I can remove the obsolete includes at > the same time. I'm fine with that.
On 2011/11/09 16:45:10, Marc-Antoine Ruel wrote: > On 2011/11/09 16:42:56, ensonic wrote: > > On 2011/11/09 15:17:11, Marc-Antoine Ruel wrote: > > > Ok fine, so Stefan otherwise you would prefer to have a gtest_prod pseudo > > > project that includes only gtest_prod.h? > > > > I think moving "base/gtest_prod_util.h" to "testing/gtest_prod_util.h" would > > work. > > > > For the 4 files under base inlcuding it > > ./base/version.h > > ./base/pickle.h > > ./base/metrics/histogram.h > > ./base/metrics/field_trial.h > > actually only pickle.h is using the macro. I can remove the obsolete includes > at > > the same time. > > I'm fine with that. I am not sure if that was clear. Even if I move "gtest_prod_util.h" it won't change anything about the initial patch. "gtest_prod_util.h" includes headers from testing and thus we need to add the dependency. I have two new patches that clean things up slightly, but this patch is still needed. The two changes are in: http://codereview.chromium.org/8555003 http://codereview.chromium.org/8528023 (no reviewer assigned yet).
On 2011/11/14 11:12:25, ensonic wrote: > I am not sure if that was clear. Even if I move "gtest_prod_util.h" it won't > change anything about the initial patch. "gtest_prod_util.h" includes headers > from testing and thus we need to add the dependency. > > I have two new patches that clean things up slightly, but this patch is still > needed. The two changes are in: > http://codereview.chromium.org/8555003 > http://codereview.chromium.org/8528023 > (no reviewer assigned yet). Let me be clearer here. Under no circumstance base will depend on gtest. In no way we're going to link gtest into production binaries. This is not a server executable here. So the fix is to create a 'project' that has only testing/gtest/include/gtest/gtest_prod.h in it, named 'gtest_prod'.
On 2011/11/14 17:19:57, Marc-Antoine Ruel wrote: > On 2011/11/14 11:12:25, ensonic wrote: > > I am not sure if that was clear. Even if I move "gtest_prod_util.h" it won't > > change anything about the initial patch. "gtest_prod_util.h" includes headers > > from testing and thus we need to add the dependency. > > > > I have two new patches that clean things up slightly, but this patch is still > > needed. The two changes are in: > > http://codereview.chromium.org/8555003 > > http://codereview.chromium.org/8528023 > > (no reviewer assigned yet). > > Let me be clearer here. Under no circumstance base will depend on gtest. In no > way we're going to link gtest into production binaries. This is not a server > executable here. > > So the fix is to create a 'project' that has only > testing/gtest/include/gtest/gtest_prod.h in it, named 'gtest_prod'. Maybe we have a misunderstanding here. Dependency to me means "needs one or more files at compile and linking phase". Right now the issue for me is that files under base depend on gtest in compiling phase already, but that dependency is not established in the rules.
Does the build use a special -D when building tests? Maybe we can put all the parts that include gtests behind an #ifdef?
On 2011/11/15 08:44:00, ensonic wrote: > Does the build use a special -D when building tests? Maybe we can put all the > parts that include gtests behind an #ifdef? That could be an option but I don't like this idea. On 2011/11/15 08:40:49, ensonic wrote: > Maybe we have a misunderstanding here. Dependency to me means "needs one or more > files at compile and linking phase". Right now the issue for me is that files > under base depend on gtest in compiling phase already, but that dependency is > not established in the rules. "files in base depend on an header in gtest." That's not the same thing that depending on compile units. It's more akin src/base/third_party/valgrind/valgrind.h. That is why if you need to specify the header file, create a gtest_prod project of type 'None' holding only gtest_prod.h. Then make both gtest and base depend on gtest_prod. If you make base depend on gtest, you pull all of gtest library into any project linking with base. That is real bad.
On 2011/11/15 14:29:29, Marc-Antoine Ruel wrote: > On 2011/11/15 08:44:00, ensonic wrote: > > Does the build use a special -D when building tests? Maybe we can put all the > > parts that include gtests behind an #ifdef? > > That could be an option but I don't like this idea. > > On 2011/11/15 08:40:49, ensonic wrote: > > Maybe we have a misunderstanding here. Dependency to me means "needs one or > more > > files at compile and linking phase". Right now the issue for me is that files > > under base depend on gtest in compiling phase already, but that dependency is > > not established in the rules. > > "files in base depend on an header in gtest." That's not the same thing that > depending on compile units. It's more akin > src/base/third_party/valgrind/valgrind.h. That is why if you need to specify the > header file, create a gtest_prod project of type 'None' holding only > gtest_prod.h. Then make both gtest and base depend on gtest_prod. > > If you make base depend on gtest, you pull all of gtest library into any project > linking with base. That is real bad. Just to explain my motivation. The build system I am working on uses sandboxes for each build step. Files that are not mentioned in the dependencies won't be there and build step execution time. If we can't resolve this, I will drag the patch along.
It's not clear why you're resisting M-A's suggestion of a gtest prod project. That seems like the right solution to me if you need the header file dependencies to be listed. Adding a full gtest dependency from base is clearly wrong.
On 2011/11/15 18:35:35, brettw wrote: > It's not clear why you're resisting M-A's suggestion of a gtest prod project. > That seems like the right solution to me if you need the header file > dependencies to be listed. Adding a full gtest dependency from base is clearly > wrong. Not resisting. We had a misunderstanding and it is clear now after a chat conversation (thanks MA for your patience). I'll make the changes.
http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp File testing/gtest.gyp (right): http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp#newcode20 testing/gtest.gyp:20: 'gtest/include/gtest/gtest_prod.h', I'd prefer to have it removed from here and have gtest depend on gtest_prod. It'd be cleaner.
http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp File testing/gtest.gyp (right): http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp#newcode20 testing/gtest.gyp:20: 'gtest/include/gtest/gtest_prod.h', On 2011/11/29 15:22:51, Marc-Antoine Ruel wrote: > I'd prefer to have it removed from here and have gtest depend on gtest_prod. > It'd be cleaner. Done.
On 2011/11/30 12:31:54, ensonic wrote: > http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp > File testing/gtest.gyp (right): > > http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp#newcode20 > testing/gtest.gyp:20: 'gtest/include/gtest/gtest_prod.h', > On 2011/11/29 15:22:51, Marc-Antoine Ruel wrote: > > I'd prefer to have it removed from here and have gtest depend on gtest_prod. > > It'd be cleaner. > > Done. This is causing the trouble I've been trying to fix with CL=8198002, but that does not seem to be the right fix. Lets hold this back until this is resolved.
On 2011/12/01 12:52:08, ensonic wrote: > On 2011/11/30 12:31:54, ensonic wrote: > > http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp > > File testing/gtest.gyp (right): > > > > http://codereview.chromium.org/8201001/diff/20001/testing/gtest.gyp#newcode20 > > testing/gtest.gyp:20: 'gtest/include/gtest/gtest_prod.h', > > On 2011/11/29 15:22:51, Marc-Antoine Ruel wrote: > > > I'd prefer to have it removed from here and have gtest depend on gtest_prod. > > > It'd be cleaner. > > > > Done. > > This is causing the trouble I've been trying to fix with CL=8198002, but that > does not seem to be the right fix. Lets hold this back until this is resolved. This has been resolved, thanks to Albert Wong.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ensonic@google.com/8201001/29001
On 2011/12/02 12:44:14, Marc-Antoine Ruel wrote: > lgtm But the description is so-so, this would be way more informative: Move gtest_prod.h into its own target so it can be correctly set as a dependent target for base.
On 2011/12/02 12:53:21, Marc-Antoine Ruel wrote: > On 2011/12/02 12:44:14, Marc-Antoine Ruel wrote: > > lgtm > > But the description is so-so, this would be way more informative: > > Move gtest_prod.h into its own target so it can be correctly set as a dependent > target for base. Done. My git commits have better messages, but they get lost when the CL is squashed and uploaded :/.
On 2011/12/02 13:04:50, ensonic wrote: > My git commits have better messages, but they get lost when the CL is squashed > and uploaded :/. Use git squash (an alias from my bin_pub/configs/.gitconfig on github) then do: git cl upload --send-mail Make sure to set the R=maruel@chromium.org line in the description and you're fine.
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ensonic@google.com/8201001/29001
Change committed as 112701 |