Skip to content
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

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

Open
hroncok opened this issue Sep 27, 2024 · 6 comments
Open

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

hroncok opened this issue Sep 27, 2024 · 6 comments

Comments

@hroncok
Copy link

hroncok commented Sep 27, 2024

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 #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:

  • The default build type in scikit-build-core appears to be Release.
  • When it is Release, the built extension modules are stripped of debuginfo (and RPM builds in Fedora fail).
  • Manually setting the build type to RelWithDebInfo fixes the problem.

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:

Or if it needs to be handled similarly to

if self.settings.install.strip is None:
self.settings.install.strip = (
install_policy
and self.settings.cmake.build_type in {"Release", "MinSizeRel"}
)

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

@hroncok hroncok changed the title RFE: Set the default Release in RPM builds to RelWithDebInfo RFE: Set the default build type in RPM builds to RelWithDebInfo Sep 27, 2024
@LecrisUT
Copy link
Collaborator

LecrisUT commented Sep 27, 2024

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
Copy link
Author

hroncok commented Sep 27, 2024

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

LecrisUT commented Sep 27, 2024

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
Copy link
Author

hroncok commented Sep 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants