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
242 stars 50 forks source link

RFE: Set the default build type in RPM builds to RelWithDebInfo #915

Open hroncok opened 1 month ago

hroncok commented 1 month ago

Hello.

Recently, with @hrnciar we discovered the Fedora RPM build of rapidfuzz fails because the debuginfo could not be found.

I've searched why that is possible and found some information in https://github.com/scikit-build/scikit-build-core/issues/875

In Fedora, we found our own CFLAGS (and CXXFLAGS) and they include (amongst others) -O2 and -g. I don't know exactly if the build type as explained in https://stackoverflow.com/a/59314670/1839451 overrides those flags or not, but I know at least this:

Hence I was wondering if it was acceptable for you to set the default build type to RelWithDebInfo when RPM build is detected. I completely understand if this is not a territory you want to go to -- we can certainly patch our RPM package of scikit-build-core to do this if you are not interested.

The detection should be possible to do like this:

    build_type: str = "Release" if "RPM_BUILD_ROOT" not in os.environ else "RelWithDebInfo"

I don't know if this can be here:

https://github.com/scikit-build/scikit-build-core/blob/f0ae31922ff802b179fce3327a9c93ff65934a23/src/scikit_build_core/settings/skbuild_model.py#L64

Or if it needs to be handled similarly to

https://github.com/scikit-build/scikit-build-core/blob/8380c9e613e9ee06bf28ff3897b80f502e4ca6c8/src/scikit_build_core/settings/skbuild_read_settings.py#L230-L234

Let me know if something like this would be acceptable to you. Thanks.

LecrisUT commented 1 month ago

I think we need to update in Fedora to 0.10.6 at the very least to fix stripping the debug symbols (nvm, I've already done that for rawhide), but after that, maybe we need to set the config settings for cmake.build-type in %pyproject_wheel or we export it as an environment variable. Would there be a way to inject that within the python-scikit-build-core.spec (or other packaged files)?

Alternatively we could check for if we are building within a RPM environment and switch the default, but that seems quite an xz move. Any suggestion on how to best detect that we are in an RPM environment?

hroncok commented 1 month ago

I think we need to update in Fedora to 0.10.6 at the very least to fix stripping the debug symbols...

This happens with 0.10.7.

Any suggestion on how to best detect that we are in an RPM environment?

Yes, see the report above:

build_type: str = "Release" if "RPM_BUILD_ROOT" not in os.environ else "RelWithDebInfo"
LecrisUT commented 1 month ago

@henryiii what do you think, should we make a function in_distro or such to detect rpm/deb environments and set the default build type accordingly? For reference the %cmake macro that is commonly used does not set a default build type, but it relies on C_FLAGS + CMAKE_INSTALL_DO_STRIP. There are no other default environment variables that we can rely on here.

One slight annoyance is that if you enter mock --shell to debug you also need to set such environment variables and it is not evident that it would depend on that. On the other hand, this would only affect symbol stripping right now so it is not so crucial.

Gentoo are considering adding cmake.verbose setting to all scikit-build-core backends #912, and I think that would be quite a useful default to set for Fedora as well, but I don't know of an approach to do that other than populating for all python builds which seems ill-advised.

henryiii commented 1 month ago

What about setting SKBUILD_CMAKE_BUILD_TYPE to "RelWithDebInfo", or setting SKBUILD_INSTALL_STRIP to "false"?

We could change the default for RPM_BUILD_ROOT (we have some customizations for conda-build, too, IIRC), but as a first thought if the scikit-build-core recipe on fedora could export an environment variable, that would work. Don't know if it's possible, though?

LecrisUT commented 1 month ago

fedora could export an environment variable, that would work. Don't know if it's possible, though?

That would be my preference as well, but it is a tricky one. @hroncok would it work if python-scikit-build-core provided macros and we would use a %scikit_build_wheel to wrap %pyproject_wheel at the build stage? Specifically does it work if the macro files are not in BuildRequires initially but are populated after %pyproject_buildrequires? That would give more control if we need to change stuff, e.g. there was a recent change of build.verbose -> cmake.verbose which Gentoo found it can be breaking to set depending on version.

Maybe it's not a preferred approach regarding python ecosystem homogeneity, but I think it is warranted because we might want to make sure some %cmake flags are included properly. Other mixed language might benefit from it, or specialized ecosystems like ansible-collections or jupyter.

hroncok commented 1 month ago

fedora could export an environment variable, that would work. Don't know if it's possible, though?

That would be my preference as well, but it is a tricky one. @hroncok would it work if python-scikit-build-core provided macros and we would use a %scikit_build_wheel to wrap %pyproject_wheel at the build stage?

It would but it goes against the fundamental principle of %pyproject_wheel -- that it is build-backend agnostic.

Specifically does it work if the macro files are not in BuildRequires initially but are populated after %pyproject_buildrequires?

Yes. This is exactly how it works e.g. in Fedora ELN/CentOS Stream, where even %pyproject_wheel definition is installed during the first round of %pyproject_buildrequires.

Maybe it's not a preferred approach regarding python ecosystem homogeneity, but I think it is warranted because we might want to make sure some %cmake flags are included properly. Other mixed language might benefit from it, or specialized ecosystems like ansible-collections or jupyter.

Well, we might engineer a solution where individual build-backends packaged in Fedora can install a shell snippet to a well-defined location (containing environment variables) and the pyproject macros would attempt to source/%load/etc. this file when calling %pyproject_wheel. This is probably out of the scope of this upstream ticket.