foundationdb icon indicating copy to clipboard operation
foundationdb copied to clipboard

Fix rocks clang build

Open sears opened this issue 2 years ago • 11 comments

I tested this in our developer environment with cmk and ccmk after removing the following environment variables (these environment variables are not set by default in the developer container, but were inadvertently being set by something else):

  - USE_VALGRIND=0
  - USE_VALGRIND_FOR_CTEST=0
  - USE_WERROR=0
  - USE_GPERFTOOLS=0
  - CXX=/opt/rh/devtoolset-8/root/usr/bin/g++
  - CC=/opt/rh/devtoolset-8/root/usr/bin/gcc
  - USE_LD=DEFAULT
  - USE_LIBCXX=0
  - # CXX=/usr/local/bin/clang++
  - # CC=/usr/local/bin/clang
  - # USE_LD=LLD
  - # USE_LIBCXX=1

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • [ ] The PR has a description, explaining both the problem and the solution.
  • [ ] The description mentions which forms of testing were done and the testing seems reasonable.
  • [ ] Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • [ ] This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • [ ] There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

sears avatar Sep 21 '22 18:09 sears

Doxense CI Report for Windows 10

  • Commit ID: 1501ae4940e8a61b917599cb6405fd18d0188ec0
  • Result: :heavy_check_mark: SUCCEEDED
  • Build Logs (available for 30 days)

fdb-windows-ci avatar Sep 21 '22 18:09 fdb-windows-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 1501ae4940e8a61b917599cb6405fd18d0188ec0
  • Duration 0:47:04
  • Result: :x: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 21 '22 19:09 foundationdb-ci

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 1501ae4940e8a61b917599cb6405fd18d0188ec0
  • Duration 2:09:27
  • Result: :x: FAILED
  • Error: Error while executing command: if $(grep -q -- "--- FAIL:" ${CODEBUILD_SRC_DIR}/fdb-kubernetes-tests/logs/*.log); then echo "TESTS FAILED SEE THESE LOGS:"; echo ; grep -l -- "--- FAIL:" ${CODEBUILD_SRC_DIR}/fdb-kubernetes-tests/logs/*.log; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 21 '22 20:09 foundationdb-ci

The root cause of why we are not being able to forward the corresponding flags to RocksDB is that:

  1. We are determining the compiler/linker flags in ConfigureCompiler.cmake. TheConfigureCompiler.cmake is using add_compile_options and add_link_options. These commands will not affect CMAKE_CXX_FLAGS, etc. Instead, they will inject those options to each target appears after these commands.
  2. RocksDB, unfortunately, is not one of the targets within our environment, and CMAKE_CXX_FLAGS, etc. are not being affected by ConfigureCompiler.cmake.

xis19 avatar Sep 21 '22 21:09 xis19

RocksDB is built using cmake. I would suggest to use FetchContent to get the RocksDB source (I think this can be done in a way so it only is downloaded if it is not available locally). Then you could just add rocksdb as a normal target.

sfc-gh-mpilman avatar Sep 21 '22 21:09 sfc-gh-mpilman

RocksDB is built using cmake. I would suggest to use FetchContent to get the RocksDB source (I think this can be done in a way so it only is downloaded if it is not available locally). Then you could just add rocksdb as a normal target.

If we use fetch content, would our compile/link flags pollutes the RocksDB environment?

xis19 avatar Sep 21 '22 21:09 xis19

yes -- though I think that's what we want.

The cleanest solution would be to only set compiler, stdlib flags etc in ConfigureCompiler and add all other flags to the flow target. That should work pretty well, though it would be some effort

sfc-gh-mpilman avatar Sep 21 '22 21:09 sfc-gh-mpilman

Why do we need to compile RocksDB at FoundationDB build time?

ammolitor avatar Sep 21 '22 21:09 ammolitor

We don't need to. However, we need to use the same stdlib for rocks and for fdb. Otherwise we get in trouble. The easiest short-term solution is to compile rocks when we compile fdb. The correct solution imho is to cache the build result and reuse it.

We probably don't want to have a binary in the docker container because the number of desired build configuration could be large (we might want to be able to have a debug build, or an address sanitizer build, etc...). If we have a cache every possible configuration should only be built once.

sfc-gh-mpilman avatar Sep 21 '22 22:09 sfc-gh-mpilman

I think a possible approach is to let the compiler/library configuration goes to CMAKE_CXX_FLAGS while the remaining can stay in COMPILE_OPTIONS.

xis19 avatar Sep 22 '22 03:09 xis19

So, we currently always recompile RocksDB, and optionally download it if the source is not available locally.

I'm not sure just passing the flags through to RocksDB will work. Their cmake configuration non-trivial.

They definitely support clang, but I couldn't figure out what their preferred approach is for using libcxx. I confirmed that the approach used (edit: to tell them to use libcxx) in this PR works under Linux with clang and g++, under windows, and in MacOS.

sears avatar Sep 22 '22 20:09 sears

Yeah; that looks OK to me.

sears avatar Sep 26 '22 18:09 sears

Spoke too soon. Comment in other PR.

sears avatar Sep 26 '22 18:09 sears

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 1501ae4940e8a61b917599cb6405fd18d0188ec0
  • Duration 0:42:37
  • Result: :x: FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 26 '22 19:09 foundationdb-ci

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 1501ae4940e8a61b917599cb6405fd18d0188ec0
  • Duration 2:10:29
  • Result: :x: FAILED
  • Error: Error while executing command: if $(grep -q -- "--- FAIL:" ${CODEBUILD_SRC_DIR}/fdb-kubernetes-tests/logs/*.log); then echo "TESTS FAILED SEE THESE LOGS:"; echo ; grep -l -- "--- FAIL:" ${CODEBUILD_SRC_DIR}/fdb-kubernetes-tests/logs/*.log; exit 1; fi. Reason: exit status 1
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 26 '22 21:09 foundationdb-ci

Can we close this PR?

jzhou77 avatar Oct 03 '22 16:10 jzhou77