-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Chore: rework Bazel build system #1657
Chore: rework Bazel build system #1657
Conversation
93cc32b
to
392ecc0
Compare
Nice job! Only one question: Will the changes break the compilation of users with old versions of bazel, e.g. 0.25 |
392ecc0
to
3dfadda
Compare
@chenzhangyi In short it would break building with bazel 0.25. Many dependencies relying on a higher version of bazel. I didn't verify each of them. However, |
The protobuf arena changes are still breaking. I'll fix it with |
3dfadda
to
89dc1fe
Compare
@wwbmmm PTAL |
LGTM |
Is there something else to make it merged? |
Hi @hcoona , Thank you so much for your contribution! But I have a little concern, some projects may depend on bazel 0.2x, like TensorFlow 1.x, and we have seen some teams using bRPC in TensorFlow. So, maybe we should consider the compatibility issues. cc @chenzhangyi @wwbmmm |
@hcoona Can you extract the changes related to “Support Protobuf 3.19.1” into a separate PR so that this part can be merged faster? |
@lorinlee @chenzhangyi In bazel philosophy, the final project owner need to take responsibility for all of its dependencies, AKA. specify how to download & build them in |
89dc1fe
to
3f74c9f
Compare
Do my best to make it compiles with bazel 0.28.0 (0.27.1 failed). See It means that if a user used bRPC in their project, it can compile with bazel 0.28.0 and higher. |
@chenzhangyi @lorinlee ping~ |
Would you help merge it? |
@chenzhangyi Would you help to take a look at it? The user project depending on bRPC would still compile if they are using bazel v0.28.0 and higher. |
Will it be merged soon? |
@hcoona please reslove conflicts, thanks. |
d235fa5
to
bb0b1e4
Compare
@guodongxiaren PTAL |
@@ -0,0 +1,36 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one or more |
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.
What is this file used for? Can we delete it?
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 solved a noisy compiler warning. I suggest applying it. The final user could use their provided glog. It's harmless.
@@ -0,0 +1,16 @@ | |||
# Licensed to the Apache Software Foundation (ASF) under one or more |
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.
Do we need to delete all the files under build_with_old_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.
It's to prove we can build brpc with old version of bazel. It's concerned by many people above in the thread.
It's under example folder. We can delete it if we want.
bb0b1e4
to
d95c50e
Compare
It's a great contribution, and could you please share a related blog or doc for that pr? I think it will be a good learning material and we will post it to our official website, please consider this, thanks a lot. |
@zyearn PTAL. I removed The CI failure cannot be reproduced on my dev environment with GCC 9.4.0 & bazel 4.2.2 by running |
@Huixxi I didn't realize there was any special work. Please look the Bazel official document & some existing repo driven by Bazel. Please raise to me if there was truly something special with details, I can help to find some document about them. |
I think the example of building with old version of bazel is useful. No need to remove it. |
CI is fixed in the latest version. Could you merge master and submit again? |
d95c50e
to
ad20082
Compare
@zyearn It works after rebase. |
LGTM. Thanks for the contribution. Btw, what do you think is the future work for improving Bazel in brpc? |
Looks like I am coming a few minutes too late after the merge! There is a slight change to be made to make it work with Mac M1:
|
@0x2Adr1 Could you make a PR separately? |
It's good enough now. I don't think we need to adjust it unless we plan to change the project structure or Bazel bring a big change. |
@hcoona MacOs上使用bazel似乎还有问题。烦请试试 |
@guodongxiaren I've no macOS environment. Please help to post the error message & I'll try to investigate it. And please notice that @0x2Adr1 raise some additional fixes for macOS, please also try it. |
It's better to update the getting_start document (cn/en) with a new "Compile brpc with bazel" part under each platform, but I'm not familiar with bazel... |
Totally rewrite the bazel build support.
Support latest version of bazel.
Build all external dependencies (except for mesalink) with bazel.
Support Protobuf 3.19.1 (breaking changes here: protocolbuffers/protobuf@624d29d)
Support generate
compile_commands.json
for bazel project.Rework the options flags & move all of them into
//bazel/config
Leave building with mesalink for future. I think maybe all dependencies should link against mesalink instead of openssl.