[pacman-dev] [PATCH v3] makepkg: implement support for stripping kernel modules

[Steven Noonan]

On Mon, Feb 10, 2014 at 4:03 PM, Allan McRae <allan at archlinux.org> wrote:
> On 10/02/14 20:56, Steven Noonan wrote:
>> On Sun, Feb 9, 2014 at 8:32 PM, Allan McRae <allan at archlinux.org> wrote:
>>> On 10/02/14 09:26, Steven Noonan wrote:
>>>> I noticed when I built a kernel package with options 'strip' and 'debug' that
>>>> the debug package was missing the debug information for the kernel modules.
>>>> This patch adds kernel modules to the targets for 'strip' and is compatible
>>>> with the split-out debug information packages.
>>>>
>>>> v2: Correct logic error -- did a 'continue' after the 2nd-level case statement
>>>>     instead of in the default case.
>>>> v3: Oops, forgot to terminate outer case. Ran this version through the test
>>>>     suite and all tests passed.
>>>>
>>>
>>> For future reference, put comments like these ....
>>>
>>>> Signed-off-by: Steven Noonan <steven at uplinklabs.net>
>>>> ---
>>>
>>> ... under this line.  That way they are ignored when committing the patch.
>>
>> I'll bear that in mind, thanks for pointing it out.
>>
>>>
>>>>  scripts/makepkg.sh.in | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
>>>> index 940e947..027fcc3 100644
>>>> --- a/scripts/makepkg.sh.in
>>>> +++ b/scripts/makepkg.sh.in
>>>> @@ -1659,6 +1659,13 @@ tidy_install() {
>>>>                                       strip_flags="$STRIP_SHARED";;
>>>>                               *application/x-archive*)    # Libraries (.a)
>>>>                                       strip_flags="$STRIP_STATIC";;
>>>> +                             *application/x-object*)
>>>> +                                     case "$binary" in
>>>> +                                             *.ko)                   # Kernel module
>>>> +                                                     strip_flags="$STRIP_SHARED";;
>>>> +                                             *)
>>>> +                                                     continue;;
>>>> +                                     esac;;
>>>
>>> This looks fine to me.
>>>
>>> I notice the Arch Linux kernel package uses options=('!strip').  I am
>>> lead to believe that stripping vmlinux lead to issues with building
>>> modules.  Also the Arch PKGBUILD compresses the modules before it gets
>>> to stripping the files.
>>
>> I maintain the Arch Linux EC2 kernel, and I'm doing adventurous things
>> with the packaging to make debugging a bit easier. I am looking at
>> enabling CONFIG_DEBUG_INFO and adding -debug packages. That will
>> indeed require removing the compression stage for the modules. In my
>> experience this has had no negative impact. In fact, tools like 'perf'
>> look for separate debug symbols.
>>
>> As for module builds, you're correct:
>>
>>   MODPOST 1 modules
>> FATAL: vmlinux has no symtab?
>> /usr/src/linux-3.13.2-1-ec2/scripts/Makefile.modpost:90: recipe for
>> target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>>
>> The build still fails even if the -debug packages are installed alongside.
>>
>>
>> I'd argue however that this doesn't impact this patch directly --
>> splitting debug symbols from the kernel modules is OK, but stripping
>> vmlinux is not. This could be worked around by:
>>
>> 1. Moving vmlinux into linux-headers. vmlinux is kind of an oddball
>> (only item in /usr/src/linux for the main package) -- maybe I'm
>> missing context as to why it's in the 'linux' package? it seems like
>> you'd need the -headers package for a module build anyway, so it can't
>> be there just for module builds.
>> 2. Change linux-headers to use options=('!strip'). This also avoids
>> the strange side effect of having a -debug for linux *and*
>> linux-headers.
>>
>> I just tested the above and successfully built a module. So I guess
>> the question is if vmlinux really needs to be in the primary package
>> (and I'd be curious to know why, if so).
>>
>
> That makes sense to me.  Can you create a feature request on the Arch
> bug tracker to implement this.
>
> Anyway - now that I am more clear on the situation, I think your patch
> makes a lot of sense and will pull it.
>

Thanks, Allan! I've submitted the feature request:

https://bugs.archlinux.org/task/38869