-
Notifications
You must be signed in to change notification settings - Fork 566
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
Adjust gbench include_dir for CI permissions #4050
Conversation
@dantegd I think this should do it. |
Can somebody bring me up to speed on why does CUML need to install gbench at all? |
@robertmaynard I don't think it needs to be installed as can be seen in the PR I opened before #4049, we thought that gtest was also being installed so this was a more consistent approach, but now that I see the logs I think my memory betrayed me and we're not installing gtest so perhaps not installing gbench would be the way to go |
I vote for not installing it, since it 'should be' an implementation detail of the benchmarks which CUML doesn't install |
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #4050 +/- ##
===============================================
Coverage ? 85.59%
===============================================
Files ? 230
Lines ? 18221
Branches ? 0
===============================================
Hits ? 15596
Misses ? 2625
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
If we do decide to go this route, we need to merge upstream changes back in. Not doing it yet to avoid an unnecessary CI run until we decide for sure whether or not we're using this PR. |
Explicitly install gbench headers in
include/benchmark
, since dockerized CI runs do not have permission to accessinclude
directly