[Freeipa-devel] [PATCHES] Support for RPM generation in SSSD

[Rob Crittenden]

Simo Sorce wrote:
> Adding to rob comments.
> 
> On Thu, 2009-02-12 at 09:24 -0500, Rob Crittenden wrote:
>> Stephen Gallagher wrote:
>>> These three patches provide beginning support for generating RPMs from
>>> the SSSD source tree. They are based on the Samba/LDB spec files. I also
>>> had to make a few modifications to our build tree to allow this.
>>>
>> With my rpm-reviewer hat on:
>>
>> - Just a nit, but you have a variable named 'tarball_name' that doesn't 
>> contain the .tar extension :-) Does it really provide clarity to have 
>> these separate variables?
> 
> Why do we have the pre_release thing at all, given we are using 0.9x we
> probably do not need that.
> 
>> - You probably don't need explicit library Requires: libtalloc, 
>> libtevent, etc. rpm should add those.
> 
> At the same time why are libtevent-devel and libldb-devel commented ?
> 
>> - I don't think you need/want separate Version/Release for subpackages.
> 
> I am not sure why we have subpackages at all, what do we gain from
> shipping infopipe and polkit in separate packages?
> 
>> - For the infopipe package do you need the -n 'sssd-' part?
>> - In %setup it looks like tarball_name just mirrors other variables. I 
>> suspect that plaint setup -q would work.
> 
> I think so too.
> 
>> - For all the %post/%postun I think it is recommended to have that on a 
>> single like like: % post -p /sbin/ldconfig
>> - You should add: Requires(post): /sbin/ldconfig
>> - For the configure call I'd replace /etc with %{_sysconfdir} and /usr 
>> with %{_usr}
>> - There is no changelog
> 
> Agree on the rest, I am also unclear about the complex path created in
> 'Buildroot:'

The default one from rpmdev-newspec is probably preferred:

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

rob