-
Notifications
You must be signed in to change notification settings - Fork 47
Disable debug symbol in cuopt_cli #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: branch-25.08
Are you sure you want to change the base?
Disable debug symbol in cuopt_cli #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR and the linked issue don't describe the issue in much detail so it's hard for me to say much... but this is a good change anyway. Changing the CMake build type to Debug
(https://cmake.org/cmake/help/v4.1/variable/CMAKE_BUILD_TYPE.html) if you needed debugging symbols is preferable to manually adding -g
and changing the optimization level like this... that'll be portable to more compilers and influence other decisions CMake makes.
And I don't think we should be shipping packages with debug symbols to users anyway.
It may also not have been auditwheel repair
that was stripping symbols. scikit-build-core
does it by default: https://github.com/scikit-build/scikit-build-core/blob/a7d5bf423013100892321f3289123fa9afba1f15/README.md#L250-L251
You should also check if cutting this would allow for a stricter limit on binary size. For example, it looks like the cuopt
wheels are around 40MiB compressed:
----- package inspection summary -----
file size
* compressed size: 39.743M
* uncompressed size: 97.652M
* compression space saving: 59.3%
So you could probably reduce the upper limit on size from 100 MiB to something like 50, to help detect unexpected package size growth sooner:
cuopt/python/cuopt/pyproject.toml
Lines 122 to 128 in 63fbb6b
[tool.pydistcheck] | |
select = [ | |
"distro-too-large-compressed", | |
] | |
# detect when package size grows significantly | |
max_allowed_size_compressed = '100M' |
@jameslamb at the time of testing, it was segfaulting only after repair, I tested before that step and it was working. So for time being, enable debug symbol for cli to unblock. And when I tried to repro and fix as follow-up, it seemed like it got fixed. So added changes to remove it in this PR. |
Seems like issue still persists. |
cpp/CMakeLists.txt
Outdated
@@ -75,7 +75,7 @@ set(CUOPT_CXX_FLAGS "") | |||
set(CUOPT_CUDA_FLAGS "") | |||
|
|||
if(CMAKE_COMPILER_IS_GNUCXX) | |||
list(APPEND CUOPT_CXX_FLAGS -Werror -Wno-error=deprecated-declarations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test comment
@@ -307,7 +307,6 @@ endif() | |||
|
|||
|
|||
|
|||
list(APPEND CUOPT_CXX_FLAGS -g -O0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test comment
Description
cuopt_cli was failing due to unknown reason when it as being stripped wheel repair, so debug symbols was enabled so it will not be stripped. But it seems there was an update to the patch which fixed this.
Issue
closes #94
Checklist