-
Notifications
You must be signed in to change notification settings - Fork 240
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
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,32 @@ | |||
-----BEGIN CERTIFICATE----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary file.
There was a problem hiding this comment.
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?
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
examples/calculator/BUILD.bazel
Outdated
@@ -0,0 +1,13 @@ | |||
load("@rules_cc//cc:defs.bzl", "cc_binary") | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
As I mentioned above, please remove cert/key files and make them auto-generated. |
@greensky00 Got a few busy days and I would fix issues raised in the next commit. |
"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." 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") |
There was a problem hiding this comment.
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
Is there any update? |
@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. |
Hi @jovany-wang |
Support build and run tests through Bazel so developers from Bazel community can reuse scripts, as is discussed in Issue 284