Bug 503136 - Review Request: csync - a file synchroniser utility
Review Request: csync - a file synchroniser utility
Status: CLOSED DUPLICATE of bug 848208
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Extras Quality Assurance
http://www.csync.org/
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2009-05-29 03:19 EDT by Alex Hudson (Fedora Address)
Modified: 2012-08-14 18:17 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-03-06 14:14:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑review-


Attachments (Terms of Use)

  None (edit)
Description Alex Hudson (Fedora Address) 2009-05-29 03:19:47 EDT
Spec URL: http://alexh.fedorapeople.org/reviews/csync/csync.spec
SRPM URL: http://alexh.fedorapeople.org/reviews/csync/csync-0.43.0-1.fc11.src.rpm
Description: 
csync is a lightweight utility to synchronize files between two directories,
potentially on different computers.

It synchronizes bidirectionally and allows the user to keep two copies of
files and directories in sync. csync uses widely adopted protocols, such
as smb or sftp, so that there is no need for a server
Comment 1 Susi Lehtola 2009-05-29 16:12:53 EDT
- 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
Comment 2 Alex Hudson (Fedora Address) 2009-06-15 14:17:20 EDT
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.
Comment 3 Susi Lehtola 2009-07-05 09:52:09 EDT
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.
Comment 4 Susi Lehtola 2009-07-22 08:37:12 EDT
ping Alex
Comment 5 Susi Lehtola 2009-08-16 05:11:55 EDT
ping again
Comment 6 Alex Hudson (Fedora Address) 2009-08-17 02:20:17 EDT
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.
Comment 7 Susi Lehtola 2009-09-07 11:18:33 EDT
ping
Comment 8 Alex Hudson (Fedora Address) 2009-09-12 08:51:09 EDT
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!
Comment 9 Susi Lehtola 2009-09-14 08:34:58 EDT
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
Comment 10 Susi Lehtola 2009-09-14 08:35:45 EDT
I'm also not quite sure about the necessity of a separate -libs package, I'd combine it to the main package.
Comment 11 Susi Lehtola 2010-01-01 17:56:45 EST
ping?
Comment 12 Susi Lehtola 2010-02-17 17:39:38 EST
ping Alex, are you there?
Comment 13 Kevin Kofler 2010-03-06 14:14:55 EST
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 ***
Comment 14 Alex Hudson (Fedora Address) 2010-03-10 14:11:50 EST
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.
Comment 15 Kevin Kofler 2012-08-14 18:17:46 EDT

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

Note You need to log in before you can comment on or make changes to this bug.