build: don't install libcuopt into conda env by default - #1650
build: don't install libcuopt into conda env by default#1650ramakrishnap-nv wants to merge 8 commits into
Conversation
`build.sh` previously ran `cmake --install` into `$CONDA_PREFIX` on every build, so agents or developers that called `ninja` directly (or rebuilt without reinstalling) would run stale installed libraries rather than their freshly compiled code. Change the default to build-only and add `--install` to opt in when an explicit conda-env install is wanted. The `-n` (no-install) flag is removed since it is now the default behaviour. Also simplify the cmake invocation: the separate `make ninja-build` code path that was only reachable via `-n` is replaced by a single `cmake --build` call with an optional `--target install`. The conda recipe for libcuopt already passed `-n` explicitly; that flag is dropped since it is now redundant. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The previous commit accidentally removed the `make ninja-build` path that was used for build-only (no-install) invocations. Restore it as the default path; the `--install` flag takes the cmake install path. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesBuild and version behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CI Test Summary✅ All 31 test job(s) passed. |
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
added
breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@build.sh`:
- Around line 391-395: Add regression tests covering the build.sh mode
selection: verify the default invocation runs the ninja-build target without
installation, and verify --install invokes the CMake install target. Place the
tests in the appropriate existing test framework and assert the $CONDA_PREFIX
side-effect contract.
🪄 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: bde3be73-b4fe-4052-aea2-04411db0b9af
📒 Files selected for processing (1)
build.sh
| elif hasArg --install; then | ||
| cmake --build "${LIBCUOPT_BUILD_DIR}" --target ${INSTALL_TARGET} ${VERBOSE_FLAG} ${JFLAG} | ||
| else | ||
| # Manual make invocation to start its jobserver | ||
| make ${JFLAG} -C "${REPODIR}/cpp" LIBCUOPT_BUILD_DIR="${LIBCUOPT_BUILD_DIR}" VERBOSE_FLAG="${VERBOSE_FLAG}" PARALLEL_LEVEL="${PARALLEL_LEVEL}" ninja-build |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression coverage for both build modes.
Verify that a default invocation runs ninja-build without installation and that --install invokes the CMake install target. This protects the $CONDA_PREFIX side-effect contract.
As per coding guidelines, contributors must add unit tests for code changes, using GoogleTest examples under cpp/src/tests for C/C++ and pytest examples under python/cuopt/cuopt/tests for Python.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 392-392: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 395-395: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@build.sh` around lines 391 - 395, Add regression tests covering the build.sh
mode selection: verify the default invocation runs the ninja-build target
without installation, and verify --install invokes the CMake install target.
Place the tests in the appropriate existing test framework and assert the
$CONDA_PREFIX side-effect contract.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CONTRIBUTING.md`:
- Around line 177-178: Update the installation-path documentation around the
--install description to refer to the `$PREFIX` environment variable instead of
`$INSTALL_PREFIX`, matching build.sh’s supported configuration while preserving
the existing `$CONDA_PREFIX` fallback wording.
🪄 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: ed5a9bcb-77bd-4389-9873-ce84833be922
📒 Files selected for processing (1)
CONTRIBUTING.md
- CONTRIBUTING.md referenced \$INSTALL_PREFIX but build.sh derives the install prefix from \$PREFIX (falling back to \$CONDA_PREFIX); use the correct variable name - Double-quote \$VERBOSE_FLAG and \$JFLAG in the cmake/make invocations to silence shellcheck SC2086 and prevent word splitting Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Adds the resolved library path to the version line that cuopt_cli prints at the start of every solve. Makes it immediately visible which library is actually loaded — build-dir or conda-installed. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/src/utilities/version_info.cpp (1)
178-181: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a data anchor for the
dladdrlookup.The current call converts
&print_version_infofrom a function pointer tovoid*. With position-independent code,dladdrcan resolve a PLT entry and report the wrong object. Use a function-local static data object, such asstatic constexpr char library_anchor = 0;, and pass&library_anchortodladdr. Add a C++ test for the reported library path.🤖 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/utilities/version_info.cpp` around lines 178 - 181, Update the dladdr lookup in print_version_info to use a function-local static data anchor, such as library_anchor, instead of converting the function pointer to void*. Preserve the existing "<unknown>" fallback and add a C++ test that verifies the reported library path.
🤖 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/src/utilities/version_info.cpp`:
- Around line 178-189: Add GoogleTest coverage under cpp/src/tests for the
library-path resolution used by print_version_info, verifying both successful
dladdr lookup and the "<unknown>" fallback. If print_version_info depends on
unavailable CUDA or logging setup, extract the lookup into a small testable
helper and test that helper without changing the existing logging behavior.
- Around line 15-16: Update the CMake target configuration for the targets
containing version_info.cpp to define _GNU_SOURCE and link ${CMAKE_DL_LIBS};
ensure the compile definition enables Dl_info/dladdr declarations and the loader
library is applied to every affected target.
---
Nitpick comments:
In `@cpp/src/utilities/version_info.cpp`:
- Around line 178-181: Update the dladdr lookup in print_version_info to use a
function-local static data anchor, such as library_anchor, instead of converting
the function pointer to void*. Preserve the existing "<unknown>" fallback and
add a C++ test that verifies the reported library path.
🪄 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: 94a672d2-7897-4134-90ac-3ccf6a3e3533
📒 Files selected for processing (1)
cpp/src/utilities/version_info.cpp
| #include <dlfcn.h> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'CMAKE_DL_LIBS|_GNU_SOURCE|target_link_libraries|dladdr|dlfcn\.h' \
--glob 'CMakeLists.txt' \
--glob '*.cmake' \
--glob '*.cpp' \
--glob '*.hpp' \
--glob '*.h' \
.Repository: NVIDIA/cuopt
Length of output: 18267
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- version_info.cpp ---'
sed -n '1,30p;165,195p' cpp/src/utilities/version_info.cpp
printf '%s\n' '--- cuopt target configuration ---'
sed -n '640,785p' cpp/CMakeLists.txt
printf '%s\n' '--- feature and linker definitions ---'
rg -n -C 4 \
'_GNU_SOURCE|CMAKE_CXX_STANDARD|add_compile_definitions|target_compile_definitions|target_compile_options|CMAKE_DL_LIBS|dladdr|Dl_info' \
--glob 'CMakeLists.txt' \
--glob '*.cmake' \
--glob '*.cpp' \
--glob '*.hpp' \
--glob '*.h' \
.Repository: NVIDIA/cuopt
Length of output: 19298
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- CUOPT_CXX_FLAGS definitions ---'
rg -n -C 5 \
'CUOPT_CXX_FLAGS|CMAKE_CXX_FLAGS|CXXFLAGS|_GNU_SOURCE|CMAKE_DL_LIBS' \
--glob 'CMakeLists.txt' \
--glob '*.cmake' \
--glob '*.yaml' \
--glob '*.yml' \
--glob '*.sh' \
.
printf '%s\n' '--- dlfcn.h feature guards ---'
for header in /usr/include/dlfcn.h /usr/include/*/dlfcn.h; do
if [ -f "$header" ]; then
printf '%s\n' "--- $header ---"
rg -n -C 3 'Dl_info|dladdr|__USE_GNU|_GNU_SOURCE' "$header"
fi
doneRepository: NVIDIA/cuopt
Length of output: 11664
Define the GNU feature macro and link the dynamic loader.
version_info.cpp uses GNU-only Dl_info and dladdr, but the build defines neither _GNU_SOURCE nor ${CMAKE_DL_LIBS}. Add the feature macro as a target compile definition and link ${CMAKE_DL_LIBS} to the targets that contain these objects. Linux builds can otherwise fail during compilation or linking.
🤖 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/utilities/version_info.cpp` around lines 15 - 16, Update the CMake
target configuration for the targets containing version_info.cpp to define
_GNU_SOURCE and link ${CMAKE_DL_LIBS}; ensure the compile definition enables
Dl_info/dladdr declarations and the loader library is applied to every affected
target.
| Dl_info dl_info{}; | ||
| const char* lib_path = (dladdr(reinterpret_cast<void*>(&print_version_info), &dl_info) && dl_info.dli_fname) | ||
| ? dl_info.dli_fname | ||
| : "<unknown>"; | ||
| CUOPT_LOG_INFO("cuOpt version: %d.%d.%d, git hash: %s, host arch: %s, device archs: %s, lib: %s", | ||
| CUOPT_VERSION_MAJOR, | ||
| CUOPT_VERSION_MINOR, | ||
| CUOPT_VERSION_PATCH, | ||
| CUOPT_GIT_COMMIT_HASH, | ||
| CUOPT_CPU_ARCHITECTURE, | ||
| CUOPT_CUDA_ARCHITECTURES); | ||
| CUOPT_CUDA_ARCHITECTURES, | ||
| lib_path); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add GoogleTest coverage for the new library-path logging.
Add a test under cpp/src/tests for the successful path lookup and the "<unknown>" fallback. Extract the lookup into a small testable helper when print_version_info requires unavailable CUDA or logging setup. No test change is included in the supplied context.
As per coding guidelines, C++ changes must add unit tests with GoogleTest under cpp/src/tests.
🤖 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/utilities/version_info.cpp` around lines 178 - 189, Add GoogleTest
coverage under cpp/src/tests for the library-path resolution used by
print_version_info, verifying both successful dladdr lookup and the "<unknown>"
fallback. If print_version_info depends on unavailable CUDA or logging setup,
extract the lookup into a small testable helper and test that helper without
changing the existing logging behavior.
Source: Coding guidelines
conda's \$LDFLAGS injects -rpath,\$CONDA_PREFIX/lib into every linked binary, which means a previously installed libcuopt.so in the conda env always shadows the freshly compiled build-dir copy regardless of RUNPATH ordering. When building without --install, explicitly remove any libcuopt*.so* from the conda prefix so the build-dir library is the only copy available. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
- Remove dladdr/lib-path debug print added during local testing - Fix shellcheck SC2010: replace ls|grep with compgen glob check Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
mlubin
left a comment
mlubin
left a comment
There was a problem hiding this comment.
Maybe hold off on landing until after 26.08, we don't want broken build scripts at release time.
The unconditional removal broke conda-python-build: rattler-build legitimately installs libcuopt into the host prefix before running ./build.sh cuopt, and we were deleting it immediately. Scope the removal to invocations that actually rebuild libcuopt (default/libcuopt/cuopt_grpc_server targets); Python-only builds are left untouched. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
4 participants
Summary
build.shpreviously rancmake --installinto\$CONDA_PREFIXon every build. Agents or developers that rebuilt without reinstalling (e.g. calledninjadirectly) would run the stale installed library rather than freshly compiled code — a silent mismatch with no easy diagnosis path.--installto explicitly opt in to installing into the active conda environment.-n(no-install) flag is removed since build-only is now the default.make ninja-buildcode path (only reachable via-n) is replaced by a singlecmake --buildcall with an optional--target install.libcuoptalready passed-nexplicitly; that flag is dropped since it is now redundant — the recipe's separatecmake --install cpp/buildoutput step is unaffected.🤖 Generated with Claude Code