Skip to content

apply -UNDEBUG to coupt flags instead of global settings#964

Open
tmckayus wants to merge 2 commits intoNVIDIA:mainfrom
tmckayus:scopeundebug
Open

apply -UNDEBUG to coupt flags instead of global settings#964
tmckayus wants to merge 2 commits intoNVIDIA:mainfrom
tmckayus:scopeundebug

Conversation

@tmckayus
Copy link
Contributor

don't pass -UNDEBUG to other libraries we may build

@tmckayus tmckayus requested review from a team as code owners March 17, 2026 17:30
@tmckayus tmckayus requested a review from rgsl888prabhu March 17, 2026 17:30
@tmckayus tmckayus added bug Something isn't working non-breaking Introduces a non-breaking change labels Mar 17, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8e63646a-b756-472b-85b9-5a303adf4bda

📥 Commits

Reviewing files that changed from the base of the PR and between b2eddb2 and 7a33c33.

📒 Files selected for processing (3)
  • cpp/CMakeLists.txt
  • cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
  • python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py
💤 Files with no reviewable changes (1)
  • python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/CMakeLists.txt

📝 Walkthrough

Walkthrough

Moves -UNDEBUG application from a global add_definitions block to appending -UNDEBUG into CUOPT_CXX_FLAGS / CUOPT_CUDA_FLAGS when DEFINE_ASSERT is set, and re-enables previously disabled CPU-only tests by removing DISABLED_ prefixes and top-level pytest skip markers.

Changes

Cohort / File(s) Summary
CMake Build Configuration
cpp/CMakeLists.txt
Replaced unconditional in-tree add_definitions(-UNDEBUG)/undefine-NDEBUG approach with appending -UNDEBUG to CUOPT_CXX_FLAGS and CUOPT_CUDA_FLAGS when DEFINE_ASSERT is active; removed global undefine block so flag propagation is target-scoped.
C++ CPU-only tests
cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
Enabled two CPU-only tests by removing DISABLED_ prefixes: lp_solve and mip_solve.
Python CPU-only tests
python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py
Removed top-level pytest skip markers on CPU-only test classes, re-enabling those tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: applying -UNDEBUG to cuopt-specific flags instead of global settings, which matches the primary objective of the PR.
Description check ✅ Passed The description directly relates to the changeset by explaining the rationale: preventing -UNDEBUG from being passed to other libraries, which aligns with the scope changes in CMakeLists.txt.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

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

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

@tmckayus tmckayus requested a review from a team as a code owner March 17, 2026 17:34
@rgsl888prabhu
Copy link
Collaborator

/ok to test b2eddb2

@tmckayus
Copy link
Contributor Author

The previous way of doing this added -UNDEBUG to everything built under the cpp directory, including fetched dependencies, if assert mode was enabled. This change applies it only to the cuopt specific compilation flags.

@anandhkb anandhkb added this to the 26.04 milestone Mar 17, 2026
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants