bazel,travis: add bazel build checking #3925
Conversation
.travis.yml
Outdated
|
|
||
| script: | ||
| - ./gradlew check :grpc-all:jacocoTestReport | ||
| - bazel build //compiler:all //context:all //core:all //netty:all //okhttp:all //protobuf:all //protobuf-lite:all //protobuf-nano:all //stub:all //testing:all |
There was a problem hiding this comment.
This can be //... right?
There was a problem hiding this comment.
Slightly different and ... will unintentionally include //examples:all which is a non-target.
There was a problem hiding this comment.
If examples is the only reason for the expansion, then we should subtract it out:
bazel build -- ... -examples/...
Also /... should be preferred to :all here, since there's no need to disable descending into subpackages (although they don't exist today).
Honestly, I would be okay with including the examples in this line. I know that users will see something slightly different since the WORKSPACE is handled differently, but building examples using its own WORKSPACE will rebuild everything a third time.
There was a problem hiding this comment.
I will change building each services to ....
ejona86
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
|
I think it may be better to use a matrix build (or similar) here. The bazel build takes 6 minutes, which is a third of the total time. A matrix build could run it in parallel. |
|
Thank you for your comments! Caching bazel artifacts is also probably needed. |
jyane
force-pushed
the
add-bazel-checking-to-bazel
branch
from
49304b0 to
1d56ee4
Compare
|
I have added matrix testing via my forked repository (jyane#1), but it is so dirty... |
jyane
force-pushed
the
add-bazel-checking-to-bazel
branch
from
52d5e74 to
be89838
Compare
jyane
force-pushed
the
add-bazel-checking-to-bazel
branch
from
be89838 to
9292ccb
Compare
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
I'm thinking it may be better to just do this as a kokoro job and leave travis as-is. I think with Kokoro we could probably do this in < 5 lines or some such. Let me see if I can cook this up quickly.
If we use travis with it, then I'd want the majority of commands for each stage to be in the same file. That could be aided by putting each stage's commands in a function. We could then also have a separate file for bazel vs gradle's functions.
| @@ -1,43 +1,40 @@ | |||
| sudo: false | |||
| sudo: required | |||
There was a problem hiding this comment.
Wait... how did it work before with sudo: false? I didn't look closely enough earlier to notice it was running dpkg. The need for sudo is why I didn't mess with this earlier; I'd rather not lose that. It looks like it is possible to install into the home directory though.
| addons: | ||
| apt: | ||
| packages: | ||
| - wget |
There was a problem hiding this comment.
Is this actually necessary? We've already been using wget in make_dependencies.sh and the Travis documentation says wget is already installed.
There was a problem hiding this comment.
Sorry, it is an oversight...
|
Thank you for your comments and reviews.
When I have seen this issue (#3502), I thought that want to add bazel build checking to kokoro. I thought writing the CI codes in an opening way is helpful for contributors because contributors could know how to build and test. |
|
@jyane, I wasn't trying to criticize your decisions in the PR. Yes, my initial issue said Kokoro, but that wasn't a strong requirement. It actually pains me quite a bit how "blind" Kokoro is to external users. And, yes, lack of CI for Bazel has been creating problems and so a PR to add Travis testing for it is quite reasonable. My preference is actually to do it in Travis as well, but I had anticipated some of the problems discussed here and thought Kokoro would be simpler. My main problem with Travis here is that it is a pain to have a completely different build[1]. Having 6 highly-coupled files would be unfortunate. I think there can be some acceptable organizations of the commands, but it seems like Kokoro is simple enough here to warrant accepting its limitations.
|
|
Thank you for your comment, and I understand. Please don't care about me. You can always close my PR. |
5 participants
add bazel build checking to
.travis.yml.This PR's CI would be failed because protobuf's sha256 is incorrect.
When it (#3924) be merged, I will rebase this branch to pass CI.
This PR is partial fix for this issue: #3502