Skip to content

[DRAFT] Minimize cuopt_static test linkage to avoid 5 GB package size explosion#1566

Open
ramakrishnap-nv wants to merge 8 commits into
mainfrom
fix-symbol-visibility-selective
Open

[DRAFT] Minimize cuopt_static test linkage to avoid 5 GB package size explosion#1566
ramakrishnap-nv wants to merge 8 commits into
mainfrom
fix-symbol-visibility-selective

Conversation

@ramakrishnap-nv

Copy link
Copy Markdown
Collaborator

Description

Follow-on to #1526 (includes all its commits). That PR correctly introduces hidden symbol visibility and cuopt_static for tests that need internal symbols — but switches all tests via the ConfigureTest macro, causing a ~67× size explosion in libcuopt-tests.

Measured impact of #1526 as-is (from its own CI artifacts):

Package Before After #1526
libcuopt-tests cuda13 linux-64 ~75 MB 4.99 GiB

This PR adds one commit on top of #1526 that limits cuopt_static linkage to only the tests that actually need it.

Analysis

Tests that genuinely need cuopt_static — routing tests only. They all include routing_test.cuh which pulls in cuopt::routing::detail types (solution_t, problem_t, fleet_order_constraints_t, fill_data_model_matrices, etc.), and local_search_cand_test.cu / top_k.cu directly construct internal solver state.

