Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatible changes with g++4.9.2 and cmake. #6043

Closed
wants to merge 1 commit into from
Closed

Compatible changes with g++4.9.2 and cmake. #6043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ else()
if(WITH_ZSTD)
find_package(zstd REQUIRED)
add_definitions(-DZSTD)
include_directories(${ZSTD_INCLUDE_DIR})
list(APPEND THIRDPARTY_LIBS zstd::zstd)
endif()
endif()
Expand Down Expand Up @@ -1070,6 +1069,7 @@ if(WITH_TESTS)
add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND})
set(TESTUTILLIB testutillib${ARTIFACT_SUFFIX})
add_library(${TESTUTILLIB} STATIC ${TESTUTIL_SOURCE})
target_link_libraries(${TESTUTILLIB} ${LIBS})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merryChris this makes sense, as some source files (db/db_test_util.h for instance) in ${TESTUTILLIB} actually references headers used by ROCKSDB_STATIC_LIB/ROCKSDB_SHARED_LIB, but i'd suggest move the cmake changes into a separated commit. and explain the reason why we need this change in the commit message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, I will pick up the cmake changes in a separated commit. Close this pr temporarily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merryChris JFYI, there is no need to create a new PR for addressing comments in existing one. you can always git push -f to your remote branch for updating a PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov thanks for reminding.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov I am not sure why I cannot update PR automatically. Sorry for that, and plz have a review at #6045.

if(MSVC)
set_target_properties(${TESTUTILLIB} PROPERTIES COMPILE_FLAGS "/Fd${CMAKE_CFG_INTDIR}/testutillib${ARTIFACT_SUFFIX}.pdb")
endif()
Expand Down
2 changes: 1 addition & 1 deletion cmake/modules/ReadVersion.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# 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)
file(READ "${CMAKE_CURRENT_SOURCE_DIR}/include/rocksdb/version.h" version_header_file)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Author

@merryChris merryChris Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm trying to build a distributed storage system based on rocksdb, and my codebase directory structure just like this.

/path/to/rocskdb_xxx$ tree -L 1
.
|-build.sh
|-CMakeLists.txt
|-my_code/
|-rocksdb/
|-something_else

My project name is rocksdb_xxx. While I'm building it at the root path, the cmake file path above is /path/to/rocskdb_xxx/include/rocksdb/version.h, which doesn't work. ${CMAKE_CURRENT_SOURCE_DIR} is a friendly option path, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. presumably you include rocksdb project using add_subdirectory() which points CMAKE_SOURCE_DIR to /path/to/rocskdb_xxx.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's it.

foreach(component MAJOR MINOR PATCH)
string(REGEX MATCH "#define ROCKSDB_${component} ([0-9]+)" _ ${version_header_file})
set(ROCKSDB_VERSION_${component} ${CMAKE_MATCH_1})
Expand Down
2 changes: 1 addition & 1 deletion db/compaction/compaction_iterator_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ class TestSnapshotChecker : public SnapshotChecker {
public:
explicit TestSnapshotChecker(
SequenceNumber last_committed_sequence,
const std::unordered_map<SequenceNumber, SequenceNumber>& snapshots = {})
const std::unordered_map<SequenceNumber, SequenceNumber>& snapshots = {{}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this change to a separated commit, and explain the rationale behind this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will separate them into two pr. Plz refer to the issue #3522, it may not friendly for g++4.9.

Copy link
Contributor

@tchaikov tchaikov Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the existing code is compliant to C++11. and why would you want to compile compile the test with a 4 years old compiler?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, it seems a sad history problem. If you would not like to make a compatibility, it's okay to make a local fit for me.

Copy link
Contributor

@tchaikov tchaikov Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's the maintainers' call to include your fix or not. i am just wondering why. the reason i was curious is that, normally, the reason why we need to stick to an older version of compiler is just we have to compile our executables/libraries for supported older platforms/distros. but the tests are not in this scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Briefly speaking, almost all projects/libs in our company are complied with g++4.9.2. As for me, I have to maintain compatible api lib version under g++4.9.2, 5.4.0, and 6.3.0, which also troubles me a lot. But this is life anyway. ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but compaction_iterator_test.cc is not included by rocksdb library. so even if it does not compile, you can still compile and link against rocksdb using GCC 4.9.2 .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we want to make use of built-in db_bench_tool as data generator, with the option WITH_TESTS=ON. But as you see, ${TESTS} sources like compaction_iterator_test is involved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/26947704/implicit-conversion-failure-from-initializer-list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdillinger thanks for the link! so it's C++14 compliant.

: last_committed_sequence_(last_committed_sequence),
snapshots_(snapshots) {}

Expand Down
3 changes: 2 additions & 1 deletion util/filter_bench.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,8 @@ double FilterBench::RandomQueryTest(uint32_t inside_threshold, bool dry_run,
}

if (!dry_run) {
fp_rate_report_ = std::ostringstream();
fp_rate_report_.clear();
fp_rate_report_.str("");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reson as above, and here is the conversation with @pdillinger #5968.

uint64_t q = 0;
uint64_t fp = 0;
double worst_fp_rate = 0.0;
Expand Down