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

merryChris · 2019-11-17T14:21:57Z

No description provided.

tchaikov · 2019-11-17T15:57:06Z

CMakeLists.txt

@@ -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})


@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.

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

@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.

@tchaikov thanks for reminding.

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

tchaikov · 2019-11-17T15:57:49Z

cmake/modules/ReadVersion.cmake

@@ -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)


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?

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

yeah, that's it.

tchaikov · 2019-11-17T15:58:18Z

db/compaction/compaction_iterator_test.cc

@@ -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 = {{}})


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

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

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?

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.

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.

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. ;)

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 .

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.

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

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

tchaikov · 2019-11-17T15:58:23Z

util/filter_bench.cc

@@ -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("");


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

pdillinger · 2019-11-18T16:48:06Z

If you get the cmake updates out of this PR (now in #6045) I'll pull in the compaction_iterator_test.cc and filter_bench.cc updates.

merryChris · 2019-11-19T03:43:24Z

If you get the cmake updates out of this PR (now in #6045) I'll pull in the compaction_iterator_test.cc and filter_bench.cc updates.

That's it. Do I need to separate these two changes into another PR, since sorry that I cannot update this PR anymore?

@merryChris

Summary: Taken from @merryChris in facebook#6043 Stackoverflow ref on {{}} vs. {}: https://stackoverflow.com/questions/26947704/implicit-conversion-failure-from-initializer-list Note that .clear() does not empty out an ostringstream, but .str("") suffices because we don't have to worry about clearing error flags. Test Plan: make check, manual run of filter_bench

pdillinger · 2019-11-19T23:29:28Z

Moved to #6053

Summary: Taken from merryChris in #6043 Stackoverflow ref on {{}} vs. {}: https://stackoverflow.com/questions/26947704/implicit-conversion-failure-from-initializer-list Note to reader: .clear() does not empty out an ostringstream, but .str("") suffices because we don't have to worry about clearing error flags. Pull Request resolved: #6053 Test Plan: make check, manual run of filter_bench Differential Revision: D18602259 Pulled By: pdillinger fbshipit-source-id: f6190f83b8eab4e80e7c107348839edabe727841

merryChris · 2019-11-20T03:17:17Z

Moved to #6053

Thanks a lot.

Summary: Taken from merryChris in facebook#6043 Stackoverflow ref on {{}} vs. {}: https://stackoverflow.com/questions/26947704/implicit-conversion-failure-from-initializer-list Note to reader: .clear() does not empty out an ostringstream, but .str("") suffices because we don't have to worry about clearing error flags. Pull Request resolved: facebook#6053 Test Plan: make check, manual run of filter_bench Differential Revision: D18602259 Pulled By: pdillinger fbshipit-source-id: f6190f83b8eab4e80e7c107348839edabe727841

Compatible changes with g++4.9.2 and cmake.

79327bf

facebook-github-bot added the CLA Signed label Nov 17, 2019

This was referenced Nov 17, 2019

cmake: cmake related cleanups #5662

Closed

More improvements to filter_bench #5968

Closed

tchaikov reviewed Nov 17, 2019

View reviewed changes

pdillinger self-assigned this Nov 18, 2019

pdillinger mentioned this pull request Nov 19, 2019

Fixes for g++ 4.9.x compatibility #6053

Closed

pdillinger closed this Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

merryChris commented Nov 17, 2019

tchaikov Nov 17, 2019

merryChris Nov 18, 2019

tchaikov Nov 18, 2019

merryChris Nov 18, 2019

merryChris Nov 18, 2019

tchaikov Nov 17, 2019

merryChris Nov 18, 2019 •

edited

Loading

tchaikov Nov 18, 2019

merryChris Nov 18, 2019

tchaikov Nov 17, 2019

merryChris Nov 18, 2019

tchaikov Nov 18, 2019 •

edited

Loading

merryChris Nov 18, 2019

tchaikov Nov 18, 2019 •

edited

Loading

merryChris Nov 18, 2019

tchaikov Nov 18, 2019

merryChris Nov 18, 2019

pdillinger Nov 18, 2019

tchaikov Nov 18, 2019

tchaikov Nov 17, 2019

merryChris Nov 18, 2019

pdillinger commented Nov 18, 2019

merryChris commented Nov 19, 2019

pdillinger commented Nov 19, 2019

merryChris commented Nov 20, 2019

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

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

Conversation

merryChris commented Nov 17, 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

Choose a reason for hiding this comment

merryChris Nov 18, 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

Choose a reason for hiding this comment

tchaikov Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Nov 18, 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

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

pdillinger commented Nov 18, 2019

merryChris commented Nov 19, 2019

pdillinger commented Nov 19, 2019

merryChris commented Nov 20, 2019

merryChris Nov 18, 2019 •

edited

Loading

tchaikov Nov 18, 2019 •

edited

Loading

tchaikov Nov 18, 2019 •

edited

Loading