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

Do not exclude nanoarrow and flatbuffers from installation if statically linked #17322

Merged
merged 7 commits into from
Nov 15, 2024
Merged

Do not exclude nanoarrow and flatbuffers from installation if statically linked #17322

merged 7 commits into from
Nov 15, 2024

Conversation

hyperbolic2346
Copy link
Contributor

Description

Had an issue crop up in spark-rapids-jni where we statically link arrow and the build started to fail due to change #17308.

Checklist

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

@hyperbolic2346
turn exclude_from_all off if statically linked
56c6d8b 
Signed-off-by: Mike Wilson <knobby@burntsheep.com>
@hyperbolic2346 hyperbolic2346 self-assigned this Nov 14, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 14, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Nov 14, 2024
@hyperbolic2346
Copy link
Contributor Author

This is in draft because I have been unable to verify this fixes the issue yet.

@hyperbolic2346 hyperbolic2346 added bug Something isn't working Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Nov 14, 2024
@gerashegalov
Copy link
Contributor

Thanks @hyperbolic2346 for the PR. I applied your patch and it fixes the build in spark-rapids-jni previously resulting in

08:41:00  [INFO]      [exec] CMake Error at /usr/local/cmake-3.26.4-linux-x86_64/share/cmake-3.26/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
08:41:00  [INFO]      [exec]   By not providing "Findnanoarrow.cmake" in CMAKE_MODULE_PATH this project
08:41:00  [INFO]      [exec]   has asked CMake to find a package configuration file provided by
08:41:00  [INFO]      [exec]   "nanoarrow", but CMake did not find one.

gerashegalov
gerashegalov approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

cpp/cmake/thirdparty/get_flatbuffers.cmake Outdated Show resolved Hide resolved
@hyperbolic2346 hyperbolic2346 marked this pull request as ready for review November 14, 2024 07:30
@hyperbolic2346 hyperbolic2346 requested a review from a team as a code owner November 14, 2024 07:30
@hyperbolic2346
Fixing whitespace
8ea2f24 
Signed-off-by: Mike Wilson <knobby@burntsheep.com>
@vyasr
Copy link
Contributor

vyasr commented Nov 14, 2024

/ok to test

bdice
bdice reviewed Nov 14, 2024
View reviewed changes
cpp/cmake/thirdparty/get_flatbuffers.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_nanoarrow.cmake Outdated Show resolved Hide resolved
@hyperbolic2346 @bdice
Update cpp/cmake/thirdparty/get_flatbuffers.cmake
f14936b 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
vyasr
vyasr approved these changes Nov 14, 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.

For posterity: I made the original PR but had forgotten exactly how the spark-rapids-jni builds and thought that we wouldn't need install rules for these two libraries to successfully build since all the outputs are run out of the build directory, but that turns out to be not quite right. This PR is the fix I proposed to Mike yesterday to resolve the issue.

@hyperbolic2346 @bdice
Update cpp/cmake/thirdparty/get_nanoarrow.cmake
933110a 
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@vyasr
Copy link
Contributor

vyasr commented Nov 14, 2024

/ok to test

@hyperbolic2346
Copy link
Contributor Author

Issue stems from the changes in #17322

@vyasr
Copy link
Contributor

vyasr commented Nov 14, 2024

@hyperbolic2346 you'll need to run pre-commit locally to get the cmake formatting right.

hyperbolic2346 and others added 2 commits November 14, 2024 16:34
@hyperbolic2346 @KyleFromNVIDIA
Update cpp/cmake/thirdparty/get_flatbuffers.cmake
4a37aa9 
Co-authored-by: Kyle Edwards <kyedwards@nvidia.com>
@hyperbolic2346 @KyleFromNVIDIA
Update cpp/cmake/thirdparty/get_nanoarrow.cmake
3a1212c 
Co-authored-by: Kyle Edwards <kyedwards@nvidia.com>
@vyasr
Copy link
Contributor

vyasr commented Nov 14, 2024

/ok to test

@vyasr
Copy link
Contributor

vyasr commented Nov 14, 2024

/merge

@rapids-bot rapids-bot bot merged commit 927ae9c into rapidsai:branch-24.12 Nov 15, 2024
102 checks passed
@hyperbolic2346 hyperbolic2346 deleted the mwilson/cmake_exclude_from_all branch November 15, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KyleFromNVIDIA KyleFromNVIDIA KyleFromNVIDIA approved these changes

@bdice bdice bdice approved these changes

@gerashegalov gerashegalov gerashegalov approved these changes

@vyasr vyasr vyasr approved these changes

Assignees

@hyperbolic2346 hyperbolic2346

Labels
bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hyperbolic2346 @gerashegalov @vyasr @bdice @KyleFromNVIDIA