- Don't use
 %{summary}.
as description for the libs package. Use something along the lines of the description of the devel package instead.

- Is the target platform stuff really necessary? Won't a simple
 %{cmake_kde4}
 make VERBOSE=1 %{?_smp_mflags}
do?

- Devel package should probably
 Requires:       %{name}-libs = %{version}-%{release}
instead of
 Requires:       %{name} = %{version}-%{release}

- Libs should not
 Requires:       %{name} = %{version}-%{release}
Is there even a need to have a separate package for the library? I'd put them in the main package.

- Devel package must own
 %{_includedir}/csync/
instead of
 %{_includedir}/csync/*
otherwise it will not own the directory.

- Main package must own
 %dir %{_sysconfdir}/csync/
as it places files in that directory

Hi Jussi,

Thanks very much for reviewing this. Sorry it has taken me a little while to respond!

I've updated the spec and SRPM:

http://alexh.fedorapeople.org/reviews/csync/csync.spec
http://alexh.fedorapeople.org/reviews/csync/csync-0.43.0-2.fc11.src.rpm

* Summary for libs - you're right. I've tried to write something better.

* target platform bits - I've had to do a push/pop because csync doesn't allow in-source builds, only out-of-source. I'm not sure the KDE4 cmake macro helps in this regard; https://fedoraproject.org/wiki/SIGs/KDE gives a similar "Best practice" (see bottom of page). The alternative would be to patch the build system, which would be possible, but isn't a system tested by upstream.

* devel requires - yes, you're right again :)

* need for -libs - I plan on packaging the pam-csync module, which uses the libraries rather than the binary, hence the separation.

* directory ownership - again, you're right: sorry, I'm not sure how I missed that.

Whoops, sorry for the delay, somehow this slipped under my radar.

- Use the %{version} macro in Source0.

- You have duplicate ownership of files in %{_sysconfdir}/csync. As you specify the files in that directory, just change
 %{_sysconfdir}/csync
to
 %dir %{_sysconfdir}/csync/
(I recommend using the trailing slash whenever listing a directory, makes the spec file easier to read).

- You might want to put an explicit require on the libs package in the main package, since the license is in -libs.

- There is a test option, so you should use it. You probably need just BR: check(-devel?) and then the section
 %check
 make test

- Is the config used just by the binary or by the libraries too?

****

rpmlint output:
csync-devel.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 0 errors, 1 warnings.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- Change the Source0 line to use %{version}.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK

MUST: Files only listed once in %files listings. NEEDSWORK
- Duplicate ownership as stated above.

MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- Missing AUTHORS and TODO.

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
- Libs package required.

MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK

SHOULD: The package builds in mock. OK
- Only builds in rawhide due to missing iniparser package in <=F11.

ping Alex

ping again

Pong Jussi - I'm so sorry, I missed your previous ping! For some reason, this bug didn't come up as needing attention from me.

I have an updated csync .spec, but I haven't addressed all the issues yet: I should have this done in the next week or so.

Thanks for being so persistent, it's much appreciated.

ping

Hi Jussi,

Apologies for the delay. The updated spec and SRPM are:

http://alexh.fedorapeople.org/reviews/csync/csync.spec
http://alexh.fedorapeople.org/reviews/csync/csync-0.43.0-3.fc11.src.rpm

The main thing which remains unaddressed is the use of the test suite. I've put in place the code to run check, and on my local system this works fine. However, when submitted to koji, it doesn't run. I assume there is something about the test which fails on koji either in what it's trying to do, or its assumptions about how the build works, but I haven't yet been able to work out why. I will keep looking at this though!

Instead of the explicit dep on -libs, I've put COPYING in both packages - I think it's nicer, and rpmlint doesn't like the explicit dep anyway. 

I've also checked that the config isn't used by the library, so I'm sure that's in the right place.

The other problems you raised should now be addressed.

Many, many thanks again for your work!

rpmlint output:
csync-devel.x86_64: W: no-documentation
5 packages and 0 specfiles checked; 0 errors, 1 warnings.

This is OK.

**

NEEDSWORK:

 %{_sysconfdir}/csync/
should be
 %dir %{_sysconfdir}/csync/
since the first version also pulls in everything in the directory. For the sake of clearness, move the %dir statement before the config files.

and

%{_libdir}/csync-0/csync_smb.so
 should be
%{_libdir}/csync-0/

**

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK

MUST: Files only listed once in %files listings. NEEDSWORK
- See above, config files are listed twice.

MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK

MUST: Large documentation files must go in a -doc subpackage. NEEDSWORK
- doc/userguide is 148K, i.e. a rather large portion of the csync package. Move it to -doc.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. NEEDSWORK
- csync requires anyway csync-libs for operation, so you don't need to duplicate COPYING in the main package.

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

I'm also not quite sure about the necessity of a separate -libs package, I'd combine it to the main package.

ping?

ping Alex, are you there?

No answer for over a week, after over a month of prior non-response => this review is officially stalled.

*** This bug has been marked as a duplicate of bug 565902 ***

Ah, I was on vacation actually - but anyhow, I don't have a problem with Andreas taking this package on: I was hoping to use it more than I have ended up to.

*** This bug has been marked as a duplicate of bug 848208 ***