New member, package review, questions about template .spec

[Philip Prindeville]

On 1/23/12 4:54 PM, Ken Dreyer wrote:
> On Mon, Jan 23, 2012 at 12:45 AM, Philip Prindeville
> <philipp_subx at redfish-solutions.com> wrote:
>> I've been developing packages for 6 or 7 years now, and I'm a contributor to Fedora (and occasionally EPEL).
> 
> Welcome to the RPM Fusion dev community!
> 
>> Anyway, so I've taken their driver and repackaged it for rpmfusion.  My first cut is here:
>>
>> ftp://ftp.redfish-solutions.com/pub/perle-serial-kmod-3.8.0-6.1.fc16.1.src.rpm
> 
> Thank you for your interest in contributing. Would you mind signing up
> for bugzilla.rpmfusion.org, and submitting a new review request? We
> can continue the review for your package there.  The steps for setting
> up a new review bug are almost identical to Fedora. The instructions
> are at http://rpmfusion.org/Contributors, under "3.2. Create a package
> review request".

I'll do that as soon as my bugzilla access is approved.

> 
>> Anyway, I noticed the following things I had to change to the .spec file I cut and pasted from the Wiki to get it to work:
>>
>> (1) added the lines
>>
>> %define repo   rpmfusion
>> %define _name  foo
>>
>> as %{repo} is undefined otherwise. This might be a glitch in my installing the rpmfusion stuff improperly.
> 
> Yeah, I think this may be an anachronism from the time when several
> repos were merging together. I see some packages have just hardcoded
> "--repo rpmfusion" in the kmodtool invocation.
> 
> Anyone else have opinions on this? Should "rpmfusion" be hardcoded in
> the kmodtool invocation, or should we use a %define at the top of each
> file? My vote is for hardcoding, because it's less to read.
>
> 
>> (2) modified the line
>>
>> Release:        1{?dist}.1
>>
>> there's a missing "%" before the "{".
> 
> Yes, this should be changed on the wiki. Done, in
> http://rpmfusion.org/Packaging/KernelModules/Kmods2?action=recall&rev=19
> 
>> Name:          %{_name}-kmod
> 
> I don't see the string "_name" anywhere on the wiki page, so I'm not
> sure about this one... I see openafs uses "%define kmod_name openafs",
> and other packages just hardcode the shortname everywhere. I think
> it's probably up to you what you want to do for the Package's
> "shortname".


Yeah, although if we end up using a template that people just cut and paste or if we use rpmdevtools support, then having:

%define _name	foo
...
Name:		%{_name}-kmod
...

etc. just means a single edit and less chance of missing a spot where it needs to be tweaked.


> 
> 
>> (3) modified the lines:
>>
>> # needed for plague to make sure it builds for i586 and i686
>> ExclusiveArch:  i586 i686 x86_64 ppc ppc64
>>
>> not sure why "ppc" and "ppc64" are included... that seems to contradict the accompanying comment (which probably should mention x86_64 but doesn't).
> 
> Yeah, I'm not a Plague expert, so I am not sure why this is needed, or
> if it is still necessary.
> 
> 
>> (4) added the lines:
>>
>> %package common
> 
> So the issue here is that "-common" is supposed to be provided by the
> userland package. The -kmod package doesn't build or provide this at
> all. See openafs.spec for an example:

Right. I guess I don't understand why both can't be built simultaneously, especially if they share the same %{SOURCE0} (as many of them do).

There's no reason why you couldn't or shouldn't if circumstances permit, right?

> 55 	Provides: %{name}-kmod-common = %{version}
> 
> Maybe openafs isn't the best example here, since that builds a few
> different userland subpackages. open-vm-tools.spec is another example:
> 
> 36 	Provides: open-vm-tools-kmod-common = %{version}
> 
> Basically the main userland package should "Provide" the -common package.

I get that, but since a single .spec file can build a package and zero or more subpackages... why not do this?

> 
>> (5) commented out this line:
> 
> No need to comment that out that I can see.

No _need_ to...  just wanted to cut down chatter when looking through build logs for warnings, etc.

> 
>> (6) modified the loop:
>>
>> for kernel_version in %{?kernel_versions} ; do
>>    cp -a foo-%{version} _kmod_build_${kernel_version%%___*}
>> done
>>
>> to use %{_name} instead of "foo".
> 
> As mentioned above, as far as I know that's a style thing, up to you.
> 
> 
>> (7) modified the loop:
>>
>> for kernel_version in %{?kernel_versions}; do
>>    make install DESTDIR=${RPM_BUILD_ROOT} KMODPATH=%{kmodinstdir_prefix}/${kernel_version%%___*}/%{kmodinstdir_postfix}
>>    # install -D -m 755 _kmod_build_${kernel_version%%___*}/foo/foo.ko  ${RPM_BUILD_ROOT}%{kmodinstdir_prefix}/${kernel_version%%___*}/%{kmodinstdir_postfix}/foo.kmod
>> done
>>
>> this shouldn't install into .../foo.kmod but into ".../foo.ko" instead.
> 
> Good catch; I've fixed this in
> http://rpmfusion.org/Packaging/KernelModules/Kmods2?action=recall&rev=20.
> 
> 
>> (8) added the lines to the %install section:
>>
>> mkdir -p ${RPM_BUILD_ROOT}%{_sysconfdir}/modprobe.d/
>> %{__cp} -p %{SOURCE1} ${RPM_BUILD_ROOT}%{_sysconfdir}/modprobe.d/%{_name}.conf
> 
> That is appropriate for your package, but not all packages will need
> to do this. For example OpenAFS loads the module in the init script.
> You can feel free to add a section to the Kmods2 wiki page describing
> this technique, though.
> 
> - Ken

Well, right, and I figured it could be included but commented out... the way the patch example was done.

-Philip