Tests that do NOT need cuopt_static:

  • MIP, LP, QP, SOCP, dual_simplex, distance engine, CLI, utilities tests
  • Several of these call raft::linalg::detail::cublassetpointermode / cusparsesetpointermode — these are raft inline headers compiled into the TU, not cuopt hidden symbols, and require no special linkage
  • cuopttestutils keeps cuopt_static PRIVATE (unchanged from Fix symbol visibility and add checks #1526); non-routing tests never call check_constraints.cu functions so the linker never pulls in that object file

GRPC tests are left unchanged — they have explicit target_link_libraries and grpc_client.cpp lacks CUOPT_EXPORT, so cuopt_static remains correct.

Changes (one commit on top of #1526)

  • cpp/tests/CMakeLists.txt: Keep ConfigureTestcuopt shared; add ConfigureInternalTestcuopt_static
  • cpp/tests/routing/CMakeLists.txt: Switch all 8 routing targets to ConfigureInternalTest

Local build verification

cmake --build cpp/build --target LP_UNIT_TEST MIP_TEST ROUTING_UNIT_TEST

# Binary sizes confirm correct linkage:
# LP_UNIT_TEST        6.2 MB  (shared)
# MIP_TEST            2.2 MB  (shared)
# ROUTING_UNIT_TEST  44.0 MB  (static — expected, accesses detail:: types)

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • Existing tests cover these changes (no test logic changed, only linkage)
  • Documentation
    • NA

arhag23 and others added 7 commits July 13, 2026 16:42
Signed-off-by: Arha Gatram <agatram@nvidia.com>
…static archive

Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
PR #1526 switched all tests to link cuopt_static, which inflates
libcuopt-tests from ~75 MB to ~5 GB (confirmed from CI artifacts)
because each of ~36 test executables embeds the full cuopt object graph.

This commit introduces ConfigureInternalTest for tests that genuinely
need access to hidden symbols, and keeps ConfigureTest on the shared
library for tests that only use the public API.

Only routing tests are switched to ConfigureInternalTest: they all
include routing_test.cuh which pulls in cuopt::routing::detail types
(solution_t, problem_t, fleet_order_constraints_t, md_utils, etc.).

MIP, LP, QP, SOCP, dual_simplex, distance engine, CLI, and utility
tests use only public cuopt API or raft::detail inline headers (which
are compiled into the TU, not linked from cuopt.so), so they stay on
the shared library.

GRPC tests are left unchanged as they have explicit target_link_libraries
and grpc_client.cpp symbols lack CUOPT_EXPORT.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ramakrishnap-nv ramakrishnap-nv force-pushed the fix-symbol-visibility-selective branch from db8af85 to 702c97d Compare July 13, 2026 21:42
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

CI Test Summary

1 failed · 30 passed · 0 skipped

@ramakrishnap-nv ramakrishnap-nv added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Jul 13, 2026
@ramakrishnap-nv ramakrishnap-nv changed the title Minimize cuopt_static test linkage to avoid 5 GB package size explosion [DRAFT] Minimize cuopt_static test linkage to avoid 5 GB package size explosion Jul 13, 2026
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: eda85fcd-1cc7-4168-bc9d-aa5403c52925

📥 Commits

Reviewing files that changed from the base of the PR and between 702c97d and 8f169d3.

📒 Files selected for processing (2)
  • ci/check_symbols.sh
  • conda/recipes/libcuopt/recipe.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • conda/recipes/libcuopt/recipe.yaml
  • ci/check_symbols.sh

📝 Walkthrough

Walkthrough

Changes

Symbol visibility export and enforcement

Layer / File(s) Summary
Export macro and public headers
cpp/include/cuopt/export.hpp, cpp/include/cuopt/**/*.hpp, cpp/src/utilities/logger.hpp
Adds CUOPT_EXPORT and applies it across public namespaces and the C API visibility region.
Library target restructuring
cpp/CMakeLists.txt
Builds cuopt_objs, cuopt, and cuopt_static from shared object files with updated properties and dependencies.
Template instantiation exports
cpp/src/grpc/..., cpp/src/io/..., cpp/src/pdlp/..., cpp/src/routing/..., cpp/src/mip_heuristics/...
Marks explicit template instantiations with CUOPT_EXPORT.
Symbol validation
ci/check_symbols.sh, conda/recipes/libcuopt/recipe.yaml
Checks installed dynamic symbols for disallowed patterns and wires the check into the conda build.
Test linkage updates
cpp/tests/**/CMakeLists.txt
Adds internal-test configuration and links internal tests against cuopt_static.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested reviewers: mlubin, gforsyth, iroy30, chris-maes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: reducing cuopt_static linkage in tests to avoid package size blowup.
Description check ✅ Passed The description is directly about the linkage fix, routing-only static linking, and package size impact.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-symbol-visibility-selective

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

ci/check_symbols.sh and conda/recipes/libcuopt/recipe.yaml were missing
the canonical suffix required by verify-copyright pre-commit hook.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp (1)

22-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Annotate optimization_problem_interface_t with CUOPT_EXPORT

cpu_optimization_problem_t is exported, but its base optimization_problem_interface_t<i_t, f_t> is declared without any visibility annotation in cpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp. That leaves the explicit instantiation in cpp/src/pdlp/cpu_optimization_problem.cpp exposed to an exported-derived/hidden-base visibility mismatch.

🤖 Prompt for 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.

In `@cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp`
around lines 22 - 45, Annotate the optimization_problem_interface_t declaration
in
cpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp
with CUOPT_EXPORT so its visibility matches the exported
cpu_optimization_problem_t base class. The explicit instantiation in
cpp/src/pdlp/cpu_optimization_problem.cpp requires no direct change; it is
corrected by the interface visibility update.
cpp/src/pdlp/solve.cu (1)

2309-2364: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Export the PDLP instantiations used by tests in cpp/src/pdlp/solve.cu:2338-2364; run_batch_pdlp, compute_optimal_batch_size, and set_pdlp_solver_mode are called from cpp/tests/linear_programming/pdlp_test.cu and cpp/tests/linear_programming/utilities/pdlp_test_utilities.cuh, so their explicit instantiations need CUOPT_EXPORT too. solve_lp_with_method can stay hidden.

🤖 Prompt for 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.

In `@cpp/src/pdlp/solve.cu` around lines 2309 - 2364, Add CUOPT_EXPORT to the
explicit instantiations of run_batch_pdlp, compute_optimal_batch_size, and
set_pdlp_solver_mode in the INSTANTIATE macro so test callers can link them.
Leave solve_lp_with_method unexported.
🧹 Nitpick comments (2)
cpp/CMakeLists.txt (1)

626-629: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead target_compile_options on cuopt_static.

cuopt_static has no sources of its own ($<TARGET_OBJECTS:cuopt_objs> only), so PRIVATE compile options here never apply to any actual compilation and are effectively dead. Safe to drop (unlike the PUBLIC compile definitions above it at Line 631, which matter as usage requirements for consumers).

🤖 Prompt for 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.

In `@cpp/CMakeLists.txt` around lines 626 - 629, Remove the target_compile_options
call for cuopt_static, including both CXX and CUDA generator-expression flags.
Leave the nearby PUBLIC compile definitions unchanged because they remain
required usage requirements for consumers.
cpp/tests/CMakeLists.txt (1)

42-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ConfigureTest and ConfigureInternalTest are near-duplicate functions.

The two functions differ only in one linked library (cuopt vs cuopt_static); everything else (include dirs, GTest links, install rules, labels) is copy-pasted. Consider factoring the common body into a private helper taking the target library as a parameter to avoid future drift between the two.

🤖 Prompt for 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.

In `@cpp/tests/CMakeLists.txt` around lines 42 - 129, Refactor the duplicated
ConfigureTest and ConfigureInternalTest implementations into a private helper
that accepts the target library as a parameter. Move the shared executable
setup, include directories, test registration, labels, install rules, and link
dependencies into that helper, then have ConfigureTest pass cuopt and
ConfigureInternalTest pass cuopt_static while preserving their current behavior.
🤖 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 `@ci/check_symbols.sh`:
- Around line 19-20: Narrow the symbol filtering in ci/check_symbols.sh so it
exempts only the intended exception-type symbols rather than any demangled name
containing “_error”. Update the awk condition used by the symbol comparison flow
to match the specific exception symbols, while allowing public API names such as
get_error_status, get_error_message, and last_error to remain in the diff.

In `@cpp/CMakeLists.txt`:
- Around line 582-599: The build configuration for cuopt_objs, cuopt_static, and
cuopt is duplicated and can drift. Extract the shared compile definitions and
link libraries—including CUDA architecture definitions, logging/CUSPARSE
definitions, CUDA, RMM, RAFT, CCCL, CUDSS, private CUDA libraries, and
conditional protobuf/gRPC dependencies—into reusable variables or a helper
function, then apply them consistently to all three targets while preserving
their existing public/private visibility.

---

Outside diff comments:
In `@cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp`:
- Around line 22-45: Annotate the optimization_problem_interface_t declaration
in
cpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp
with CUOPT_EXPORT so its visibility matches the exported
cpu_optimization_problem_t base class. The explicit instantiation in
cpp/src/pdlp/cpu_optimization_problem.cpp requires no direct change; it is
corrected by the interface visibility update.

In `@cpp/src/pdlp/solve.cu`:
- Around line 2309-2364: Add CUOPT_EXPORT to the explicit instantiations of
run_batch_pdlp, compute_optimal_batch_size, and set_pdlp_solver_mode in the
INSTANTIATE macro so test callers can link them. Leave solve_lp_with_method
unexported.

---

Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 626-629: Remove the target_compile_options call for cuopt_static,
including both CXX and CUDA generator-expression flags. Leave the nearby PUBLIC
compile definitions unchanged because they remain required usage requirements
for consumers.

In `@cpp/tests/CMakeLists.txt`:
- Around line 42-129: Refactor the duplicated ConfigureTest and
ConfigureInternalTest implementations into a private helper that accepts the
target library as a parameter. Move the shared executable setup, include
directories, test registration, labels, install rules, and link dependencies
into that helper, then have ConfigureTest pass cuopt and ConfigureInternalTest
pass cuopt_static while preserving their current behavior.
🪄 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: c78c9a15-603d-4cd0-a9f0-f714de6b6474

📥 Commits

Reviewing files that changed from the base of the PR and between 5e6e5ba and 702c97d.

📒 Files selected for processing (71)
  • ci/check_symbols.sh
  • conda/recipes/libcuopt/recipe.yaml
  • cpp/CMakeLists.txt
  • cpp/include/cuopt/error.hpp
  • cpp/include/cuopt/export.hpp
  • cpp/include/cuopt/grpc/cython_grpc_client.hpp
  • cpp/include/cuopt/grpc/grpc_client_env.hpp
  • cpp/include/cuopt/mathematical_optimization/backend_selection.hpp
  • cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp
  • cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem_solution.hpp
  • cpp/include/cuopt/mathematical_optimization/cpu_pdlp_warm_start_data.hpp
  • cpp/include/cuopt/mathematical_optimization/cuopt_c.h
  • cpp/include/cuopt/mathematical_optimization/io/data_model_view.hpp
  • cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp
  • cpp/include/cuopt/mathematical_optimization/io/mps_writer.hpp
  • cpp/include/cuopt/mathematical_optimization/io/parser.hpp
  • cpp/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hpp
  • cpp/include/cuopt/mathematical_optimization/io/writer.hpp
  • cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hpp
  • cpp/include/cuopt/mathematical_optimization/mip/solver_solution.hpp
  • cpp/include/cuopt/mathematical_optimization/optimization_problem.hpp
  • cpp/include/cuopt/mathematical_optimization/optimization_problem_solution.hpp
  • cpp/include/cuopt/mathematical_optimization/pdlp/pdlp_warm_start_data.hpp
  • cpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hpp
  • cpp/include/cuopt/mathematical_optimization/pdlp/solver_solution.hpp
  • cpp/include/cuopt/mathematical_optimization/solve.hpp
  • cpp/include/cuopt/mathematical_optimization/solve_remote.hpp
  • cpp/include/cuopt/mathematical_optimization/solver_settings.hpp
  • cpp/include/cuopt/mathematical_optimization/utilities/cython_solve.hpp
  • cpp/include/cuopt/mathematical_optimization/utilities/cython_types.hpp
  • cpp/include/cuopt/routing/assignment.hpp
  • cpp/include/cuopt/routing/cython/cython.hpp
  • cpp/include/cuopt/routing/data_model_view.hpp
  • cpp/include/cuopt/routing/routing_structures.hpp
  • cpp/include/cuopt/routing/solve.hpp
  • cpp/include/cuopt/routing/solver_settings.hpp
  • cpp/src/grpc/client/solve_remote.cpp
  • cpp/src/grpc/grpc_problem_mapper.cpp
  • cpp/src/grpc/grpc_settings_mapper.cpp
  • cpp/src/grpc/grpc_solution_mapper.cpp
  • cpp/src/io/data_model_view.cpp
  • cpp/src/io/lp_parser.cpp
  • cpp/src/io/mps_data_model.cpp
  • cpp/src/io/mps_writer.cpp
  • cpp/src/io/parser.cpp
  • cpp/src/io/writer.cpp
  • cpp/src/math_optimization/solution_reader.hpp
  • cpp/src/math_optimization/solver_settings.cu
  • cpp/src/mip_heuristics/solve.cu
  • cpp/src/mip_heuristics/solver_settings.cu
  • cpp/src/mip_heuristics/solver_solution.cu
  • cpp/src/pdlp/cpu_optimization_problem.cpp
  • cpp/src/pdlp/cpu_pdlp_warm_start_data.cu
  • cpp/src/pdlp/optimization_problem.cu
  • cpp/src/pdlp/pdlp_warm_start_data.cu
  • cpp/src/pdlp/solution_conversion.cu
  • cpp/src/pdlp/solve.cu
  • cpp/src/pdlp/solver_settings.cu
  • cpp/src/pdlp/solver_solution.cu
  • cpp/src/routing/assignment.cu
  • cpp/src/routing/data_model_view.cu
  • cpp/src/routing/distance_engine/waypoint_matrix.cpp
  • cpp/src/routing/solve.cu
  • cpp/src/routing/solver_settings.cu
  • cpp/src/routing/utilities/cython.cu
  • cpp/src/utilities/logger.hpp
  • cpp/tests/CMakeLists.txt
  • cpp/tests/examples/routing/CMakeLists.txt
  • cpp/tests/linear_programming/CMakeLists.txt
  • cpp/tests/linear_programming/grpc/CMakeLists.txt
  • cpp/tests/routing/CMakeLists.txt

Comment thread ci/check_symbols.sh
Comment on lines +19 to +20
# Ignore symbols containing "_error" since these are likely exception types
# and should be exported.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

readelf --dyn-syms symbol visibility naming convention exception types "_error" suffix

💡 Result:

There is no single, universal naming convention in the ELF specification that mandates an "_error" suffix for symbols [1][2]. When you encounter a symbol with an "_error" suffix in the output of readelf --dyn-syms, it is typically a result of project-specific internal naming conventions rather than an ELF-level standard [3][4][5]. Naming Conventions and Exception Types: In large software projects like GNU Binutils or GDB, the "_error" suffix is frequently used as a naming convention to identify functions or variables responsible for handling, reporting, or defining error conditions [3][6][4][5]. For instance, GDB uses an "_ERROR" suffix for various exception types within its common-exceptions.h header to maintain naming consistency, even when the specific symbol is not strictly a fatal error [4][5]. Similarly, internal BFD (Binary File Descriptor) library functions—such as _bfd_x86_elf_link_report_tls_invalid_section_error—use this suffix to clearly denote their purpose in error reporting during the linking process [6]. Visibility and readelf --dyn-syms: The presence of such symbols in the dynamic symbol table (viewable via readelf --dyn-syms) indicates that these symbols are exported or required for dynamic linking [1]. In many cases, developers intentionally mark these internal symbols as hidden (using attributes like ATTRIBUTE_HIDDEN) to prevent them from being visible or preemptable outside of the library itself, even if they appear in the object file [3][6][7][8]. This practice reduces the size of the dynamic symbol table and improves encapsulation, effectively making them private to the DSO (Dynamic Shared Object) while keeping them accessible for internal build-time operations [3][7][8]. In summary, the "_error" suffix is a developer-chosen label for error-handling code, and its visibility in your dynamic symbol table is a separate characteristic managed by ELF symbol visibility attributes (such as STV_HIDDEN, STV_PROTECTED, or STV_DEFAULT) [6][1].

Citations:


🏁 Script executed:

#!/bin/bash
set -eEuo pipefail

# Cleanup on exit.
cleanup() {
    if [[ -d "${TEMP_DIR:-}" ]]; then
        rm -rf "${TEMP_DIR}"
    fi
}
trap cleanup EXIT

LIB_NAME="${1}"

# Ignore symbols containing "_error" since these are likely exception types
# and should be exported.
readelf --dyn-syms --wide "${LIB_NAME}" \
    | c++filt \
    | awk '$0 !~ /_error/' \
    | sed -n 's/.*\[\(.*\)\]/\1/p' \
    | sort \
    | uniq > "${TEMP_DIR}/exported_symbols.txt"

diff -u "${TEMP_DIR}/exported_symbols.txt" "${TEMP_DIR}/expected_symbols.txt"

Repository: NVIDIA/cuopt

Length of output: 185


🏁 Script executed:

rg -n --hidden --glob '!**/.git/**' "_error|check_symbols\.sh|expected_symbols" .

Repository: NVIDIA/cuopt

Length of output: 50368


🏁 Script executed:

#!/bin/bash
set -eEuo pipefail

rg -n --hidden --glob '!**/.git/**' "_error|check_symbols\.sh|expected_symbols" .

Repository: NVIDIA/cuopt

Length of output: 50368


ci/check_symbols.sh:25 ignores too much. awk '$0 !~ /_error/' drops any demangled symbol containing _error, so public API names like get_error_status, get_error_message, or last_error never reach the diff. Narrow this to the specific exception symbols you want to exempt.

🤖 Prompt for 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.

In `@ci/check_symbols.sh` around lines 19 - 20, Narrow the symbol filtering in
ci/check_symbols.sh so it exempts only the intended exception-type symbols
rather than any demangled name containing “_error”. Update the awk condition
used by the symbol comparison flow to match the specific exception symbols,
while allowing public API names such as get_error_status, get_error_message, and
last_error to remain in the diff.

Comment thread cpp/CMakeLists.txt
Comment on lines +582 to +599
target_compile_definitions(cuopt_objs PUBLIC
CUOPT_CUDA_ARCHITECTURES="${JOINED_CUDA_ARCHITECTURES}"
CUOPT_CPU_ARCHITECTURE="${CMAKE_SYSTEM_PROCESSOR}")

target_link_libraries(cuopt_objs
PUBLIC
CUDA::cublas
CUDA::cusparse
rmm::rmm
rapids_logger::rapids_logger
CCCL::CCCL
raft::raft
${CUDSS_LIB_FILE}
PRIVATE
${CUOPT_PRIVATE_CUDA_LIBS}
$<$<BOOL:${CUOPT_ENABLE_GRPC}>:protobuf::libprotobuf>
$<$<BOOL:${CUOPT_ENABLE_GRPC}>:gRPC::grpc++>
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Triplicated build config across cuopt_objs/cuopt_static/cuopt risks drift.

Because OBJECT-library usage requirements don't propagate through $<TARGET_OBJECTS:cuopt_objs>, the same target_compile_definitions/target_link_libraries blocks (CUOPT_CUDA_ARCHITECTURES, CUOPT_CPU_ARCHITECTURE, CUOPT_LOG_ACTIVE_LEVEL, CUSPARSE_ENABLE_EXPERIMENTAL_API, CUDA::cublas/cusparse, rmm, raft, CCCL, CUDSS, CUOPT_PRIVATE_CUDA_LIBS, protobuf/grpc++) are copy-pasted across cuopt_objs (582-599), cuopt_static (604-639), and later cuopt itself. A future dependency/define change applied to only one target will silently desync the other two, potentially causing subtle ABI/link mismatches between the static-test build and the shipped shared library.

Consider hoisting these into reusable CMake list/variable(s) (e.g. CUOPT_PUBLIC_LINK_LIBS, CUOPT_COMMON_COMPILE_DEFS) and reusing them in all three target_link_libraries/target_compile_definitions calls, or wrap the common setup in a small function()/macro applied to each target.

♻️ Sketch of a shared-variable approach
+set(CUOPT_COMMON_COMPILE_DEFS
+    "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}"
+    CUSPARSE_ENABLE_EXPERIMENTAL_API
+    "CUOPT_CUDA_ARCHITECTURES=${JOINED_CUDA_ARCHITECTURES}"
+    "CUOPT_CPU_ARCHITECTURE=${CMAKE_SYSTEM_PROCESSOR}")
+set(CUOPT_PUBLIC_LINK_LIBS CUDA::cublas CUDA::cusparse rmm::rmm
+    rapids_logger::rapids_logger CCCL::CCCL raft::raft ${CUDSS_LIB_FILE})

 target_compile_definitions(cuopt_objs
-  PUBLIC "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}"
-  PUBLIC CUSPARSE_ENABLE_EXPERIMENTAL_API
+  PUBLIC ${CUOPT_COMMON_COMPILE_DEFS}
 )
 ...
 target_link_libraries(cuopt_static
-        PUBLIC
-        CUDA::cublas
-        CUDA::cusparse
-        rmm::rmm
-        rapids_logger::rapids_logger
-        CCCL::CCCL
-        raft::raft
-        ${CUDSS_LIB_FILE}
+        PUBLIC ${CUOPT_PUBLIC_LINK_LIBS}
         PRIVATE
         ${CUOPT_PRIVATE_CUDA_LIBS}
         ...
 )

Also applies to: 604-639

🤖 Prompt for 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.

In `@cpp/CMakeLists.txt` around lines 582 - 599, The build configuration for
cuopt_objs, cuopt_static, and cuopt is duplicated and can drift. Extract the
shared compile definitions and link libraries—including CUDA architecture
definitions, logging/CUSPARSE definitions, CUDA, RMM, RAFT, CCCL, CUDSS, private
CUDA libraries, and conditional protobuf/gRPC dependencies—into reusable
variables or a helper function, then apply them consistently to all three
targets while preserving their existing public/private visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants