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

Add support for cloning Google benchmark #293

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 14, 2022

Description

Many RAPIDS libraries use Google benchmark to manage C++ benchmarks. This PR centralizes the dependency and the version management.

Resolves #288.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@vyasr vyasr added feature request New feature or request non-breaking Introduces a non-breaking change 3 - Ready for Review Ready for review by team labels Oct 14, 2022
@vyasr vyasr requested a review from a team as a code owner October 14, 2022 18:11
@vyasr vyasr self-assigned this Oct 14, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Oct 14, 2022

I should add that cuML and cuGraph's current versions of this function specify the lib dir for gbench, but I don't think we need or want that. Please correct me if I'm wrong.

Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Looks great. Just a couple of small changes needed

rapids-cmake/cpm/gbench.cmake Outdated Show resolved Hide resolved
rapids-cmake/cpm/gbench.cmake Show resolved Hide resolved
@robertmaynard
Copy link
Contributor

I should add that cuML and cuGraph's current versions of this function specify the lib dir for gbench, but I don't think we need or want that. Please correct me if I'm wrong.

We will want to specify the install directory if INSTALL_EXPORT_SET is set. But we will want to use rapids_cmake_install_lib_dir to compute the correct location. I expect this was done so it doesn't go into something like lib64 and therefore breaks conda layout requirements.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

No comments aside from what @robertmaynard mentioned. Nice, thanks for doing this!

@vyasr
Copy link
Contributor Author

vyasr commented Oct 14, 2022

I should add that cuML and cuGraph's current versions of this function specify the lib dir for gbench, but I don't think we need or want that. Please correct me if I'm wrong.

We will want to specify the install directory if INSTALL_EXPORT_SET is set. But we will want to use rapids_cmake_install_lib_dir to compute the correct location. I expect this was done so it doesn't go into something like lib64 and therefore breaks conda layout requirements.

I'm now setting the CMAKE_INSTALL_LIBDIR for this target unconditionally (using rapids_cmake_install_lib_dir). I'm assuming that the BENCHMARK_ENABLE_INSTALL option for GBench will take care of whether installation happens at all so it should be safe to always set this.

@vyasr vyasr requested a review from robertmaynard October 14, 2022 22:05
@vyasr
Copy link
Contributor Author

vyasr commented Oct 21, 2022

@robertmaynard are you happy with the current state of this PR?

Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@vyasr
Copy link
Contributor Author

vyasr commented Oct 21, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 49b5f61 into rapidsai:branch-22.12 Oct 21, 2022
@vyasr vyasr deleted the feat/gbench branch October 21, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] rapids-cmake should support gbench
3 participants