add CMake's option for bindings #5318

PierreZ · 2021-08-02T10:04:54Z

This PR is related to #5218.

The goal of this PR is to add CMake's options to choose which bindings you want to build at compile time.
It introduces one CMake option(defaulting to ON) per binding:

BUILD_C_BINDING
BUILD_JAVA_BINDING
BUILD_PYTHON_BINDING
BUILD_DOCUMENTATION
BUILD_GO_BINDING
BUILD_RUBY_BINDING

Code-Reviewer Section

The general 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 master 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)

foundationdb-ci · 2021-08-02T10:29:28Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build
Commit ID: 0cd7b79
Result: SUCCEEDED
Build Logs (available for 7 days)

sfc-gh-mpilman

The dependencies with bindings (especially python) is sadly quite complex...

Apart from the comments I left, I am also not sure whether the output at the end will be correct (not sure whether it was before either, so I won't insist on this getting fixed in this PR). For example: If the C bindings are not build but go bindings is enabled, the output will tell a user that the go bindings will be built (even though there's a dependency).

So I think the logic has to be:

Python not found -> don't build any bindings
C bindings are disabled -> don't build any bindings
If Python is found and the C bindings are enabled, all other bindings can be disabled/enabled individually (if their dependencies are found).

Also, I would prefer if cmake wouldn't change the option. Instead, cmake should use the option to initialize some other variable, but optimally an option would be only read-only.

sfc-gh-mpilman · 2021-08-03T17:26:39Z

CMakeLists.txt

-if(WITH_PYTHON)
-  add_subdirectory(bindings)
-endif()
+add_subdirectory(bindings)


All bindings depend on python. We use a python script to generate some assembly code that is required for the C bindings

Thanks for the tips 👍

sfc-gh-mpilman · 2021-08-03T17:30:30Z

cmake/FDBComponents.cmake

-else()
-  #message(FATAL_ERROR "Could not found a suitable python interpreter")
+option(BUILD_PYTHON_BINDING "build python binding" ON)
+if(NOT BUILD_PYTHON_BINDING)


Multiple things in the build system (at least the C bindings and the documentation) depend on python. So I think you need to run find_package(Python COMPONENTS Interpreter) even if you don't want to build the python bindings.

Thanks for the tips 👍

PierreZ · 2021-08-05T13:57:51Z

I should have taken all your comments into consideration, PTAL 😄

foundationdb-ci · 2021-08-05T13:58:39Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build
Commit ID: bd75251
Result: FAILED
Build Logs (available for 7 days)

foundationdb-ci · 2021-08-05T13:58:45Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build-macos
Commit ID: bd75251
Result: FAILED
Build Logs (available for 7 days)

foundationdb-ci · 2021-08-05T13:59:16Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build
Commit ID: 3692d99
Result: FAILED
Build Logs (available for 7 days)

foundationdb-ci · 2021-08-05T15:19:46Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build
Commit ID: e52fc41
Result: FAILED
Build Logs (available for 7 days)

foundationdb-ci · 2021-08-05T16:23:07Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build-macos
Commit ID: e52fc41
Result: FAILED
Build Logs (available for 7 days)

sfc-gh-mpilman

Looks good, but breaks the documentation build

sfc-gh-mpilman · 2021-08-05T16:54:30Z

cmake/FDBComponents.cmake

+option(BUILD_DOCUMENTATION "build documentation" ON)
+if (BUILD_DOCUMENTATION AND WITH_PYTHON)


we use ${Python3_EXECUTABLE} in documentation/CMakeLists.txt. Removing the call to find_package(Python3) will make this then fail (so the documentation doesn't build anymore.

The easiest solution would be to reintroduce that call, though that is not pretty. Alternatively you could do the following:

Replace ${Python3_EXECUTABLE}

Only set WITH_PYTHON to ON if Python_VERSION_MAJOR is 3 or:

Suggested change

option(BUILD_DOCUMENTATION "build documentation" ON)

if (BUILD_DOCUMENTATION AND WITH_PYTHON)

option(BUILD_DOCUMENTATION "build documentation" ON)

if (BUILD_DOCUMENTATION AND WITH_PYTHON AND Python_VERSION_MAJOR EQUAL 3)

PierreZ · 2021-08-05T20:00:15Z

Should be better now, thanks! I tried downloading the log archives from the build, but they were empty, do you know why?

foundationdb-ci · 2021-08-05T20:27:20Z

AWS CodeBuild CI Report

CodeBuild project: foundationdb-pull-request-build
Commit ID: 415bf2a
Result: SUCCEEDED
Build Logs (available for 7 days)

sfc-gh-mpilman · 2021-08-06T19:05:43Z

I tried downloading the log archives from the build, but they were empty, do you know why?

The build logs are only available for a very short amount of time (the "available for 7 days" message is not accurate). They usually get deleted within hours -- but sometimes even within minutes. But the Linux build succeeded, macOS CI is not ready yet and the result can be ignored.

PierreZ · 2021-08-09T08:34:55Z

The build logs are only available for a very short amount of time (the "available for 7 days" message is not accurate). They usually get deleted within hours -- but sometimes even within minutes. But the Linux build succeeded, macOS CI is not ready yet and the result can be ignored.

Thanks for your help @sfc-gh-mpilman 👍 Do you have any policy/rules regarding cherry-pick for release-7.0 ?

sfc-gh-mpilman · 2021-08-09T16:33:44Z

Do you have any policy/rules regarding cherry-pick for release-7.0 ?

Typically cherry-picks have to either be strict bug fixes or changes that are required for a business reason (very loosely defined as "we need this change in order to fix an imminent problem with a production system"). Cherry-picks also are not allowed to cause any behavior-change and need to go through more testing and they require two review-approvals.

I think this change is ok to back port to 7.0. But the following will be needed:

Link to this PR in the description of the cherry-pick PR
Describe the testing that you've done. I think for a cherry-pick you would need to test this on macOS and on Linux. Something like "verified manually X, Y, and Z" would be sufficient -- there's no hard rules, you just need to convince the reviewers ;) I would assume that this won't require a lot of testing though.
Provide a reason why you want to cherry-pick (or rather why waiting for 7.1 would be a problem).

PierreZ · 2021-08-11T19:40:34Z

I think this change is ok to back port to 7.0. But the following will be needed:

1. Link to this PR in the description of the cherry-pick PR

2. Describe the testing that you've done. I think for a cherry-pick you would need to test this on macOS and on Linux. Something like "verified manually X, Y, and Z" would be sufficient -- there's no hard rules, you just need to convince the reviewers ;) I would assume that this won't require a lot of testing though.

3. Provide a reason why you want to cherry-pick (or rather why waiting for 7.1 would be a problem).

Thanks for all the information. I cherry-picked my commit against release-7.0 for testing. For now, I cannot verify my commit:

using my source-based distribution, with release-7.0, I'm having an issue where:
- boost is not detected anymore (was detected on tag 6.3.X),
- then boost needs to be downloaded and compiled, triggering a compile error,
using the official Docker image foundationdb/build:centos7-latest, I cannot build any branch, even master, because of an C compiler does not support c11 atomics. I created a dummy Github action that can reproduce the issue.

I will dig in both of them.

sfc-gh-mpilman · 2021-08-11T19:50:23Z

The official docker image is based on CentOS 7 which comes with a very old version of gcc. You either have to use devtoolset or clang (clang is probably better, as compilation times are much lower). A relatively recent version of clang is installed in the official docker image

PierreZ · 2021-08-11T20:40:09Z

The official docker image is based on CentOS 7 which comes with a very old version of gcc. You either have to use devtoolset or clang (clang is probably better, as compilation times are much lower). A relatively recent version of clang is installed in the official docker image

Thanks, GH action is working properly thanks to your devtoolset advice 🚀 I will try clang later, I will dig into my boost error:

*** argument error
* rule path.root ( path root )
* called with: (  )
* missing argument root

Thanks again for all your help @sfc-gh-mpilman !

jzhou77 requested review from sfc-gh-anoyes and sfc-gh-mpilman August 2, 2021 20:09

jzhou77 assigned sfc-gh-mpilman Aug 2, 2021

sfc-gh-mpilman requested changes Aug 3, 2021

View reviewed changes

PierreZ force-pushed the cmake/options branch 2 times, most recently from bd75251 to 3692d99 Compare August 5, 2021 13:54

PierreZ force-pushed the cmake/options branch from 3692d99 to e52fc41 Compare August 5, 2021 15:09

sfc-gh-mpilman approved these changes Aug 5, 2021

View reviewed changes

sfc-gh-mpilman requested changes Aug 5, 2021

View reviewed changes

add CMake's option for bindings

415bf2a

PierreZ force-pushed the cmake/options branch from e52fc41 to 415bf2a Compare August 5, 2021 19:58

sfc-gh-mpilman approved these changes Aug 6, 2021

View reviewed changes

sfc-gh-mpilman merged commit eb90854 into apple:master Aug 6, 2021

PierreZ mentioned this pull request Aug 16, 2021

[cherry-pick] add CMake's option for bindings #5389

Merged

5 tasks

		option(BUILD_DOCUMENTATION "build documentation" ON)
		if (BUILD_DOCUMENTATION AND WITH_PYTHON)

add CMake's option for bindings #5318

add CMake's option for bindings #5318

Uh oh!

Conversation

PierreZ commented Aug 2, 2021

Code-Reviewer Section

For Release-Branches

Uh oh!

foundationdb-ci commented Aug 2, 2021

AWS CodeBuild CI Report

Uh oh!

sfc-gh-mpilman left a comment

Choose a reason for hiding this comment

Uh oh!

sfc-gh-mpilman Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

PierreZ Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

sfc-gh-mpilman Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

PierreZ Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

PierreZ commented Aug 5, 2021

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

sfc-gh-mpilman left a comment

Choose a reason for hiding this comment

Uh oh!

sfc-gh-mpilman Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

PierreZ commented Aug 5, 2021

Uh oh!

foundationdb-ci commented Aug 5, 2021

AWS CodeBuild CI Report

Uh oh!

sfc-gh-mpilman commented Aug 6, 2021

Uh oh!

PierreZ commented Aug 9, 2021

Uh oh!

sfc-gh-mpilman commented Aug 9, 2021

Uh oh!

PierreZ commented Aug 11, 2021

Uh oh!

sfc-gh-mpilman commented Aug 11, 2021

Uh oh!

PierreZ commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants