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.

Sign up for GitHub

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

Jump to bottom

Made cudftestutil header-only and removed GTest dependency #16839

Merged
merged 21 commits into from
Oct 14, 2024
Merged

Made cudftestutil header-only and removed GTest dependency #16839

merged 21 commits into from
Oct 14, 2024

Conversation

lamarrr
Copy link
Contributor

@lamarrr lamarrr commented Sep 19, 2024 •
edited
Loading

Description

This merge request follows up on #16658.
It removes the dependency on GTest by cudftestutil. It satisfies the requirement that we only need API compatibility with the GTest API and we don't expose the GTest symbols to our consumers nor ship any binary artifact.
The source files defining the symbols are late-binded to the resulting executable (via library INTERFACE sources).
The user has to link to manually link the GTest and GMock libraries to the final executable as illustrated below.

Closes #16658

Usage

CMakeLists.txt:

add_executable(test1 test1.cpp)
target_link_libraries(test1 PRIVATE GTest::gtest GTest::gmock GTest::gtest_main cudf::cudftestutil cudf::cudftestutil_impl)

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@lamarrr lamarrr added feature request New feature or request improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 19, 2024
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 19, 2024
@lamarrr lamarrr marked this pull request as ready for review September 19, 2024 10:58
@github-actions github-actions bot added the CMake CMake build issue label Sep 19, 2024
@lamarrr lamarrr requested review from a team as code owners September 19, 2024 10:58
@lamarrr lamarrr added feature request New feature or request and removed improvement Improvement / enhancement to an existing function feature request New feature or request labels Sep 19, 2024
@lamarrr lamarrr force-pushed the cudftestutil-header branch 7 times, most recently from 94dda22 to e8c1059 Compare September 20, 2024 15:26
@GregoryKimball
Copy link
Contributor

@vyasr would you please take a look? Does this look like the right approach?

davidwendt
davidwendt requested changes Sep 25, 2024
View reviewed changes
cpp/tests/common/cudf_test_impl.cu Outdated Show resolved Hide resolved
cpp/tests/common/cudf_test_impl.cpp Outdated Show resolved Hide resolved
@vyasr vyasr added breaking Breaking change and removed non-breaking Non-breaking change labels Sep 27, 2024
vyasr
vyasr approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Nice work! This looks good. I changed the labels to mark this PR as breaking since it will affect all external consumers of cudftestutil.

Can you rebase this PR on branch-24.12? I'm approving assuming that you do that, and pending resolution of David's comments.

cpp/tests/common/cudf_test_impl.cpp Outdated Show resolved Hide resolved
cpp/tests/common/cudf_test_impl.cu Outdated Show resolved Hide resolved
@vyasr vyasr changed the base branch from branch-24.10 to branch-24.12 September 27, 2024 16:24
@lamarrr lamarrr force-pushed the cudftestutil-header branch from 307c34c to 08e2e67 Compare September 28, 2024 00:18
vyasr
vyasr approved these changes Oct 7, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

One question about whether the targets are perhaps a bit redundant, at least at the consumer's end (it might still be cleaner to retain the separation on our end). I'm happy with the result either way, so I'm approving now.

@lamarrr lamarrr requested a review from davidwendt October 10, 2024 07:50
robertmaynard
robertmaynard reviewed Oct 11, 2024
View reviewed changes
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@robertmaynard
Copy link
Contributor

One question about whether the targets are perhaps a bit redundant, at least at the consumer's end (it might still be cleaner to retain the separation on our end). I'm happy with the result either way, so I'm approving now.

The separate targets allow consumers the ability to optin to a subset of functionality without any code changes. So they can also write the logic to compile the subset of functions they want

@lamarrr @robertmaynard
updated cudftestutil_impl doc
3c07115 
Co-authored-by: Robert Maynard <robertjmaynard@gmail.com>
@davidwendt
Copy link
Contributor

I know this may sound silly but would you mind changing a few of the gtests to fail and make sure they fail like they are supposed to? The last time we attempted this kind of change we got some strange results from our gtests runs. Also include one of the large_strings tests as well.

@lamarrr
Copy link
Contributor Author

lamarrr commented Oct 11, 2024

I know this may sound silly but would you mind changing a few of the gtests to fail and make sure they fail like they are supposed to? The last time we attempted this kind of change we got some strange results from our gtests runs. Also include one of the large_strings tests as well.

I've done that and can confirm they fail as they are supposed to

@lamarrr
Copy link
Contributor Author

lamarrr commented Oct 14, 2024

/merge

@rapids-bot rapids-bot bot merged commit 3bee678 into rapidsai:branch-24.12 Oct 14, 2024
102 checks passed
@lamarrr lamarrr deleted the cudftestutil-header branch October 14, 2024 17:15
@lamarrr lamarrr mentioned this pull request Oct 14, 2024
Merged
3 tasks
@pxLi pxLi mentioned this pull request Oct 15, 2024
Closed
@jlowe jlowe mentioned this pull request Oct 15, 2024
Merged
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Oct 16, 2024
@lamarrr
Updated libcudftestutil CMake linking logic for 24.12 (#1475)
8314c13 
This merge request fixes the CMake linking logic to cudftestutil for branch-24.12, allowing you to specify which version of GTest/GBench to use as long as the API is source compatible.

Follows up on: rapidsai/cudf#16839

Authors:
  - Basit Ayantunde (https://github.com/lamarrr)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Bradley Dice (https://github.com/bdice)

URL: #1475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr approved these changes

@davidwendt davidwendt davidwendt approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

@msarahan msarahan msarahan approved these changes

@pmattione-nvidia pmattione-nvidia Awaiting requested review from pmattione-nvidia pmattione-nvidia is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@bdice bdice Awaiting requested review from bdice bdice was automatically assigned from rapidsai/cudf-python-codeowners

@charlesbluca charlesbluca Awaiting requested review from charlesbluca charlesbluca was automatically assigned from rapidsai/cudf-python-codeowners

Assignees
No one assigned
Labels
breaking Breaking change CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code.
Projects
libcudf
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Make cudftestutil a header-only package
6 participants
@lamarrr @GregoryKimball @robertmaynard @davidwendt @msarahan @vyasr