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

Adjust gbench include_dir for CI permissions #4050

Closed

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Jul 13, 2021

Explicitly install gbench headers in include/benchmark, since dockerized CI runs do not have permission to access include directly

@wphicks wphicks requested a review from a team as a code owner July 13, 2021 14:10
@wphicks wphicks added bug Something isn't working non-breaking Non-breaking change and removed CMake CUDA/C++ labels Jul 13, 2021
@wphicks
Copy link
Contributor Author

wphicks commented Jul 13, 2021

@dantegd I think this should do it.

@robertmaynard
Copy link
Contributor

Can somebody bring me up to speed on why does CUML need to install gbench at all?

@dantegd
Copy link
Member

dantegd commented Jul 13, 2021

@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

@robertmaynard
Copy link
Contributor

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

@wphicks
Copy link
Contributor Author

wphicks commented Jul 13, 2021

I don't have strong feelings on this either way, but if we're not installing gtest, there's certainly no need to install gbench. @dantegd any objection to me closing this out in favor of #4049?

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@f654048). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #4050   +/-   ##
===============================================
  Coverage                ?   85.59%           
===============================================
  Files                   ?      230           
  Lines                   ?    18221           
  Branches                ?        0           
===============================================
  Hits                    ?    15596           
  Misses                  ?     2625           
  Partials                ?        0           
Flag Coverage Δ
dask 48.14% <0.00%> (?)
non-dask 77.92% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f654048...58ef833. Read the comment docs.

@wphicks
Copy link
Contributor Author

wphicks commented Jul 13, 2021

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.

@wphicks wphicks closed this Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants