[LU-4014] Control building separate 'debuginfo' rpm with configure option to Lustre

• Status: Resolved
• Project: Lustre
• Type: Bug
• Priority: Minor
• Reporter: Patrick Farrell (Inactive)
• Assignee: Dmitry Eremin (Inactive)
• Resolution: Won't Fix
• Labels: patch
• Severity: 3

Description

Cray recently began building some of our Lustre code on CentOS build servers, rather than on SLES. When we did this, we discovered an important difference in RPM macro defaults between the distributions:

On SLES, by default, no debuginfo RPM is created, and the brp-strip macro isn't run. If either the debuginfo RPM is created or the brp-strip macro is run, many/most of the debug symbols are stripped from the .ko files built by Lustre.

On CentOS by default, a debuginfo RPM is created, and the brp-strip macro is run. This means that if you want the debug symbols to be in the various .ko files, you must disable creation of a debuginfo RPM AND not run the brp-strip macro.

Overriding the RPM macros can be done from the spec file, ~/.rpmmacros, or
/etc/rpm/macros .

In particular, you need the following in any of the listed locations:

%global debug_package %

{nil}
%global __os_install_post /usr/lib/rpm/brp-compress %{nil}

With this, you will not get a debuginfo RPM and the brp-strip macro will not be run. (brp-strip is called from __os_install_post)

Similarly, you should be able to enable them on systems where they're disabled by default, such as SLES.

I've written a patch that puts these in the spec file and lets you control them with configure options, but I have a few questions:

1) In your opinion, is this an appropriate thing to allow Lustre to configure? The specific concern is overriding the system RPM macros that may be configured somewhere else. Since it's just for Lustre, I think it's OK.

2) My current patch has separate options to enable or disable the debuginfo RPM. (--with-enable-separate-debuginfo, --with-disable-separate-debuginfo), and if neither is provided, you'll get the system RPM macro defaults.

Is this how we want to do it? Should we have separate options for debuginfo and stripping? (Enabling debuginfo effectively enables stripping, even though it doesn't directly call brp-strip)

I think it's reasonable to assume that anyone who's interested in configuring this would either want a separate debuginfo RPM or would like the symbols left in the KO files, IE, non-stripped.

Thoughts?

Comments

[Patrick Farrell (Inactive)]

Patch up at:
http://review.whamcloud.com/#/c/7779/

[Patrick Farrell (Inactive)]

One known issue:
As noted in the patch, I haven't found a distribution independent way to set up the debug_package macro to force the debuginfo RPM to be built. On CentOS, simply not setting the debug_package macro to nil is sufficient, but I assume on SLES, more would be needed.

FX, this is the debug_package macro from /usr/lib/rpm/macros on CentOS:

1. Template for debug information sub-package.
   %debug_package \
   %ifnarch noarch\
   %global __debug_package 1\
   %package debug\
   Summary: Debug information for package % {name}\
   Group: Development/Debug\
   AutoReqProv: 0\
   %description debug\
   This package provides debug information for package %{name}

   .\
   Debug information is useful when developing applications that use this\
   package or when debugging this package.\
   %files debug -f debugfiles.list\
   %defattr(-,root,root)\
   %endif\
   %

   {nil}
   
   When I try to put this in the spec file:
   %global debug_package \
   [Etc...]
   %{nil}

The block of text generated from that macro is somehow appended to the make arguments, rather than causing a debuginfo package to be created.

I also don't have a SLES build server available to me for testing, which makes this tricky to validate.

[Oleg Drokin]

we need to be careful here not to break debug symbols generation for rhel RPMs, so whoever gets assigned here please make sure all is fine in this area.

[Christopher Morrone]

You seem to want an option to make it easy to break the standard packing behavior. In other words, it is standard practice to strip the debug symbols and put them in a separate package with a name ending in -debuginfo, but you seem to want to break that.

It is not clear to me why we would want to condone such practice in the upstream tree.

[Patrick Farrell (Inactive)]

Christopher,

This is a valid concern. We're using a version of this internally because we build Lustre in many situations on build servers that are used to build other things as well, which require different RPM settings vis-a-vis debuginfo. As things are upstream, we have to modify those settings before building Lustre.

If you feel changing that every time Lustre is built is the domain of administering a build server - rather than the Lustre build configuration itself - that's justifiable.

Separately, to respond to your comment in the review (It's a bit easier to discuss in a longer format):
I felt that if we added an option to disable separate debug info, we should then add an option to enable it. That's a lot trickier, and it may be that even if we want to add an option for disabling the separate debug info, we don't want to/can't add one for enabling it.

In retrospect, it would be much cleaner to have a single option to enable or disable separate debug info, but that's only viable if we can enable separate debuginfo successfully on an arbitrary build system.

Cray's needs here would be served quite happily by only an option to disable separate debug info, or, as noted, we can handle this separately - either by carrying a version of this locally or by putting a wrapper around the Lustre build process to change (and change back) the rpmmacros on the build server - if Intel feels that's the appropriate choice.

[Christopher Morrone]

The autoconf configure script is a way to test and modify the way that the code is compiled. While we do have the lustre build system feed information to the spec file in places, we do this because the spec file needs adapt its packing behavior do to those changes. If it doesn't need to adapt its behavior, it doesn't need to know about the configure option.

On the flip side, you want to add a configure option that makes no changes to the compilation of lustre. The only purpose for the configure option is to later pass it on to the packing system.

Which makes me wonder why you don't just tell the packing system the option directly.

A good build system for rpms would build the binary rpms from the .src.rpm. If you are doing that, you should just be able to pass options on the "rpmbuild --rebuild" command line right to the lustre spec file to make the changes you want. Lustre's autoconf system need not have any knowledge of the debuginfo change that you want to make.

If your build system doesn't have that flexibility, then I can understand if you need to carry a local patch to work with what you've got.

But I'm not yet seeing the general applicability of this change. And if anything, I worry about the way the patch changes the currently working automatic debuginfo package generation.

[Patrick Farrell (Inactive)]

I see your point there about configure vs. packing - I hadn't been thinking of them as separate steps.

The reason we don't tell the packing system the option directly is that in this instance (for a particular usage of Lustre internally), we were using the upstream provided Makefile to do 'make rpms'. In that case, we're not invoking the packing stage separately.

We also have a full build system, and in that case, we can and do control the packing options as needed. (Why we use both that and sometimes the upstream Makefile process is a good question, but there's no good answer.)

Thank you for breaking this down - You've convinced me the patch isn't appropriate, and that the functionality it provides isn't useful enough to justify it. Internally, we're still going to carry a spec file change that overrides the RPM macros responsible for generating debug info, but that's a local preference, and can and should be handled accordingly.

I'm going to abandon the patch, please close out the ticket unless you have other concerns.