scikit-build / scikit-build-core

A next generation Python CMake adaptor and Python API for plugins
https://scikit-build-core.readthedocs.io
Apache License 2.0
223 stars 47 forks source link

Why does scikit-build-core strip debug info by default? #875

Closed godlygeek closed 2 days ago

godlygeek commented 2 weeks ago

The documentation explains that debug info is stripped by default regardless of build type, but doesn't explain why. It is extremely surprising to me that, on top of configuring CMake to generate debug info for my extension module, I additionally need to configure scikit-build-core to leave that debug info alone. What's the rationale for that default? Why doesn't tool.scikit-build.cmake.build-type = "RelWithDebInfo" imply tool.scikit-build.install.strip = False ?

henryiii commented 2 weeks ago

Making this default dynamic based on build-type seems reasonable. Would we register a list of true build-types or a list of false build-types, though?

godlygeek commented 2 weeks ago

I'm not clear on why anyone would ever want scikit-build-core to strip debug info. What's the motivation for this feature? There clearly must be a use case if someone went to the trouble of adding it and switching the default to it, but I'm not understanding what that use case is.

ZeroIntensity commented 2 weeks ago

At the very least, I think the documentation should clarify to use strip=false if debug information is needed, instead of burying it in the API reference. Defaulting to true makes debugging very painful!

godlygeek commented 2 weeks ago

I'm not clear on why anyone would ever want scikit-build-core to strip debug info.

Ah, perhaps I see it: is it for handling the case where CMake is configured to not generate debug info, but a library containing debug info is statically linked into the extension module shared library, and so the final .so contains some debug info that the author doesn't want?

henryiii commented 2 weeks ago

Okay, I think it should then a list of true build-types? If the build type is not a recognized build type like "Release", it won't default to true?

Keeping binaries shipped to PyPI small is quite important. Yes, it's possible to get debug info linked in.

ZeroIntensity commented 2 weeks ago

That sounds reasonable to me.

godlygeek commented 2 weeks ago

If removing debug info that was linked into a Release type build is the primary reason for wanting this feature, then it seems like the stripping should happen for Release and MinSizeRel builds, but shouldn't happen for Debug or RelWithDebInfo. I'm not sure what's most reasonable for custom configurations, though... I'm by no means a CMake expert, but: I don't suppose there's some way to introspect whether the build type's CFLAGS include -g? Perhaps there's something clever that could be done, like seeing whether there is DWARF for a function matching PyInit_*.

If there's no way to do something clever or introspective when we don't know whether the user wants debug info or not, my preference would be for larger but debuggable binaries. I'd rather defaults not be destructive.

henryiii commented 2 weeks ago

That's why I was asking about a list of true vs. list a false. Release, MinSizeRel could be strip=True, and all others will be strip=false (but of course you can set it manually if needed)?

godlygeek commented 2 weeks ago

That makes sense to me

leeyuky9802 commented 2 days ago

Exactly... I was trying to figure out why debug info was not added to the binary lib after setting tool.build-type to Debug. And the the g++ build command used by ninja is 100% on point.

I love this tool, but could somebody add a warning that when build-type is Debug strip is enabled by default?

henryiii commented 2 days ago

Actually, I think this likely can be considered a bug and fixed without back-compat in a patch release. Not stripping shouldn't break debug releases, so let's try it.