Skip to content

Improve installation hygiene of built and header-only dependencies - #22341

Merged
rapids-bot[bot] merged 26 commits into
NVIDIA:mainfrom
vyasr:feat/minimize_installation
May 11, 2026
Merged

Improve installation hygiene of built and header-only dependencies#22341
rapids-bot[bot] merged 26 commits into
NVIDIA:mainfrom
vyasr:feat/minimize_installation

Conversation

@vyasr

@vyasr vyasr commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Description

Historically we have taken a very piecemeal approach to controlling when we build some dependency statically or dynamically, and we've patched out header-only dependencies from our installation on a case-by-case basis. This PR standardizes that process with a few common controls. Specifically, we assume that there are almost no practical use cases where we want a CPM-downloaded package to be built as a shared library; if we want to dynamically link, it will be because we found the shared library on the system (e.g. a conda environment). Further, we assume that installation of private header-only deps is almost never desirable, so we should always EXCLUDE_FROM_ALL all private dependencies.

Since rmm and rapids-logger are public dependencies, handling them is a bit more challenging. If they are downloaded as part of the build, they must also be installed along with the necessary CMake, headers, and libraries. In the case where we are building a shared libcudf.so but statically linking in the dependencies, however, we instead subsume those dependencies with whole-archive linking and revendor the headers directly.

Checklist

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

@vyasr vyasr self-assigned this Apr 30, 2026
@vyasr
vyasr requested a review from a team as a code owner April 30, 2026 19:16
@vyasr vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 30, 2026
@copy-pr-bot

copy-pr-bot Bot commented Apr 30, 2026

Copy link
Copy Markdown

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 Apr 30, 2026

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All seems fine to me. Devcontainers and pip/conda will be verified by CI, but it'd be good to make sure that Spark builds work with these changes (sometimes that CI job is failing for other reasons).

Comment thread cpp/cmake/thirdparty/get_rmm.cmake
@vyasr
vyasr force-pushed the feat/minimize_installation branch 3 times, most recently from b8c6f9d to ff6a861 Compare April 30, 2026 20:01
Comment thread cpp/CMakeLists.txt
Comment thread cpp/CMakeLists.txt Outdated
Comment thread cpp/CMakeLists.txt Outdated
@vyasr
vyasr force-pushed the feat/minimize_installation branch from 41f6c6f to e387c3d Compare April 30, 2026 20:49
@vyasr
vyasr requested a review from robertmaynard April 30, 2026 20:50
@vyasr
vyasr force-pushed the feat/minimize_installation branch from aeae8ea to 245ee2f Compare April 30, 2026 22:43
@vyasr
vyasr requested review from a team as code owners May 2, 2026 05:45
@github-actions github-actions Bot added the Java Affects Java cuDF API. label May 2, 2026
@vyasr
vyasr force-pushed the feat/minimize_installation branch 5 times, most recently from 29563c8 to 413b42e Compare May 5, 2026 15:39
vyasr and others added 7 commits May 5, 2026 10:36
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
vyasr added 16 commits May 6, 2026 11:01
When building shared libcudf with static dependencies, rmm and
rapids_logger are absorbed into the shared library using
--whole-archive to ensure all symbols are exported. This commit adds:

- Conditional WHOLE_ARCHIVE linkage for rmm/rapids_logger
- BUILD_INTERFACE wrapping to hide absorbed static deps from install
- Transitive dependency promotion for absorbed libraries
- Export set merging (rmm-exports/rapids_logger-exports into cudf-exports)
- Conda/pre-installed shared library detection (TYPE introspection)
- DSO header installation for standalone mode
- get_rmm.cmake: full EXCLUDE_FROM_ALL support + conda fix
- get_nvtx.cmake: remove export sets (rmm handles nvtx publicly)
- rapids_logger: conditional static build + export package for conda
WHOLE_ARCHIVE is only meaningful when absorbing a static library into
a shared library. Applying it to a shared library target triggers CMake
warnings. Gate both WHOLE_ARCHIVE and BUILD_INTERFACE behind the
TYPE==STATIC_LIBRARY check so the genex is never generated for
pre-installed shared deps (e.g. conda rmm/rapids_logger).
Two issues caused CI failure:

1. foreach(_mode BUILD INSTALL) used uppercase, but rapids-cmake export
   targets use lowercase names (rapids_export_build_*, rapids_export_install_*).
   The if(NOT TARGET) check silently skipped the entire merge block.

2. get_nvtx.cmake was missing BUILD_EXPORT_SET/INSTALL_EXPORT_SET args.
   When rmm is absorbed into libcudf, its public dep nvtx3-cpp must be in
   an export set for CMake's install(EXPORT) validation to pass.
When static deps (rmm, rapids_logger) are absorbed into libcudf.so via
whole-archive, they are not installed separately. The export-set merging
loop was copying rmm-exports' install packages (including rapids_logger)
into cudf-exports, producing a find_dependency(rapids_logger) that would
fail for consumers of an installed standalone build.

Skip packages matching _absorbed_deps names when merging install-side
export set metadata.
When promoting an absorbed library's transitive deps into cudf's public
interface, filter out deps that are themselves absorbed (already linked
via whole-archive) and LINK_ONLY entries (private link deps already
statically linked into the absorbed library).
nvtx3 is header-only. By installing its headers directly into cudf's
install tree, consumers find them via cudf's include path without needing
find_dependency(nvtx3) or any separate nvtx3 package.

Changes:
- get_nvtx.cmake: remove INSTALL_EXPORT_SET; propagate nvtx3_SOURCE_DIR
  to parent scope for header installation
- get_rmm.cmake: strip nvtx3::nvtx3-cpp from rmm's INTERFACE_LINK_LIBRARIES
  after fetch, preventing the absorption loop from promoting it into
  cudf's installed public interface
- CMakeLists.txt: filter nvtx3 from install-side export set merging;
  install nvtx3 headers from nvtx3_SOURCE_DIR in the DSO headers section

@jameslamb jameslamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a suggestion, but otherwise approving.

Comment thread dependencies.yaml Outdated
@coderabbitai

coderabbitai Bot commented May 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Refactored CMake build system for improved control over static and shared dependencies.
    • Added flexible build options for managing dependency installation and export behavior.
    • Enhanced dependency configuration for test and Java/JNI builds.
    • Optimized dependency linking mechanism for shared library builds.

Walkthrough

This PR refactors cudf's CMake build system to centralize static/shared dependency control. It introduces a 3-state CUDF_BUILD_STATIC_DEPS cache variable (ON/OFF/FORCE) that drives derived configuration of dependency installation and CPM packaging behavior, parameterizes thirdparty dependency functions to accept shared/static and EXCLUDE_FROM_ALL controls, reworks the cudf target to absorb static dependencies via whole-archive while promoting their transitive public dependencies, and updates build-specific configurations (tests, Java) to opt into the new strategy.

Changes

Centralized Dependency Control & Parameterization

Layer / File(s) Summary
Cache Variable Foundation
cpp/CMakeLists.txt
Introduces CUDF_BUILD_STATIC_DEPS (3-state: ON/OFF/FORCE) as the primary control, derives CUDA_STATIC_RUNTIME, CUDF_INSTALL_LIBRARY_DEPS, CUDF_EXCLUDE_DEPS_FROM_ALL, and CUDF_DEPS_BUILD_SHARED as centralized flags.
CPM Behavior & Conditional Export
cpp/CMakeLists.txt, cpp/cmake/thirdparty/get_arrow.cmake, cpp/cmake/thirdparty/get_nvtx.cmake
Enables CPM_DOWNLOAD_ALL when CUDF_BUILD_STATIC_DEPS is FORCE; updates rapids_logger and nvtx3 to conditionally export packages and exclude from install based on CUDF_INSTALL_LIBRARY_DEPS.
RMM Parameterization
cpp/cmake/thirdparty/get_rmm.cmake
Converts find_and_configure_rmm to accept BUILD_SHARED and EXCLUDE_FROM_ALL, adjusts CPM arguments and target type detection, propagates rmm_SOURCE_DIR/rmm_BINARY_DIR to parent scope.
NVComp Installation & Target Selection
cpp/cmake/thirdparty/get_nvcomp.cmake
Adds INSTALL_LIBRARY option; conditionally installs artifacts; exposes both nvcomp::nvcomp and nvcomp::nvcomp_static targets; selects appropriate target based on build mode.
Standard Dependency Threading
cpp/cmake/thirdparty/get_{arrow,croaring,dlpack,flatbuffers,kvikio,zstd,nanoarrow}.cmake, cpp/cmake/thirdparty/get_{cucollections,thread_pool}.cmake
Consistently parameterize dependency functions to accept EXCLUDE_FROM_ALL and/or BUILD_SHARED; remove local _exclude_from_all helpers; thread through CUDF_EXCLUDE_DEPS_FROM_ALL and CUDF_DEPS_BUILD_SHARED; nanoarrow selects shared/static target variant.
CUDF Target Linkage & Whole-Archive Absorption
cpp/CMakeLists.txt
Reworks cudf target to absorb static rmm/rapids_logger via whole-archive, promotes absorbed deps' public transitive dependencies into cudf's PUBLIC interface with filtering, merges export metadata into cudf-exports, strips nvtx3 from rmm when absorbed.
Header Installation Fallback
cpp/CMakeLists.txt
Adds header-only installation for rmm, rapids_logger, and nvtx3 when CUDF_INSTALL_LIBRARY_DEPS is OFF, supporting standalone DSO deployments.
Build-Specific Overrides
cpp/tests/CMakeLists.txt, java/src/main/native/CMakeLists.txt
Test build disables library installation and marks dependencies EXCLUDE_FROM_ALL; Java build enables library installation for JNI static linking.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Improve installation hygiene of built and header-only dependencies' directly and clearly describes the main objective of the PR: standardizing and improving how dependencies are installed, which is the primary focus of all the changes across multiple CMake configuration files.
Description check ✅ Passed The description comprehensively explains the rationale, approach, and implementation details of standardizing dependency controls, covering the key changes for static builds, private header-only dependencies, and public dependencies (rmm/rapids-logger) with whole-archive absorption.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cpp/CMakeLists.txt`:
- Around line 1101-1112: The code is mutating the actual rmm::rmm target
(_rmm_real / _rmm_libs) to remove nvtx3::nvtx3-cpp; instead, stop altering the
real RMM target and perform the nvtx3 filter only on the interface list you plan
to promote into cuDF (e.g., build a separate _iface_libs variable from
get_target_property(... INTERFACE_LINK_LIBRARIES) and list(REMOVE_ITEM
_iface_libs nvtx3::nvtx3-cpp) before using set_property or installing/setting
cuDF's INTERFACE_LINK_LIBRARIES), leaving rmm::rmm and its ALIASED_TARGET
untouched so other in-tree targets still see nvtx headers.
- Around line 320-323: CUDF_BUILD_STATIC_DEPS currently sets CPM_DOWNLOAD_ALL ON
only when equal to "FORCE", but because CPM_DOWNLOAD_ALL is a CMake cache
variable it must be explicitly reset when not in FORCE mode; update the CMake
logic around CUDF_BUILD_STATIC_DEPS to add an else() branch that sets
CPM_DOWNLOAD_ALL to OFF (mirroring how CUDA_STATIC_RUNTIME is handled) so
previously configured FORCE builds don’t leave CPM_DOWNLOAD_ALL cached ON after
switching to ON or OFF.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 963eee15-17d8-48e3-b7e5-f35aeb338748

📥 Commits

Reviewing files that changed from the base of the PR and between 64a3109 and 7890c4a.

📒 Files selected for processing (15)
  • cpp/CMakeLists.txt
  • cpp/cmake/thirdparty/get_arrow.cmake
  • cpp/cmake/thirdparty/get_croaring.cmake
  • cpp/cmake/thirdparty/get_cucollections.cmake
  • cpp/cmake/thirdparty/get_dlpack.cmake
  • cpp/cmake/thirdparty/get_flatbuffers.cmake
  • cpp/cmake/thirdparty/get_kvikio.cmake
  • cpp/cmake/thirdparty/get_nanoarrow.cmake
  • cpp/cmake/thirdparty/get_nvcomp.cmake
  • cpp/cmake/thirdparty/get_nvtx.cmake
  • cpp/cmake/thirdparty/get_rmm.cmake
  • cpp/cmake/thirdparty/get_thread_pool.cmake
  • cpp/cmake/thirdparty/get_zstd.cmake
  • cpp/tests/CMakeLists.txt
  • java/src/main/native/CMakeLists.txt

Comment thread cpp/CMakeLists.txt
Comment thread cpp/CMakeLists.txt
@vyasr

vyasr commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@vyasr
vyasr dismissed robertmaynard’s stale review May 11, 2026 17:48

Stale, addressed.

@rapids-bot
rapids-bot Bot merged commit dd3b8ad into NVIDIA:main May 11, 2026
250 of 253 checks passed
@vyasr
vyasr deleted the feat/minimize_installation branch May 11, 2026 17:48
rapids-bot Bot pushed a commit that referenced this pull request May 11, 2026
#22341 removed the usage of this function in `get_nvcomp`, which I suspect was previously being relied on to supply the definition for some of these others. We do properly fetch it in `get_arrow.cmake` as well so it's just a matter of when each file is included.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

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

URL: #22460
shrshi pushed a commit to shrshi/cudf that referenced this pull request May 12, 2026
…VIDIA#22341)

Historically we have taken a very piecemeal approach to controlling when we build some dependency statically or dynamically, and we've patched out header-only dependencies from our installation on a case-by-case basis. This PR standardizes that process with a few common controls. Specifically, we assume that there are almost no practical use cases where we want a CPM-downloaded package to be built as a shared library; if we want to dynamically link, it will be because we found the shared library on the system (e.g. a conda environment). Further, we assume that installation of private header-only deps is almost never desirable, so we should always `EXCLUDE_FROM_ALL` all private dependencies.

Since rmm and rapids-logger are public dependencies, handling them is a bit more challenging. If they are downloaded as part of the build, they must also be installed along with the necessary CMake, headers, and libraries. In the case where we are building a shared libcudf.so but statically linking in the dependencies, however, we instead subsume those dependencies with whole-archive linking and revendor the headers directly.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - MithunR (https://github.com/mythrocks)
  - James Lamb (https://github.com/jameslamb)

URL: NVIDIA#22341
shrshi pushed a commit to shrshi/cudf that referenced this pull request May 12, 2026
NVIDIA#22341 removed the usage of this function in `get_nvcomp`, which I suspect was previously being relied on to supply the definition for some of these others. We do properly fetch it in `get_arrow.cmake` as well so it's just a matter of when each file is included.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

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

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

Labels

CMake CMake build issue improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants