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

Add proper logic for debuginfo enablement #3085

Merged
merged 1 commit into from
May 13, 2024

Conversation

pmatilai
Copy link
Member

@pmatilai pmatilai commented May 10, 2024

All these years, enabling debuginfo has required distros to hijack the spec %install section with a macro like this:

%install %{?_enable_debug_packages:%{?buildsubdir:%{debug_package}}}\
%%install\
%{nil}

This for a widely used, longtime upstream supported feature is just gross, and also very non-obvious, feeble and whatnot. And totally prevents the new append/prepend options from being used with %install.

Take advantage of several newish features to make this happen: we need expressions to properly handle the numeric %_enable_debug_packages value from a macro, and if enabled, output the debuginfo template as a dynamic .specpart.

Enable debuginfo on Linux by default in the platform configuration. Notably noarch should not have debuginfo so it's disabled in the platform configuration - since 96467dc we can now actually rely on the platform configuration being valid, so we can drop the "%ifnarch noarch" from the debug package check. Further streamlining should be possible.

specstep.spec needs to be made noarch here, otherwise it'll now try to produce debuginfo and fail.

Fixes: #2204
Fixes: #1878

@pmatilai
Copy link
Member Author

The first two commits are simply #3084 which is needed for this to work.

@pmatilai
Copy link
Member Author

It'd probably be possible to further drop the __debug_package logic and merge __debug_install_post into this, and maybe this should be in couple of separate commits, just wanted to get this out before the weekend 😅

Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot cleaner and I like this setup.

@pmatilai
Copy link
Member Author

Yeah it's nice to be able to use the dynamic spec stuff for our own purposes.

To clarify, #3084 is only needed for the "rpmbuild %caps" test which intentionally does not use %setup to test that case, and whose debuginfo now gets generated. So that's basically another bug fixed / limitation eliminated, I could've sworn we have a ticket on debuginfo requiring %setup but can't find it.

I haven't tested but it may well cure #3042 too because the combo means it no longer requires the full %setup thing to run.

@pmatilai
Copy link
Member Author

Mentioned the %setup-less debuginfo case and added an explicit sub-test for it.

@pmatilai
Copy link
Member Author

Had a brief look at killing %__debug_package, but that gets more complicated than it should (and who's surprised?) So many packages rely on redefining %debug_package to %{nil} for disabling debuginfo generation that we just have to preserve that for the time being, and having %debug_package define something else as a side-effect is an easy (if ugly) way to handle this. Not worth messing with just now.

@ffesti
Copy link
Contributor

ffesti commented May 13, 2024

This is even cleaner than my own variant. Great so see we got this to the point where it can be done this cleanly.

@ffesti ffesti added the highlight Release highlight label May 13, 2024
@ffesti
Copy link
Contributor

ffesti commented May 13, 2024

Release notes need to mention this and that distribution need to drop the

%install %{?_enable_debug_packages:%{?buildsubdir:%{debug_package}}}\
%%install\
%{nil}

hack in *-rpm-config. So this probably needs to go into the "Compatibility" section at least in parts.

All these years, enabling debuginfo has required distros to hijack the
spec %install section with a macro like this:

    %install %{?_enable_debug_packages:%{?buildsubdir:%{debug_package}}}\
    %%install\
    %{nil}

This for a widely used, longtime upstream supported feature is just
gross, and also very non-obvious, feeble and whatnot. And totally
prevents the new append/prepend options from being used with %install.

Take advantage of several newish features to make this happen: we need
expressions to properly handle the numeric %_enable_debug_packages value
from a macro, and if enabled, output the debuginfo template as a dynamic
.specpart.

Enable debuginfo on Linux by default in the platform configuration.
Notably noarch should not have debuginfo so it's disabled in the
platform configuration - since 96467dc
we can now actually rely on the platform configuration being valid,
so we can drop the "%ifnarch noarch" from the debug package check.
Further streamlining should be possible.

Note that the old %install hack MUST BE REMOVED from distros now.

As a nice bonus, this makes debuginfo work for packages that don't use
%setup. Add an explicit test for this in the "rpmbuild %caps" test.
specstep.spec needs to be made noarch here, otherwise it'll now try
to produce debuginfo and fail.

Co-authored-by: Florian Festi <ffesti@redhat.com>

Fixes: rpm-software-management#2204
Fixes: rpm-software-management#1878
@ffesti ffesti merged commit 8535694 into rpm-software-management:master May 13, 2024
1 check passed
@pmatilai
Copy link
Member Author

Just noting it here that since distros are widely overriding %__spec_install_post, they'll need to update that to match the new logic in there. @ffesti's version placed the %_enable_debuginfo_packages test into %__spec_install_pre which isn't as widely overridden and so would be more backwards compatible, but then it wouldn't be all in one place. It's a hugely annoying mess 😒

@dmnks
Copy link
Contributor

dmnks commented May 20, 2024

Just noting it here that since distros are widely overriding %__spec_install_post, they'll need to update that ...

I suppose this was obsoleted by #3098, correct?

@pmatilai
Copy link
Member Author

Oh, yup. I thought I made a further comment here but apparently didn't...

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

Successfully merging this pull request may close these issues.

Properly upstream debuginfo enablement rpmspec does not expect debuginfo rpm for a subpackage
4 participants