cmake: do not build tests for Release build and cleanups #5916

tchaikov · 2019-10-14T06:05:56Z

fixes #2445

riversand963 · 2019-10-14T16:45:37Z

Appveyor seems to be broken by the PR.

tchaikov · 2019-10-24T04:47:40Z

@riversand963 could you help review this change?

tchaikov · 2019-11-13T14:07:48Z

ping?

tchaikov · 2019-12-10T07:08:52Z

@riversand963 @siying , is there anything i can do to progress this change? i just found that #6098 was merged. it addresses this issue in a different way. but i think this this change is still relevant.

riversand963 · 2019-12-10T18:17:59Z

Will take a look.

siying · 2019-12-10T19:12:09Z

CMakeLists.txt

-
-  add_executable(db_bench${ARTIFACT_SUFFIX}
-    tools/db_bench.cc
-    tools/db_bench_tool.cc


I'm a little bit confused why it can still build after removing this file.

my bad, added db_bench_tool.cc back . the reason it built is that gflags is not found by cmake as neither do we have Findgflags.cmake nor {gflagsConfig,gflags-config}.cmake in the (ancient) xenial CI node created by travis.

i added Findgflags.cmake to address this issue in this PR.

siying · 2019-12-12T20:27:32Z

Travis fails. Can you take a look?

tchaikov · 2019-12-13T03:15:30Z

@siying yes. fixed, rebased and repushed.

cmake pass '-DNDEBUG' to compiler when compiling non-Debug builds. and rocksdb's tests are short-circuited if the "NDEBUG" macro is defined, so the "Release" builds fail when building tests. in this change, cmake disables `WITH_TESTS` option if `CMAKE_BUILD_TYPE` is `Release`. this also maps how `Makefile` defines `dbg` and `release` targets. in `Makefile`, `release` target does not depend on `$(TESTS)`, while `dbg` does. fixes facebook#2445 Signed-off-by: Kefu Chai <tchaikov@gmail.com>

so we can reference it without worrying about its dependencies like gtest. testharness is used by table_reader_bench. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

`WITH_GFLAGS` is enabled/disabled depending on the building system, so it'd be simpler if we could define it using `CMAKE_DEPENDENT_OPTION`. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

unfold the loop, and remove the unused dependencies from linked libraries. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

TEST_EXES and C_TEST_EXES are just aliases, so remove them Signed-off-by: Kefu Chai <tchaikov@gmail.com>

* no need to use a loop for building it * no need to set properties like `EXCLUDE_FROM_DEFAULT_BUILD_RELEASE`, these properties are not read by anybody * no need to link against testutillib Signed-off-by: Kefu Chai <tchaikov@gmail.com>

Signed-off-by: Kefu Chai <tchaikov@gmail.com>

newer libgflags-dev ships with cmake support, but libgflags-dev 2.1.2-3 shipped by xenial does not. let's add the find_package() support. so our CI can build with gflags Signed-off-by: Kefu Chai <tchaikov@gmail.com>

Signed-off-by: Kefu Chai <tchaikov@gmail.com>

as we don't have libgflags-dev installed on mingw, and the toolchain is not MSVC. please note, currently WITH_GFLAGS is OFF by default on MINGW, but it's still better to be explicit. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

since WITH_GFLAGS is ON by default on OSX, we need to install or or disable it explicitly. it'd be better to install it for better coverage of the CI test. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

stress_tool/batched_ops_stress.cc:178:41: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] 2822 std::array<std::string, 10> keys = {"0", "1", "2", "3", "4", 2823 ^~~~~~~~~~~~~~~~~~~~~~~~ 28241 error generated. see the discussions at https://stackoverflow.com/questions/22501368/why-wasnt-a-double-curly-braces-syntax-preferred-for-constructors-taking-a-std and https://en.cppreference.com/w/cpp/container/array Signed-off-by: Kefu Chai <tchaikov@gmail.com>

facebook-github-bot

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

siying · 2019-12-13T18:05:36Z

Thank you for your contribution and your patient!

facebook-github-bot · 2019-12-13T21:41:07Z

This pull request has been merged in ac304ad.

Summary: fixes facebook#2445 Pull Request resolved: facebook#5916 Differential Revision: D19031236 fbshipit-source-id: bc3107b6b25a01958677d7cb411b1f381aae91c6

facebook-github-bot added the CLA Signed label Oct 14, 2019

tchaikov mentioned this pull request Oct 14, 2019

Compile error in release version #5866

Closed

tchaikov mentioned this pull request Dec 10, 2019

Add an option to the CMake build to disable building shared libraries #6122

Closed

tchaikov force-pushed the wip-cmake branch 2 times, most recently from e4a8d28 to 27a7a44 Compare December 10, 2019 07:45

siying reviewed Dec 10, 2019

View reviewed changes

tchaikov force-pushed the wip-cmake branch 2 times, most recently from e6dd0ad to 7273261 Compare December 11, 2019 03:23

tchaikov force-pushed the wip-cmake branch 2 times, most recently from 6194007 to a7a1d05 Compare December 13, 2019 03:14

tchaikov added 7 commits December 13, 2019 17:59

cmake: compile testharness as a static library

7d1de89

so we can reference it without worrying about its dependencies like gtest. testharness is used by table_reader_bench. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: use CMAKE_DEPENDENT_OPTION() to conditionalize option definition

9e33443

`WITH_GFLAGS` is enabled/disabled depending on the building system, so it'd be simpler if we could define it using `CMAKE_DEPENDENT_OPTION`. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: add benchmark tools without a loop

3d65227

unfold the loop, and remove the unused dependencies from linked libraries. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: remove alias variables

49d3a25

TEST_EXES and C_TEST_EXES are just aliases, so remove them Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: simplify db/c_test.c build

a0599be

* no need to use a loop for building it * no need to set properties like `EXCLUDE_FROM_DEFAULT_BUILD_RELEASE`, these properties are not read by anybody * no need to link against testutillib Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: default to Debug

b77534d

Signed-off-by: Kefu Chai <tchaikov@gmail.com>

tchaikov force-pushed the wip-cmake branch from ee9192d to 4874c3e Compare December 13, 2019 10:09

tchaikov added 4 commits December 13, 2019 19:28

cmake: add Findgflags.cmake

2117e9a

newer libgflags-dev ships with cmake support, but libgflags-dev 2.1.2-3 shipped by xenial does not. let's add the find_package() support. so our CI can build with gflags Signed-off-by: Kefu Chai <tchaikov@gmail.com>

cmake: require gflags if WITH_GFLAGS

a9b5366

Signed-off-by: Kefu Chai <tchaikov@gmail.com>

.travis.yml: disable WITH_GFLAGS for cmake-mingw

5492027

as we don't have libgflags-dev installed on mingw, and the toolchain is not MSVC. please note, currently WITH_GFLAGS is OFF by default on MINGW, but it's still better to be explicit. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

.travis.yml: install gflags on osx

f8cd930

since WITH_GFLAGS is ON by default on OSX, we need to install or or disable it explicitly. it'd be better to install it for better coverage of the CI test. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

tchaikov force-pushed the wip-cmake branch from 4874c3e to f1f21e1 Compare December 13, 2019 11:57

tchaikov force-pushed the wip-cmake branch from f1f21e1 to 04f3573 Compare December 13, 2019 12:48

facebook-github-bot reviewed Dec 13, 2019

View reviewed changes

facebook-github-bot closed this in ac304ad Dec 13, 2019

facebook-github-bot added the Merged label Dec 13, 2019

bh1xuw mentioned this pull request Mar 22, 2020

ci(rocks-sys): fix static build without gflags bh1xuw/rust-rocks#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: do not build tests for Release build and cleanups #5916

cmake: do not build tests for Release build and cleanups #5916

tchaikov commented Oct 14, 2019

riversand963 commented Oct 14, 2019

tchaikov commented Oct 24, 2019

tchaikov commented Nov 13, 2019

tchaikov commented Dec 10, 2019 •

edited

Loading

riversand963 commented Dec 10, 2019

siying Dec 10, 2019

tchaikov Dec 11, 2019

siying commented Dec 12, 2019

tchaikov commented Dec 13, 2019

facebook-github-bot left a comment

siying commented Dec 13, 2019

facebook-github-bot commented Dec 13, 2019

cmake: do not build tests for Release build and cleanups #5916

cmake: do not build tests for Release build and cleanups #5916

Conversation

tchaikov commented Oct 14, 2019

riversand963 commented Oct 14, 2019

tchaikov commented Oct 24, 2019

tchaikov commented Nov 13, 2019

tchaikov commented Dec 10, 2019 • edited Loading

riversand963 commented Dec 10, 2019

siying Dec 10, 2019

Choose a reason for hiding this comment

tchaikov Dec 11, 2019

Choose a reason for hiding this comment

siying commented Dec 12, 2019

tchaikov commented Dec 13, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

siying commented Dec 13, 2019

facebook-github-bot commented Dec 13, 2019

tchaikov commented Dec 10, 2019 •

edited

Loading