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 program logic for debuginfo enablement #3036

Closed

Conversation

pmatilai
Copy link
Member

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.

Replace the logic parts with actual C code where the logic is more straightforward to follow, taking advantage of dynamic spec generation so debuginfo packages are only ever generated during an actual build.

There's a crazy amount of details buried in such a tiny piece, commented in the code now.

A noteworthy point here is that the presence of the old %install hack now causes an explicit failure, something like:

error: line 4: %package debuginfo: package foo-debuginfo already exists
error: parsing failed

This explicit break is very much intentional as we want distros to remove those %install hacks from their macro files.

Fixes: #2204
Fixes: #1878

@pmatilai pmatilai added RFE packaging Package building, SPEC files, etc. bug and removed RFE labels Apr 15, 2024
@pmatilai
Copy link
Member Author

FWIW I think there's plenty of cleanup possibilities in this area, but for now I just want something to ship in 4.20.

@ffesti
Copy link
Contributor

ffesti commented Apr 15, 2024

While the code looks ok, I wonder if there is a better way to do this? We should be able to add the debuginfo generation into %__spec_install_pre. This was not possible in the past as you could not expand a package definition into the %install section. But now we can just use the same route as here and just drop it into a dynamic .specpart file. We'd basically need the same code as doDebugPackage but as a macro - or probably better - luascript. But we could leave %install as a normal build script without any special treatment in the rpm C code.

As this is about getting things to work for 4.20 I am fine with just keeping this patch as is and have a second run at this later on during 6.0 development.

@ffesti
Copy link
Contributor

ffesti commented Apr 15, 2024

Ok, the longer I look at this from up close the more head ache I get. May be lets leave this as is....

@pmatilai
Copy link
Member Author

pmatilai commented Apr 16, 2024

Oh it's certainly not the best way imaginable, it'd be nice to move this all to a script or something, but ... its complicated. The decision to create debuginfo packages must happen before %install in the spec is parsed, because extracting debuginfo is hooked into %__spec_install_post based on whether %__debug_package is defined or not, so it needs to be present when the main spec is parsed. If that gets defined in the .specpart, it happens too late and the extraction never happens. There are four different debuginfo-specific macros involved in the logic... I considered adding an autogenerated %install -a section for the debuginfo extraction to get rid of that macro, but it'd break packages overriding __spec_install_post for their own stuff (I think eg kernel does that).

After banging my head against this all for a few hours yesterday, I concluded that it's best to have the logic in a single spot where it's "easy" to follow, rather than have it scattered around in macros and spec bits, a little C and whatnot.

I do have this "not quite right" feeling about this so lets leave it to dry out here for a few days and see if the vision clears up 😅

It's a headache for sure.

@pmatilai
Copy link
Member Author

This really needs to be done with %_enable_debug_packages 1 as default in macros.in.
With that, this fails on 332 and 409, so there's at least that much more work to do.

@pmatilai pmatilai marked this pull request as draft April 16, 2024 09:52
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.

Replace the logic parts with actual C code where the logic is more
straightforward to follow, taking advantage of dynamic spec generation
so debuginfo packages are only ever generated during an actual build.

There's a crazy amount of details buried in such a tiny piece, commented
in the code now.

A noteworthy point here is that the presence of the old %install hack now
causes an explicit failure, something like:

    error: line 4: %package debuginfo: package foo-debuginfo already exists
    error: parsing failed

This explicit break is very much intentional as we want distros to remove
those %install hacks from their macro files.

Fixes: rpm-software-management#2204
Fixes: rpm-software-management#1878
@pmatilai
Copy link
Member Author

Now with debug packages enabled by default.

@pmatilai
Copy link
Member Author

Closing in favor of #3085

@pmatilai pmatilai closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug packaging Package building, SPEC files, etc.
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
2 participants