-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 benchmark and all tests with Bazel without modifying C++ source code #501
Conversation
see #496 how is this different? |
✅ Build benchmark 909 completed (commit 900c2f7d30 by @qzmfranklin) |
Hi @dominichamon , Thanks for the reply! I saw the one you listed before I created this PR.
Given the above, I believed there was still value in creating this PR. How do you think? Thanks, |
BUILD
Outdated
], | ||
copts = benchmark_copts, | ||
deps = [ | ||
':output_test_helper', |
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.
these should also depend on :benchmark, 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.
Yes. But we do not need to specify that dependency here because output_test_helper
already depends on :benchmark
.
BUILD
Outdated
copts = benchmark_copts, | ||
deps = [ | ||
':benchmark', | ||
gtest_dep, |
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 don't think any of these need gtest
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.
Thanks! Will remove with the next update.
BUILD
Outdated
], | ||
deps = [ | ||
':benchmark', | ||
gtest_dep, |
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.
doesn't use gtest, afaik.
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.
Thanks! Removed. Will update with the next update.
WORKSPACE
Outdated
@@ -0,0 +1,69 @@ | |||
new_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.
i don't know enough bazel: is this defining the googletest repo dependency for use in the BUILD? or is it supposed to define the benchmark package and this is a bad copy-paste?
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 former.
Here is an explanation of how the new_git_repository
rule registers a git repository as a dependency:
https://docs.bazel.build/versions/master/be/workspace.html#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.
But I believe you might have had question regarding the comment of copying the content of the BUILD file. The in-source comment is, to the best of my knowledge, stating the facts and rationale. Hopefully with a look at the git_repository
rule that I pasted above, it clears out for you and potentially other reviewers.
can you add a travis bazel build? |
Will update with a Bazel build test for Travis. Though, at this very moment, |
OK, before that, the Travis bot got a case-sensitivity issue. This run failed to create the
This is because the filesystem it uses is case-insensitive and this PR is adding a file named I have found a few things on Travis: Looks like they just do not have case-sensitivity be default on some bots. I am going to rename |
The rationale for making this change is docmented here: google#501 Briefly speaking, some test bots on TravisCI do not have case sensitive filesystems. So the build will fail to create the `build directory`. It does not look like Travis is going to resolve this problem on their side anytime soon, yet. So I am making this change to pass the test.
1feb9ab
to
731423f
Compare
✅ Build benchmark 910 completed (commit 538a966059 by @qzmfranklin) |
By using a different fork, for now, with the understanding that we should switch to the google official googletest repo as soon as the issue below is resolved: google/googletest#1212
✅ Build benchmark 916 completed (commit c706dc2345 by @qzmfranklin) |
Urrhh, not successful with adding the bazel test yet. Somehow it fails on TravisCI bot. I need to go to sleep soon. Will continue tomorrow. Thanks for all your advice so far. Really appreciate them. |
✅ Build benchmark 921 completed (commit e602f99d9b by @qzmfranklin) |
you can also change the build directory to be _build if that helps. |
Have not got time to work on this one tonight. Will try tomorrow. |
Just checking in.. did you have a chance to look again? |
Made obsolete by #533 |
This PR attempts to build all libraries and tests of benchmark with Bazel without modifying the source code.
Tested on a Ubuntu 16.04 x86_64 machine with GCC 4.7.0 and clang 5.0.0:
Also, this BUILD file is written to allow this repository to be included in other Bazel based projects in source format.
Of the 17 tests, 16 are passing as of this writing.
The failed one,
user_counters_tabular_test
has the following error log: