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

Remove public gtest dependency from libcudf conda package #15534

Merged

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Apr 15, 2024

Description

Reworks the cudftestutil and dependency chain to remove the public gtest dependency in libcudf conda package.
The libcudftestutil was previously made static due to issues using a static system GTest that wasn't build with fPIC. Using a GTest from rapids-cmake which is built with fPIC enabled, removes this restriction and allows us to remove the public depedency.

Some notes:

  • We need to align all of RAPIDS C++ projects on static GTest from rapids-cmake
  • None of the compiled components / classes of libcudftestutils publically depend on GTest
  • Two of the libcudftestutils header only components bring include gtest. Since these headers aren't required to be used we are going to consider them optional.
  • Therefore using these optional libcudftestutils header will require downstream users to bring in GTest.

Fixes #13381
Contributes to rapidsai/build-planning#32

Checklist

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

@robertmaynard robertmaynard added feature request New feature or request non-breaking Non-breaking change labels Apr 15, 2024
@robertmaynard robertmaynard requested review from a team as code owners April 15, 2024 17:54
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue conda labels Apr 15, 2024
@bdice
Copy link
Contributor

bdice commented Apr 16, 2024

I merged the upstream to get CI unblocked.

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.

While I think this change is already safe across the rest of RAPIDS (rapidsai/build-planning#32), I suspect we need to do a bit more work in cudf. Specifically, I think we have to address #13381. Currently libcudftestutil.a exports a public dependency on GTest. Granting that it is already a static library, that still means that consumers of it will see APIs that have use gtest. What happens if someone tries to link against libcudftestutil.a, but has compiled their code using a different version of gtest? Won't that result in incompatible binaries?

In practice the only use case I'm aware of is cuspatial, which uses rapids-cmake and we control, so it's probably not a serious issue, but unless I'm misunderstanding it is a real potential issue that is worth trying to resolve before merging this.

@robertmaynard
Copy link
Contributor Author

While I think this change is already safe across the rest of RAPIDS (rapidsai/build-planning#32), I suspect we need to do a bit more work in cudf. Specifically, I think we have to address #13381. Currently libcudftestutil.a exports a public dependency on GTest. Granting that it is already a static library, that still means that consumers of it will see APIs that have use gtest. What happens if someone tries to link against libcudftestutil.a, but has compiled their code using a different version of gtest? Won't that result in incompatible binaries?

In practice the only use case I'm aware of is cuspatial, which uses rapids-cmake and we control, so it's probably not a serious issue, but unless I'm misunderstanding it is a real potential issue that is worth trying to resolve before merging this.

This is a great point. Using a static GTest with libcudf would mean that conda consumers would be forced to never have GTest in the conda env. I agree with you that this is not a desired outcome, and it looks like we can't move over to a static GTest for libcudf without first splitting the testing component into a separate conda package.

I am going to close this PR in that case, as that is out of scope of this change

@robertmaynard robertmaynard requested a review from a team as a code owner April 18, 2024 21:43
@robertmaynard
Copy link
Contributor Author

Re-opnening this PR since we realized that by always building our own GTest, we don't need to worry about linking to a static system GTest which wasn't built with fPIC.

@robertmaynard robertmaynard changed the title Use static gtest and gbench everywhere Remove public gtest dependency from libcudf conda package Apr 19, 2024
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.

Re-reviewed on request. Looks good.

cpp/tests/utilities/random_seed.cpp Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Apr 19, 2024

/merge

@@ -16,86 +16,6 @@

#pragma once

#ifdef GTEST_INCLUDE_GTEST_GTEST_H_
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed now that we use a GTest version dowloaded via rapids-cmake. In addition the logic was incorrectly triggering on the devcontainer builds due to a system installed GTest causing us to break the internals of GTest

@rapids-bot rapids-bot bot merged commit 7341866 into rapidsai:branch-24.06 Apr 23, 2024
70 checks passed
@robertmaynard robertmaynard deleted the fea/static_test_dependencies branch April 23, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEA] Remove libcudf dependency on gtest / gmock
5 participants