libroach: use gtest in unitests by mberhault · Pull Request #20594 · cockroachdb/cockroach

mberhault · 2017-12-09T16:39:57Z

Release Note: none.

Add new googletest c-dep and use it for libroach unitests.
Add support for many unittests in libroach, with naming scheme based on
the dir and filename. (eg: ccl_libroach_ccl_test for ccl/libroach_ccl_test.cc)

cockroach-teamcity · 2017-12-09T16:40:06Z

This change is

benesch · 2017-12-09T18:11:32Z

Shoot, we duplicated some work here. I've got a PR that adds some tests for our C++ encoding (8aa8ce9) that's been sidelined on other, unrelated changes in the PR. My bad!

The good news is that our approach is more or less the same. I'd love to get those encoding tests merged, though, while we're adding libroach tests. @mberhault how do you want to proceed?

benesch · 2017-12-09T18:29:49Z

Extracted and rebased here: #20595

mberhault · 2017-12-09T18:35:10Z

Ah! funny! Let's get yours in, the main difference is in CMakeLists and my setup with split libroach and libroachccl is premature. I'll rebase on top of yours and keep working on adding googletest.

mberhault · 2017-12-09T20:25:02Z

Added googletest for unittests, this should be more pleasant.

benesch · 2017-12-09T21:31:43Z

c-deps/libroach/encoding_test.cc

benesch · 2017-12-09T21:31:59Z

c-deps/libroach/encoding_test.cc

If you're bored, would be nice to let clang format handle these, but it totally mangles cases64 IMO.

benesch · 2017-12-09T21:32:21Z

c-deps/libroach/encoding_test.cc

Should be able to drop this variable now!

mberhault · 2017-12-09T22:37:11Z

@benesch: rebased on your change, PTAL.

Not much to be done about clang formatting I'm afraid. We can change some of the line merging things (eg: no one-line functions), but ultimately it parses everything then re-generates it from the AST so any arbitrary whitespace formatting is lost.

mberhault · 2017-12-10T16:06:10Z

Makefile

this is weird, Cmake fails when cross-compiling (acceptance tests for some reason). This is kind of the accepted answer (mentioned in the failure itself). I need the absolute path as we're running cmake from the temp build dir.

Ugh, what a pain! I think we can at least avoid some of the complexity, though, by dropping

set(THREADS_PTHREAD_ARG "2" CACHE STRING "Forcibly set by CMakeLists.txt." FORCE)

into libroach's CMakeLists.txt instead of introducing the whole cachefile kit and caboodle.

Could you also add a comment linking to https://gitlab.kitware.com/cmake/cmake/issues/16920? Looks like this is fixed in CMake 3.10 and so this workaround should eventually become unnecessary.

benesch

I think you're doing too much work! Both CMake and the top-level Makefile are building googletest AFAICT.

There are two options I see:

Let CMake handle building libgtest by simply removing all references to googletest from the top-level Makefile. In this case I think we should move the submodule to c-deps/libroach/googletest.
Let top-level Make handle building libgtest, and set the library search paths appropriately for libroach, and potentially other libraries whose tests need libgtest, like Snappy.

I have a slight preference for the first option since it's simpler and we don't actually care about running Snappy's tests, but I don't have strong feelings.

benesch · 2017-12-09T23:54:50Z

c-deps/libroach/CMakeLists.txt

+    CXX_STANDARD_REQUIRED YES
+    CXX_EXTENSIONS NO
+    COMPILE_OPTIONS "-Werror;-Wall;-Wno-sign-compare"
+  )


Shoot, I wish there were some way to share these properties across all targets. I don't know enough CMake to suggest a better alternative, though.

yeah, I'm still fuzzy here, I've been meaning to check the rules for non-target properties. Is it "everything" after this point?

benesch · 2017-12-10T19:42:30Z

Makefile


 PHONY: check-libroach
-check-libroach: $(LIBROACH_DIR)/Makefile
+check-libroach: $(LIBROACH_DIR)/Makefile libgtest


By my read, this libgtest is built into ~/go/native/TRIPLE/googletest and is actually entirely ignored by libroach's CMake, which builds its own version at ~/go/native/TRIPLE/libroach/googletest thanks to the magic of add_subdirectory.

See main comment: our cmake doesn't build googletest, it just needs a built library it can link in. This is what add_subdirectory does.

mberhault · 2017-12-10T19:52:59Z

Our cmake uses the built googletest by calling add_subdirectory, it does not build it. It is possible to have cmake build it from source, but it's just as tedious (more so even, especially when you're not using a subdirectory).
The only thing our cmake does it look for googletest and finds the correct include and lib directories.

benesch · 2017-12-10T19:58:04Z

Now I’m terribly confused. add_subdirctory instructs CMake to build from the sources in the subdirectory AIUI. I checked out your branch locally and reverted all the changes to the top-level Makefile and `make check-libroach` still worked just fine. What gives?

…

On Sun, Dec 10, 2017 at 2:54 PM marc ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Makefile <#20594 (comment)> : > @@ -620,9 +630,13 @@ libroachccl: $(LIBROACH_DIR)/Makefile libroach @$(MAKE) --no-print-directory -C $(LIBROACH_DIR) roachccl PHONY: check-libroach -check-libroach: $(LIBROACH_DIR)/Makefile +check-libroach: $(LIBROACH_DIR)/Makefile libgtest See main comment: our cmake doesn't build googletest, it just needs a built library it can link in. This is what add_subdirectory does. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20594 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IKptZq0ER83ID8EoKCRSlmzHRXpaks5s_Db4gaJpZM4Q8J34> .

mberhault · 2017-12-10T20:04:02Z

Ok, ditto. I'm also very very confused, but that summarizes my week-end pretty well.
I'm pushing a commit with the main makefile changes reverted (except for that annoying cmake cache crap). Let's see how it goes. I'm just hoping the success isn't an artifact of my build caching, but that shouldn't impact you.

mberhault · 2017-12-10T20:58:50Z

Ok, all green. Damn, I need to read up on cmake I guess.
I'll squash and push again.

benesch · 2017-12-11T07:00:16Z

build/cmake_cache.txt

@@ -0,0 +1,3 @@
+# Cross-compilation does strange things with CMake. Tell it the pthreads check passes.
+set(THREADS_PTHREAD_ARG "0" CACHE STRING "Result from TRY_RUN" FORCE)


By my read of https://github.com/Kitware/CMake/blob/v3.9.0/Modules/FindThreads.cmake, don't you want to set this to "2", not "0"?

yeah, I've tried a number of things, including setting it in the libroach cmake only. The only thing that's worked so far is setting it as a cache preset using -C. I honestly don't want to play with this crap again. It's broken, and this fixes it.

ugh, now it works. Let's see if TC agrees.

benesch · 2017-12-11T07:03:22Z

Makefile

Ugh, what a pain! I think we can at least avoid some of the complexity, though, by dropping

set(THREADS_PTHREAD_ARG "2" CACHE STRING "Forcibly set by CMakeLists.txt." FORCE)

into libroach's CMakeLists.txt instead of introducing the whole cachefile kit and caboodle.

Could you also add a comment linking to https://gitlab.kitware.com/cmake/cmake/issues/16920? Looks like this is fixed in CMake 3.10 and so this workaround should eventually become unnecessary.

benesch

Amazing, LGTM! Sorry this was such a chore.

Probably worth getting someone else to buy in on the googletest dependency too.

mberhault · 2017-12-11T15:23:27Z

TFTR!
You mean the location of the googletest tree? I think keeping the submodule in the same place as all the others is least surprising. But a point could be made for sticking it in libroach/

@bdarnell: any thoughts?

benesch · 2017-12-11T15:28:51Z

No no, I don’t feel strongly about its location! Happy with it where it is. I mean the fact that we’re bringing in the dependency at all. If there’s a policy on adding new C/C++ dependencies I don’t know it.

…

On Mon, Dec 11, 2017 at 10:23 AM marc ***@***.***> wrote: TFTR! You mean the location of the googletest tree? I think keeping the submodule in the same place as all the others is least surprising. But a point could be made for sticking it in libroach/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20594 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IJ9qsQ-qQgKQ8BM2AyzVOCK_iiZeks5s_Uj7gaJpZM4Q8J34> .

mberhault · 2017-12-11T15:36:01Z

I don't think there is, and anything that can make unit testing easier is good in my book.

benesch · 2017-12-11T15:58:10Z

Mk, SGTM.

mberhault · 2017-12-11T16:07:43Z

Peter doesn't care. Squashing and rebuilding.

Release note: None Add new `googletest` c-dep and use it for libroach unitests. Add support for many unittests in libroach, with naming scheme based on the dir and filename. (eg: `ccl_libroach_ccl_test` for `ccl/libroach_ccl_test.cc`)

We don't need it anymore. We originally introduced it to support tests in libroach (cockroachdb#20594), which has since been pared down to something much simpler (cockroachdb#55509, just DBDumpThreadStacks). Release note: None

55900: c-deps: remove googletest r=irfansharif a=irfansharif We don't need it anymore. We originally introduced it to support tests in libroach (#20594), which has since been pared down to something much simpler (#55509, just DBDumpThreadStacks). Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

mberhault requested review from a team and benesch December 9, 2017 16:39

benesch reviewed Dec 9, 2017

View reviewed changes

mberhault force-pushed the marc/add_libroach_tests branch 4 times, most recently from f96bb11 to 4e57fc7 Compare December 9, 2017 22:21

mberhault changed the title ~~libroach: add unittest targets for libroach~~ libroach: use gtest in unitests Dec 9, 2017

mberhault force-pushed the marc/add_libroach_tests branch from 4e57fc7 to 6ad711f Compare December 9, 2017 22:27

mberhault force-pushed the marc/add_libroach_tests branch 4 times, most recently from 4bf9684 to 3b7880c Compare December 10, 2017 16:07

mberhault commented Dec 10, 2017

View reviewed changes

mberhault mentioned this pull request Dec 10, 2017

storage: pass encryption options to libroachccl. #20570

Merged

benesch reviewed Dec 10, 2017

View reviewed changes

mberhault force-pushed the marc/add_libroach_tests branch 2 times, most recently from 36e0ddb to 55747d6 Compare December 11, 2017 02:24

benesch reviewed Dec 11, 2017

View reviewed changes

benesch approved these changes Dec 11, 2017

View reviewed changes

libroach: use gtest in unitests

2d310c7

Release note: None Add new `googletest` c-dep and use it for libroach unitests. Add support for many unittests in libroach, with naming scheme based on the dir and filename. (eg: `ccl_libroach_ccl_test` for `ccl/libroach_ccl_test.cc`)

mberhault force-pushed the marc/add_libroach_tests branch from c1a74d1 to 2d310c7 Compare December 11, 2017 16:09

mberhault merged commit f9e5a08 into cockroachdb:master Dec 11, 2017

mberhault deleted the marc/add_libroach_tests branch December 11, 2017 17:24

irfansharif mentioned this pull request Oct 23, 2020

c-deps: remove googletest #55900

Merged

		@@ -0,0 +1,3 @@
		# Cross-compilation does strange things with CMake. Tell it the pthreads check passes.
		set(THREADS_PTHREAD_ARG "0" CACHE STRING "Result from TRY_RUN" FORCE)

Conversation

mberhault commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Dec 9, 2017

Uh oh!

benesch commented Dec 9, 2017

Uh oh!

benesch commented Dec 9, 2017

Uh oh!

mberhault commented Dec 9, 2017

Uh oh!

mberhault commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mberhault commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mberhault Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

benesch commented Dec 10, 2017 via email

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

mberhault commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benesch commented Dec 11, 2017 via email

Uh oh!

mberhault commented Dec 11, 2017

Uh oh!

benesch commented Dec 11, 2017

Uh oh!

mberhault commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mberhault commented Dec 9, 2017 •

edited

Loading

mberhault Dec 10, 2017 •

edited

Loading

mberhault commented Dec 11, 2017 •

edited

Loading