foundationdb icon indicating copy to clipboard operation
foundationdb copied to clipboard

Fix RocksDB link issue

Open xis19 opened this issue 2 years ago • 6 comments

By default, RocksDB is using its own compile/link flags, no matter how
FDB flags are. This led to the issue that if FDB decides to use
clang/ldd/libc++, RocksDB will pick up the compiler/linker but still use
libstdc++, which is incompatible to libc++, causing Symobl Missing error
during the link stage.

With this patch, if FDB uses libc++, then the information is stored in
CMAKE_CXX_FLAGS and being forwarded to RocksDB. RocksDB will then use
libc++ and compatible with FDB.

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)

xis19 avatar Sep 22 '22 23:09 xis19

This is tested by 1) build FDB using clang/ldd/libc++ 2) build FDB using gcc/ld/libstdc++ in okteto enviroment. Both build succeed.

xis19 avatar Sep 22 '22 23:09 xis19

Doxense CI Report for Windows 10

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

fdb-windows-ci avatar Sep 23 '22 00:09 fdb-windows-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: fa0fc1f34fbc47f0bf621edf0d2c9c6f89592380
  • Duration 0:49:35
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 23 '22 00:09 foundationdb-ci

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 0ebd3fbed08a950faad1a979e458c7df1baa1660
  • Duration 1:19:59
  • Result: :white_check_mark: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)
  • Build Artifact (available for 30 days)

foundationdb-ci avatar Sep 23 '22 00:09 foundationdb-ci

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

  • Commit ID: fa0fc1f34fbc47f0bf621edf0d2c9c6f89592380
  • Duration 2:11:57
  • 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 23 '22 01:09 foundationdb-ci

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

  • Commit ID: 0ebd3fbed08a950faad1a979e458c7df1baa1660
  • Duration 2:50:50
  • 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 23 '22 02:09 foundationdb-ci

This broke the clang build for me. I'm using the current build of dev container; default environment variables + okteto distcc stuff:

  - DISTCC_HOSTS=--randomize ...
  - DISTCC_FALLBACK=0
  - CCACHE_PREFIX=distcc
  - CCACHE_DIR=/var/cache/ccache/sears
  - BOOST_ROOT=/opt/boost_1_78_0
  - JOSHUA_USER=sears
  - USER=sears
  - KUBECONFIG=/root/.kube/config
  - NINJA_STATUS=[STARTED %s, FINISHED %f OF %t IN %e SEC]
  - DOCKER_BUILDKIT=1
  - ARTIFACTORY_URL=...

(I replaced some host names with ... when I pasted that in).

$ ccmk
...
/root/build_output/zstd/programs/util.c:661:2049: error: array index 2 is past the end of the array (which contains 2 elements) [-Werror,-Warray-bounds]                                 

We could define -Wno-array-bounds for all of FDB, but I'd rather not.

https://github.com/apple/foundationdb/pull/8265 avoided this class of problem by passing through as few compiler flags as possible. Any other ideas?

sears avatar Sep 26 '22 18:09 sears

@ammolitor do we have a CI build for clang? I'm trying to figure out how this keeps passing in CI, but failing in different ways in two different okteto setups.

If there was ground truth for what we expect to do to compile the code, then whoever has the divergent environment could just fix their setup.

sears avatar Sep 26 '22 19:09 sears

We do not have a CI build for clang all CI and Releases (as of this comment) are compiled with gcc

ammolitor avatar Sep 26 '22 19:09 ammolitor

This broke the clang build for me. I'm using the current build of dev container; default environment variables + okteto distcc stuff:

  - DISTCC_HOSTS=--randomize ...
  - DISTCC_FALLBACK=0
  - CCACHE_PREFIX=distcc
  - CCACHE_DIR=/var/cache/ccache/sears
  - BOOST_ROOT=/opt/boost_1_78_0
  - JOSHUA_USER=sears
  - USER=sears
  - KUBECONFIG=/root/.kube/config
  - NINJA_STATUS=[STARTED %s, FINISHED %f OF %t IN %e SEC]
  - DOCKER_BUILDKIT=1
  - ARTIFACTORY_URL=...

(I replaced some host names with ... when I pasted that in).

$ ccmk
...
/root/build_output/zstd/programs/util.c:661:2049: error: array index 2 is past the end of the array (which contains 2 elements) [-Werror,-Warray-bounds]                                 

We could define -Wno-array-bounds for all of FDB, but I'd rather not.

#8265 avoided this class of problem by passing through as few compiler flags as possible. Any other ideas?

Yes, it seems that zstd is introduced these days and clang is not happy with it. Yet I think the root cause of the issue is because

-Warray-bounds

is set by default to be enabled, so no matter what fix we apply, clang will never feel happy about ZSTD. I will fix it by add an compile option to ZSTD::ZSTD target, which strictly restrict the warning to ZSTD.

xis19 avatar Sep 27 '22 20:09 xis19