-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Build Raylet with Bazel #3806
Build Raylet with Bazel #3806
Conversation
BUILD
Outdated
"src/ray/thirdparty/hiredis/*.h", | ||
"src/ray/thirdparty/hiredis/adapters/*.h", | ||
"src/ray/thirdparty/hiredis/dict.c", | ||
"src/ray/thirdparty/ae/ae_kqueue.c" |
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.
@ericl What's the best way to do this in a platform independent way? On linux we need ae_epoll.c here.
Is it possible to move the logic into an ifdef? There's also
https://docs.bazel.build/versions/master/platforms.html#defining-constraints-and-platforms
…On Fri, Jan 18, 2019, 4:02 PM Philipp Moritz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In BUILD
<#3806 (comment)>:
> + ],
+ includes = ["src/ray/thirdparty"],
+)
+
+cc_library(
+ name = "hiredis",
+ srcs = glob([
+ "src/ray/thirdparty/ae/ae.c",
+ "src/ray/thirdparty/hiredis/*.c",
+ ]),
+ hdrs = glob([
+ "src/ray/thirdparty/ae/*.h",
+ "src/ray/thirdparty/hiredis/*.h",
+ "src/ray/thirdparty/hiredis/adapters/*.h",
+ "src/ray/thirdparty/hiredis/dict.c",
+ "src/ray/thirdparty/ae/ae_kqueue.c"
@ericl <https://github.com/ericl> What's the best way to do this in a
platform independent way? On linux we need qe_epoll.c here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3806 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA6SlbnazDNtixInSi2OY8Za3ruyNFCks5vEmChgaJpZM4aI1Oj>
.
|
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test FAILed. |
Test PASSed. |
bazel/plasma.bzl
Outdated
@@ -0,0 +1,80 @@ | |||
load("@com_github_google_flatbuffers//:build_defs.bzl", "flatbuffer_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.
Should this file be eventually moved to the plasma repo?
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.
absolutely!
@rsepassi could you take a quick look and see if things look reasonable? |
Test PASSed. |
Test PASSed. |
@@ -31,6 +31,10 @@ matrix: | |||
script: | |||
- ./java/test.sh | |||
|
|||
# Test Bazel build | |||
- ./.travis/install-bazel.sh | |||
- bazel build ... |
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 fine to put it here for now, but this is temporary, right?
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.
Yeah, eventually everything should be switched, for now it just makes sure it doesn't get broken. This is the build matrix entry that is the least loaded (aside from linting) and has java already installed, so it's the natural candidate.
.travis/install-bazel.sh
Outdated
|
||
OS=linux | ||
ARCH=x86_64 | ||
if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then OS=darwin; fi |
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.
Possibly not a huge deal, but might as well make this script run outside of Travis. The way we normally detect OS in Travis is
ray/.travis/install-dependencies.sh
Lines 7 to 18 in aad48ee
platform="unknown" | |
unamestr="$(uname)" | |
if [[ "$unamestr" == "Linux" ]]; then | |
echo "Platform is linux." | |
platform="linux" | |
elif [[ "$unamestr" == "Darwin" ]]; then | |
echo "Platform is macosx." | |
platform="macosx" | |
else | |
echo "Unrecognized platform." | |
exit 1 | |
fi |
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.
good idea yeah, I fixed it
@@ -0,0 +1,309 @@ | |||
# Bazel build |
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.
Any reason to prefer BUILD
or BUILD.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.
yes, BUILD wouldn't work because we already have a build directory and macOS is case-insensitive (I found out the hard way)
BUILD.bazel
Outdated
srcs = glob([ | ||
"src/ray/thirdparty/ae/ae.c", | ||
"src/ray/thirdparty/hiredis/*.c", | ||
], exclude = ["src/ray/thirdparty/hiredis/test.c"]), |
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.
exclude
should probably be on a new line
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.
or maybe not? not sure what the proper formatting is
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.
exclude is part of the glob command so it makes sense to me to have it on the same line
BUILD.bazel
Outdated
|
||
cc_library( | ||
name = "gcs", | ||
srcs = glob( |
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.
The formatting here is a bit different from before
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'll try to make it more consistent
git_repository( | ||
name = "com_github_nelhage_rules_boost", | ||
commit = "6d6fd834281cb8f8e758dd9ad76df86304bf1869", | ||
remote = "https://github.com/nelhage/rules_boost", |
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 this official or just some random repo on the internet?
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.
There is no official BAZEL support for boost, so we can be glad to have this random repo. It's the best I could find so far, short of writing build files for boost ourselves.
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
@@ -0,0 +1,29 @@ | |||
load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository", "new_git_repository") | |||
|
|||
git_repository( |
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 you need this git_repository
command in addition to the load
command below?
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.
load brings "git_repository" into the namespace (this is needed to be compatible with the latest version of bazel)
@@ -0,0 +1,82 @@ | |||
load("@com_github_google_flatbuffers//:build_defs.bzl", "flatbuffer_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.
Is the bazel/BUILD
file necessary?
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, so Bazel can find the plasma build file, which now needs to be a file registered with Bazel (this is needed since the new version of the new_git_command, but is backward compatible fortunately)
name = "plasma", | ||
visibility = ["//visibility:public"], | ||
hdrs = [ | ||
"cpp/src/plasma/client.h", |
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's the convention for when to list all the files versus to use *.h
?
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.
no convention, if all the .h are included, I put *.h, but if it's only a small subset I list them
includes = ["cpp/src/plasma/format/common.fbs"] | ||
) | ||
|
||
exports_files(["cpp/src/plasma/format/common.fbs"]) |
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 does this line do?
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 makes the file available from outside of the plasma env (i.e. for ray)
Test PASSed. |
Test FAILed. |
Test PASSed. |
Test PASSed. |
LGTM! Hope it makes builds easier. Was having the half-baked BUILD file I sent a while back helpful or was it easier to just start from scratch? |
Thanks @rsepassi the earlier PR was very helpful as a starting point. A lot has changed in Ray since then, but it definitely sped up the process. We're still in the process of switching entirely to Bazel, so may ask you to comment on a few more PRs in the future. |
Sounds good!
…On Tue, Jan 22, 2019 at 11:40 AM Robert Nishihara ***@***.***> wrote:
Thanks @rsepassi <https://github.com/rsepassi> the earlier PR was very
helpful as a starting point. A lot has changed in Ray since then, but it
definitely sped up the process. We're still in the process of switching
entirely to Bazel, so may ask you to comment on a few more PRs in the
future.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3806 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABEGW5o2_1k2ySxESa9tJFSynn3N5I_kks5vF2k7gaJpZM4aI1Oj>
.
|
Yes, it was definitely helpful to start with the build file you provided. The next step will be to integrate the python wrappers for the backend with Bazel. |
This allows to build the raylet with
in the root directory.
This is based off #2899. Related to #2887.