Skip to content
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.

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

Support build with Bazel #286

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support build with Bazel #286

wants to merge 4 commits into from

Conversation

lizhanhui
Copy link

Support build and run tests through Bazel so developers from Bazel community can reuse scripts, as is discussed in Issue 284

@@ -0,0 +1,32 @@
-----BEGIN CERTIFICATE-----

Choose a reason for hiding this comment

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

Unnecessary file.

Copy link
Contributor

Choose a reason for hiding this comment

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

We (eBay security dept) will not allow to include any certificate file in the public repo, even though it is just a dummy. Is it possible to add a script like this?

NuRaft/CMakeLists.txt

Lines 199 to 213 in cce3af7

add_custom_command(
TARGET build_ssl_key
PRE_BUILD
COMMAND
openssl req
-new
-newkey rsa:4096
-days 365
-nodes
-x509
-subj "/C=AB/ST=CD/L=EFG/O=ORG/CN=localhost"
-keyout ${CMAKE_CURRENT_BINARY_DIR}/key.pem
-out ${CMAKE_CURRENT_BINARY_DIR}/cert.pem
2> /dev/null
)

README.md Outdated
Build with Bazel
----------
#### 1. Install Bazel
Follow [install instructions](https://docs.bazel.build/versions/5.0.0/install.html)

Choose a reason for hiding this comment

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

Because bazel is not backward compatible, we should tell the versions we support here.

Copy link
Author

Choose a reason for hiding this comment

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

OK.

@@ -0,0 +1,13 @@
load("@rules_cc//cc:defs.bzl", "cc_binary")

Choose a reason for hiding this comment

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

Is it possible to not add so many BUILD.bazel files?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's possible to combine some of them. But practice of bazel recommends "Every directory that contains buildable files should be a package.", https://docs.bazel.build/versions/main/best-practices.html#packages

Some empty BUILD file works as a flag(say {source-repository}/bazel/BUILD), it's required to keep them to make the build system happy.

Choose a reason for hiding this comment

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

I see.

Copy link

@jovany-wang jovany-wang left a comment

Choose a reason for hiding this comment

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

This PR almost look good to me. But one concern is: do we have a better way to unify the cmake and bazel? otherwise they are hard to maintain.

push:
branches:
- master
- bazel

Choose a reason for hiding this comment

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

Remove bazel branch.

pull_request:
branches:
- master
- bazel

Choose a reason for hiding this comment

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

Remove


- name: Setup Bazel
run: |
sudo apt-get -qq install npm

Choose a reason for hiding this comment

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

It's better to pin the bazel version.

@greensky00
Copy link
Contributor

As I mentioned above, please remove cert/key files and make them auto-generated.

@lizhanhui
Copy link
Author

@greensky00 Got a few busy days and I would fix issues raised in the next commit.

@lizhanhui
Copy link
Author

"This PR almost look good to me. But one concern is: do we have a better way to unify the cmake and bazel? otherwise they are hard to maintain."
It is true that Bazel may use rules_foreign_cc to compile directly. But doing so would undermine dependency management, rendering automatic dependency version resolution infeasible.

As a matter of fact, glob is used while defining nuraft artifact. It would be a rare case when there is a need to modify bazel script in the future.

@@ -0,0 +1,8 @@
load("@rules_cc//cc:defs.bzl", "cc_library")

Choose a reason for hiding this comment

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

I'd like to move this file to bazel/thirdparty/asio.BUILD

@jovany-wang
Copy link

Is there any update?

@jovany-wang
Copy link

@greensky00 If there is no more update, how about merging this PR and then we're refining it in the next PR?

I'm looking forward this feature to integrate it to our repo in bazel building toolchain.

@greensky00
Copy link
Contributor

Hi @jovany-wang
I'm ok with partial merging, but before that, the cert files must be removed as it is monitored by the security team.
Since those dummy cert files are used for SSL testing only, this PR may need to exclude building unit tests, but still compiling the library itself should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants