Skip to content

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

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

Jump to bottom
Merged
sfc-gh-anoyes merged 1 commit into from
Aug 17, 2021
Merged

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

sfc-gh-anoyes merged 1 commit into from
Aug 17, 2021
+87 −44

Conversation

@PierreZ
Copy link
Contributor

@PierreZ PierreZ commented Aug 16, 2021

Cherry-pick of #5318, relating to the issue #5218.

This commit is adding more CMake options to disable bindings if needed. This will greatly help packaging the upcoming 7.0 release for source-based distributions as it allow to:

  • reduce compile time,
  • have less dependencies (we can avoid having Go, Java, Ruby as required buil.d dependencies),
  • avoid network call like go get during build time which cannot be done in a sandbox.

Options are by default ON to avoid messing with existing CIs.

Verification

Verified locally with Docker:

❯ docker run --rm -it foundationdb/build:centos7-latest

$ git clone https://github.com/PierreZ/foundationdb.git
$ cd foundationdb && git checkout cherrypick-415bf2a && cd ..
$ mkdir build && cd build
$ source /opt/rh/devtoolset-8/enable
$ source /opt/rh/rh-python36/enable
$ cmake -G Ninja -DBUILD_JAVA_BINDING=OFF -DBUILD_RUBY_BINDING=OFF -DBUILD_GO_BINDING=off ../foundationdb
# Will output:
# ...
# -- =========================================
# --    Components Build Overview 
# -- =========================================
# -- Build Bindings (depends on Python):   ON
# -- Build C Bindings:                     ON
# -- Build Python Bindings:                ON
# -- Build Java Bindings:                  OFF
# -- Build Go bindings:                    OFF
# -- Build Ruby bindings:                  OFF
# -- Build with TLS support:               ON
# -- Build Documentation (make html):      ON
# -- Build Python sdist (make package):    ON
# -- Configure CTest (depends on Python):  ON
# -- Build with RocksDB:                   ON
# -- =========================================
# ...

$ ninja -j10
$ cpack -G DEB

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
Copy link
Contributor

foundationdb-ci commented Aug 16, 2021

AWS CodeBuild CI Report

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

@foundationdb-ci
Copy link
Contributor

foundationdb-ci commented Aug 16, 2021

AWS CodeBuild CI Report

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

@jzhou77 jzhou77 requested review from sfc-gh-anoyes and xis19 August 16, 2021 20:25
xis19
xis19 requested changes Aug 16, 2021
View reviewed changes
cmake/FDBComponents.cmake
if(WITH_PYTHON)
set(WITH_PYTHON_BINDING ON)
else()
#message(FATAL_ERROR "Could not found a suitable python interpreter")
Copy link
Collaborator

@xis19 xis19 Aug 16, 2021

Choose a reason for hiding this comment

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

This comment can be removed?

Copy link
Collaborator

@sfc-gh-anoyes sfc-gh-anoyes Aug 16, 2021

Choose a reason for hiding this comment

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

Since this is a cherry-pick, I think we should address review comments on master first to make sure things don't get de-synced

cmake/FDBComponents.cmake

option(BUILD_JAVA_BINDING "build java binding" ON)
if(NOT BUILD_JAVA_BINDING OR NOT WITH_C_BINDING)
set(WITH_JAVA_BINDING OFF)
Copy link
Collaborator

@xis19 xis19 Aug 16, 2021

Choose a reason for hiding this comment

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

Maybe add a line

message(WARNING "Java depends on C binding, but C binding is not enabled")

or even promote WARNING to FATAL_ERROR since this might cause unexpected java binding missing?

cmake/FDBComponents.cmake
if(GO_EXECUTABLE AND NOT WIN32)
set(WITH_GO ON)
option(BUILD_GO_BINDING "build go binding" ON)
if(NOT BUILD_GO_BINDING OR NOT BUILD_C_BINDING)
Copy link
Collaborator

@xis19 xis19 Aug 16, 2021

Choose a reason for hiding this comment

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

Similar above.

cmake/FDBComponents.cmake
set(GEM_COMMAND ${RUBY_EXECUTABLE} ${GEM_EXECUTABLE})
set(WITH_RUBY ON)
option(BUILD_RUBY_BINDING "build ruby binding" ON)
if(NOT BUILD_RUBY_BINDING OR NOT BUILD_C_BINDING)
Copy link
Collaborator

@xis19 xis19 Aug 16, 2021

Choose a reason for hiding this comment

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

Similar above

@xis19 xis19 self-requested a review August 16, 2021 22:48
xis19
xis19 approved these changes Aug 16, 2021
View reviewed changes
Copy link
Collaborator

@xis19 xis19 left a comment

Choose a reason for hiding this comment

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

There could be improvements but since this is a cherry-pick, the changes can be addressed in another commit.

@sfc-gh-anoyes sfc-gh-anoyes merged commit 91b7e8a into apple:release-7.0 Aug 17, 2021
@PierreZ
Copy link
Contributor Author

PierreZ commented Aug 17, 2021

There could be improvements but since this is a cherry-pick, the changes can be addressed in another commit.

Thanks for the merge, I will add the improvements in another commit 👍

This was referenced Aug 19, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfc-gh-anoyes sfc-gh-anoyes sfc-gh-anoyes approved these changes

@xis19 xis19 xis19 approved these changes

Assignees

@sfc-gh-anoyes sfc-gh-anoyes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PierreZ @foundationdb-ci @xis19 @sfc-gh-anoyes