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

Build Raylet with Bazel #3806

Merged
merged 30 commits into from
Jan 20, 2019
Merged

Conversation

pcmoritz
Copy link
Contributor

@pcmoritz pcmoritz commented Jan 19, 2019

This allows to build the raylet with

bazel build -c opt ...

in the root directory.

This is based off #2899. Related to #2887.

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

@pcmoritz pcmoritz Jan 19, 2019

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.

@ericl
Copy link
Contributor

ericl commented Jan 19, 2019 via email

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10948/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10949/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10952/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10950/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10951/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10953/
Test PASSed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10963/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10964/
Test PASSed.

bazel/plasma.bzl Outdated
@@ -0,0 +1,80 @@
load("@com_github_google_flatbuffers//:build_defs.bzl", "flatbuffer_cc_library")
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely!

@robertnishihara
Copy link
Collaborator

@rsepassi could you take a quick look and see if things look reasonable?

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10981/
Test PASSed.

@pcmoritz pcmoritz changed the title [WIP] Build Raylet with Bazel Build Raylet with Bazel Jan 20, 2019
@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10982/
Test PASSed.

@@ -31,6 +31,10 @@ matrix:
script:
- ./java/test.sh

# Test Bazel build
- ./.travis/install-bazel.sh
- bazel build ...
Copy link
Collaborator

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?

Copy link
Contributor Author

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.


OS=linux
ARCH=x86_64
if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then OS=darwin; fi
Copy link
Collaborator

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

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

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

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"]),
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Contributor Author

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(
Copy link
Collaborator

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

Copy link
Contributor Author

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",
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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(
Copy link
Collaborator

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?

Copy link
Contributor Author

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")

Copy link
Collaborator

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?

Copy link
Contributor Author

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",
Copy link
Collaborator

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?

Copy link
Contributor Author

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"])
Copy link
Collaborator

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?

Copy link
Contributor Author

@pcmoritz pcmoritz Jan 20, 2019

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)

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10983/
Test PASSed.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10984/
Test FAILed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10986/
Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/10987/
Test PASSed.

@robertnishihara robertnishihara merged commit 0dad4e6 into ray-project:master Jan 20, 2019
@robertnishihara robertnishihara deleted the bazelize-raylet branch January 20, 2019 20:16
@rsepassi
Copy link
Contributor

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?

@robertnishihara
Copy link
Collaborator

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.

@rsepassi
Copy link
Contributor

rsepassi commented Jan 22, 2019 via email

@pcmoritz
Copy link
Contributor Author

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.

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.

5 participants