Skip to content

scope application of -undebug so that it only applies to cuopt targets#962

Closed
tmckayus wants to merge 0 commit intoNVIDIA:mainfrom
tmckayus:scopeundebug
Closed

scope application of -undebug so that it only applies to cuopt targets#962
tmckayus wants to merge 0 commit intoNVIDIA:mainfrom
tmckayus:scopeundebug

Conversation

@tmckayus
Copy link
Contributor

With global -UNDEBUG settings, cuopt wheel builds could be turning on debug assertions in non-cuopt libraries built from source (we saw this recently with librmm). We should only set -UNDEBUG on cuopt targets.

@tmckayus tmckayus requested review from a team as code owners March 16, 2026 20:54
@tmckayus tmckayus requested review from Iroy30 and vyasr March 16, 2026 20:54
@tmckayus tmckayus added bug Something isn't working non-breaking Introduces a non-breaking change labels Mar 16, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

This PR refactors the application of the -UNDEBUG compiler flag across multiple CMakeLists.txt files. The change moves from a global add_definitions(-UNDEBUG) approach to conditional per-target target_compile_options(-UNDEBUG) when the DEFINE_ASSERT flag is set. This prevents the flag from leaking into dependencies and applies it only where needed.

Changes

Cohort / File(s) Summary
Main Build Configuration
cpp/CMakeLists.txt
Removed global -UNDEBUG definition and added conditional per-target -UNDEBUG compile options for cuopt, cuopt_cli, solve_MIP, and solve_LP targets when DEFINE_ASSERT is set.
Test CMakeLists
cpp/libmps_parser/tests/CMakeLists.txt, cpp/tests/CMakeLists.txt, cpp/tests/examples/routing/CMakeLists.txt, cpp/tests/linear_programming/CMakeLists.txt
Added conditional per-target -UNDEBUG compile options to test executables and libraries (cuopttestutils, test targets, routing example targets, and C API test targets) when DEFINE_ASSERT is set.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the primary change: restricting -UNDEBUG application to cuopt targets only, moving from a global to per-target scope.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem (debug assertions in non-cuopt libraries) and the solution (per-target -UNDEBUG application).
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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

Tip

CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.

OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
cpp/CMakeLists.txt (1)

187-190: Update the explanatory comment to match the actual mechanism.

Line 187-190 says -UNDEBUG is applied via target_compile_definitions, but the implementation uses target_compile_options. Keeping this comment accurate will prevent future confusion.

Suggested comment-only fix
-# Note: -UNDEBUG is applied via target_compile_definitions (not add_definitions)
-# to avoid leaking into dependencies like RMM that are built in-tree.
-# See target_compile_definitions(cuopt ...) and target_compile_definitions(cuopt_cli ...)
-# below, after those targets are defined.
+# Note: -UNDEBUG is applied via target_compile_options (not add_definitions)
+# to avoid leaking into dependencies like RMM that are built in-tree.
+# See target_compile_options(cuopt ...) and target_compile_options(cuopt_cli ...)
+# below, after those targets are defined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp/CMakeLists.txt` around lines 187 - 190, Update the explanatory comment to
reflect that the -UNDEBUG flag is applied via target_compile_options rather than
target_compile_definitions; locate the comment near the references to
target_compile_definitions(cuopt ...) and target_compile_definitions(cuopt_cli
...) and change the wording to state that -UNDEBUG is applied via
target_compile_options (not add_definitions) to avoid leaking into dependencies
like RMM, mentioning the relevant targets (cuopt and cuopt_cli) for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 187-190: Update the explanatory comment to reflect that the
-UNDEBUG flag is applied via target_compile_options rather than
target_compile_definitions; locate the comment near the references to
target_compile_definitions(cuopt ...) and target_compile_definitions(cuopt_cli
...) and change the wording to state that -UNDEBUG is applied via
target_compile_options (not add_definitions) to avoid leaking into dependencies
like RMM, mentioning the relevant targets (cuopt and cuopt_cli) for clarity.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d147777c-04c6-4d57-b8cb-e44c55caa54a

📥 Commits

Reviewing files that changed from the base of the PR and between c5116e1 and 66c9d63.

📒 Files selected for processing (5)
  • cpp/CMakeLists.txt
  • cpp/libmps_parser/tests/CMakeLists.txt
  • cpp/tests/CMakeLists.txt
  • cpp/tests/examples/routing/CMakeLists.txt
  • cpp/tests/linear_programming/CMakeLists.txt

@anandhkb anandhkb added this to the 26.04 milestone Mar 16, 2026
@tmckayus tmckayus added the P1 label Mar 17, 2026
@tmckayus tmckayus closed this Mar 17, 2026
@tmckayus
Copy link
Contributor Author

accidentally closed ...

@tmckayus
Copy link
Contributor Author

continuing

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

Labels

bug Something isn't working non-breaking Introduces a non-breaking change P1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants