cmake: cmake related cleanups #5662

tchaikov · 2019-08-01T08:36:09Z

cmake: use the builtin FindBzip2.cmake from CMake
cmake: require CMake v3.5.1
cmake: add imported target for 3rd party libraries
cmake: extract ReadVersion.cmake out and refactor it

maysamyabandeh · 2019-08-01T20:16:29Z

@hliu18 @burtonli Would appreciate it if you could verify the suggested changes.

hliu18 · 2019-08-01T20:25:24Z

Looks fine to me.

yiwu-arbug · 2019-08-01T22:57:39Z

CMakeLists.txt

-enable_language(ASM)
+cmake_minimum_required(VERSION 3.5.1)
+project(rocksdb
+  VERSION 6.2.0


is it possible to get version from version.h?

it's better to not duplicate in two places. Is "VERSION 6.2.0" needed here?

agreed. i extracted ReadVersion.cmake out, and use it instead.

burtonli · 2019-08-02T00:46:35Z

cmake/modules/Findlz4.cmake

-# LZ4_INCLUDE_DIR - where to find lz4.h, etc.
-# LZ4_LIBRARIES - List of libraries when using lz4.
-# LZ4_FOUND - True if lz4 found.
+# lz4_INCLUDE_DIR - where to find lz4.h, etc.


lz4_INCLUDE_DIRS?

thanks. fixed.

burtonli · 2019-08-02T00:51:09Z

CMakeLists.txt

    add_definitions(-DBZIP2)
-    include_directories(${BZIP2_INCLUDE_DIR})
+    include_directories(${BZIP2_INCLUDE_DIRS})


should we also update Findbzip2.cmake accordingly?

Findbzip2.cmake is removed in the same commit. please refer the commit message for more details.

FindBzip2.cmake has been shipped along with CMake since v2.6.0, so let's just use it instead of the homebrew one. please note, in CMake 3.5.1, FindBzip2.cmake exposed `BZIP2_INCLUDE_DIR`, see https://github.com/Kitware/CMake/blob/v3.5.1/Modules/FindBZip2.cmake. while since CMake 3.12.1, FindBzip2.cmake exposes both Bzip2::Bzip2 and `BZIP2_INCLUDE_DIRS`, see https://github.com/Kitware/CMake/blob/v3.12.1/Modules/FindBZip2.cmake Signed-off-by: Kefu Chai <tchaikov@gmail.com>

* require 3.5.1 * use project(... LANGUAGES ..) to silence warnings regarding to CMP0048 * use imported target of ZLIB::ZLIB, as it's available since 3.5.1 cmake 3.5.1 is offered by ubuntu xenial, and cmake3.13 is offered by EPEL7. so it should be safe to bump the required cmake version. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

tchaikov · 2019-08-02T03:40:50Z

changelog

s/lz4_INCLUDE_DIR/lz4_INCLUDE_DIRS/
add imported target for more Find*.cmake. and use it for VERSION in project() command
extract ReadVersion.cmake out

* so it would be simpler to import these libraries * lower case the {lz4,snappy,zstd}_* variables to follow the convention of CMake that Find<name>.cmake should expose names of <name>_*. instead of <NAME>_*. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

for couple reasons * modularize the cmake file, to extract the subroutines into their own modules. this approach also avoid pollution the global namespace with the local variables. * use `<PROJECT_NAME>_VERSION_*` instead of `ROCKSDB_VERSION_*` to follow CMake conventions. please note, the project name of rocksdb in this CMakefile script is "rocksdb" instead of "ROCKSDB". * refactor the code to read the version number using a loop, let repeating this way. Signed-off-by: Kefu Chai <tchaikov@gmail.com>

maysamyabandeh · 2019-08-05T17:03:24Z

@tchaikov could you please update the PR description with a summary of the changes.

maysamyabandeh · 2019-08-05T17:04:39Z

@burtonli @yiwu-arbug Thanks for the review. In the PR good for landing?

tchaikov · 2019-08-05T17:42:45Z

@maysamyabandeh sure. updated.

burtonli · 2019-08-05T22:41:39Z

LGTM

yiwu-arbug · 2019-08-06T00:24:17Z

LGTM

facebook-github-bot

@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-08-06T04:08:39Z

@maysamyabandeh merged this pull request in cc9fa7f.

rocksdb needs CMake 3.5.1 since facebook/rocksdb#5662 Signed-off-by: Kefu Chai <kchai@redhat.com>

rocksdb needs CMake 3.5.1 since facebook/rocksdb#5662 Fixes: https://tracker.ceph.com/issues/41253 Signed-off-by: Kefu Chai <kchai@redhat.com>

rocksdb needs CMake 3.5.1 since facebook/rocksdb#5662 Fixes: https://tracker.ceph.com/issues/41253 Signed-off-by: Kefu Chai <kchai@redhat.com> (cherry picked from commit 52277bf)

This reverts commit cc9fa7f.

merryChris · 2019-11-15T14:14:01Z

CMakeLists.txt

@@ -93,43 +93,38 @@ else()
  if(WITH_SNAPPY)
    find_package(snappy REQUIRED)
    add_definitions(-DSNAPPY)
-    include_directories(${SNAPPY_INCLUDE_DIR})


@tchaikov Why do you remove this line? Seems that cannot find it in include path. Errors like
/root/cs/suzanwen/repos/C++/storage/rocksdb_xxx/rocksdb/util/compression.h:31:20: fatal error: snappy.h: No such file or directory #include <snappy.h> ^ compilation terminated.

because, IIUC, snappy::snappy should be able to populate the "INSTALL_INTERFACE" property to the library/executable linked against it, so cmake will determine the correct build property of consumer of snappy::snappy and library publicly linked against this library.

rocksdb/cmake/modules/Findsnappy.cmake

Line 28 in f65ec09

INTERFACE_INCLUDE_DIRECTORIES ${snappy_INCLUDE_DIRS})

Yeah, that's right for the most targets, such as ROCKSDB_STATIC_LIB. But not for TESTUTILLIB. Plz have a glance at the following snippet.

rocksdb/CMakeLists.txt

Line 1072 in f65ec09

add_library(${TESTUTILLIB} STATIC ${TESTUTIL_SOURCE})

snappy.h was included in db/db_test_util.cc(just as the source file in target TESTUTILLIB). Porbably we need make it works fine with target_link_libraries(${TESTUTILLIB} ${LIBS}).

@tchaikov #6043 pr as it is, plz make a review.

Summary: - cmake: use the builtin FindBzip2.cmake from CMake - cmake: require CMake v3.5.1 - cmake: add imported target for 3rd party libraries - cmake: extract ReadVersion.cmake out and refactor it Pull Request resolved: facebook#5662 Differential Revision: D16660974 Pulled By: maysamyabandeh fbshipit-source-id: 681594910e74253251fe14ad0befc41a4d0f4fd4

wbrown-lg · 2019-12-13T17:01:19Z

cmake/modules/ReadVersion.cmake

+# Read rocksdb version from version.h header file.
+
+function(get_rocksdb_version version_var)
+  file(READ "${CMAKE_SOURCE_DIR}/include/rocksdb/version.h" version_header_file)


This should be CMAKE_CURRENT_SOURCE_DIR, not CMAKE_SOURCE_DIR. This particular variable breaks a rockdb source build that's been vendor'ed via add_subdirectory(vendor/rocksdb) in a parent source tree, as it instead tries to get the include/rocksdb/version.h in the parent project.

wbrown-lg · 2019-12-13T17:02:40Z

As noted in an inline comment, this breaks the rocksdb build if we do a CMake add_subdirectory on rocksdb.

tchaikov · 2019-12-13T18:05:27Z

@wbrown-lg thanks for pointing out this. Will fix it shortly.

tchaikov · 2019-12-14T15:42:25Z

@wbrown-lg this was fixed by adcf920 . could you use a commit which includes that change instead?

facebook-github-bot added the CLA Signed label Aug 1, 2019

yiwu-arbug reviewed Aug 1, 2019

View reviewed changes

burtonli reviewed Aug 2, 2019

View reviewed changes

tchaikov added 2 commits August 2, 2019 11:33

tchaikov force-pushed the wip-cmake branch from 13baf14 to 05d75eb Compare August 2, 2019 03:34

tchaikov force-pushed the wip-cmake branch from 05d75eb to c7d85ea Compare August 2, 2019 16:33

tchaikov added 2 commits August 3, 2019 00:34

tchaikov force-pushed the wip-cmake branch from c7d85ea to f7765d1 Compare August 2, 2019 16:34

facebook-github-bot reviewed Aug 6, 2019

View reviewed changes

facebook-github-bot closed this in cc9fa7f Aug 6, 2019

tchaikov deleted the wip-cmake branch August 6, 2019 02:59

facebook-github-bot added the Merged label Aug 6, 2019

tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 10, 2019

qa/workunits/rados/test_envlibrados_for_rocksdb: install newer cmake

7637a1d

rocksdb needs CMake 3.5.1 since facebook/rocksdb#5662 Signed-off-by: Kefu Chai <kchai@redhat.com>

tchaikov mentioned this pull request Aug 10, 2019

qa/workunits/rados/test_envlibrados_for_rocksdb: install newer cmake ceph/ceph#29584

Merged

3 tasks

tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 11, 2019

qa/workunits/rados/test_envlibrados_for_rocksdb: install newer cmake

e60d159

rocksdb needs CMake 3.5.1 since facebook/rocksdb#5662 Signed-off-by: Kefu Chai <kchai@redhat.com>

ajkr added a commit to cockroachdb/rocksdb that referenced this pull request Nov 1, 2019

Revert "cmake: cmake related cleanups (facebook#5662)"

fd2799f

This reverts commit cc9fa7f.

merryChris reviewed Nov 15, 2019

View reviewed changes

wbrown-lg reviewed Dec 13, 2019

View reviewed changes

siying mentioned this pull request Dec 16, 2019

CMake error with PyTorch for 6.5.2 #6179

Closed

tabokie mentioned this pull request May 12, 2022

work with 6.29.tikv tikv/titan#246

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: cmake related cleanups #5662

cmake: cmake related cleanups #5662

tchaikov commented Aug 1, 2019 •

edited

Loading

maysamyabandeh commented Aug 1, 2019

hliu18 commented Aug 1, 2019

yiwu-arbug Aug 1, 2019

hliu18 Aug 1, 2019

tchaikov Aug 2, 2019

burtonli Aug 2, 2019 •

edited

Loading

tchaikov Aug 2, 2019

burtonli Aug 2, 2019

tchaikov Aug 2, 2019

tchaikov commented Aug 2, 2019

maysamyabandeh commented Aug 5, 2019

maysamyabandeh commented Aug 5, 2019

tchaikov commented Aug 5, 2019

burtonli commented Aug 5, 2019

yiwu-arbug commented Aug 6, 2019

facebook-github-bot left a comment

facebook-github-bot commented Aug 6, 2019

merryChris Nov 15, 2019

tchaikov Nov 16, 2019

merryChris Nov 17, 2019

merryChris Nov 17, 2019

wbrown-lg Dec 13, 2019

wbrown-lg commented Dec 13, 2019

tchaikov commented Dec 13, 2019

tchaikov commented Dec 14, 2019

cmake: cmake related cleanups #5662

cmake: cmake related cleanups #5662

Conversation

tchaikov commented Aug 1, 2019 • edited Loading

maysamyabandeh commented Aug 1, 2019

hliu18 commented Aug 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burtonli Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Aug 2, 2019

maysamyabandeh commented Aug 5, 2019

maysamyabandeh commented Aug 5, 2019

tchaikov commented Aug 5, 2019

burtonli commented Aug 5, 2019

yiwu-arbug commented Aug 6, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbrown-lg commented Dec 13, 2019

tchaikov commented Dec 13, 2019

tchaikov commented Dec 14, 2019

tchaikov commented Aug 1, 2019 •

edited

Loading

burtonli Aug 2, 2019 •

edited

Loading