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

[Simo Sorce]

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:'

> For the other patches, should there be a distclean target at the top-level?

It wouldn't be bad, but we can add this in another patchset.

> Otherwise the code looks ok to me but I'm not that familiar with SSSD.

Looking at the code now.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York