Bug 478362

Summary: Review Request: fmirror - Mirror directories from ftp servers
Product: [Fedora] Fedora Reporter: Fabian Affolter <mail>
Component: Package ReviewAssignee: Christoph Wickert <cwickert>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: cwickert, fedora-package-review, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-04-02 18:04:35 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On:    
Bug Blocks: 201449    

Description Fabian Affolter 2008-12-28 18:20:55 EST
Spec URL: http://fab.fedorapeople.org/packages/SRPMS/fmirror.spec
SRPM URL: http://fab.fedorapeople.org/packages/SRPMS/fmirror-0.8.4-1.fc9.src.rpm

fmirror is a C program to mirror a directory tree from a remote ftp
server. It allows regexp (regular expression) pattern matching to
help target files that are to be included and excluded. It uses a
combination of timestamp, file size and file permissions to decide
what files to transfer from the ftp server.

Koji scratch build:

rpmlint output:
[fab@laptop024 i386]$ rpmlint fmirror*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

[fab@laptop024 SRPMS]$ rpmlint fmirror-0.8.4-1.fc9.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 1 Christoph Wickert 2009-01-03 16:35:57 EST
REVIEW FOR deecdd74d33f7ed0cb2cd358c27663c0  fmirror-0.8.4-1.fc9.src.rpm

OK - MUST: $ rpmlint /var/lib/mock/fedora-rawhide-i386/result/fmirror-*
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file matches the base package %{name}, in the format %{name}.spec.
FIX - MUST: The package must meet the Packaging Guidelines, see below for details

OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines (GPLv2+) 
OK - MUST: The License field in the package spec file matches the actual license (GPLv2+)
OK - MUST: The source package includes the text of the license in its own file, and that file is included in %doc.
OK - MUST: The spec file is written in American English.
OK - MUST: The spec file for the package is legible.
OK - MUST: The sources used to build the package match the upstream source, as provided in the spec URL: md5sum 78652ce5bb50e6c120c9ca0988cb9dca
OK - MUST: The package successfully compiles and builds into binary rpms on at least one primary architecture: tested on i386
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires: None, because all build deps are listed in the exceptions section of the Packaging Guidelines.
N/A - MUST: The spec file MUST handle locales properly.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates: None except in docdir
FIX - MUST: The package does not contain duplicate files in the %files listing, but the files listing needs work, see below

OK - MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf %{buildroot}.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included something as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
N/A - MUST: Packages must NOT contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf %{buildroot}.
OK - MUST: All filenames in rpm packages are be valid UTF-8.

N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The package builds in mock.
OK - SHOULD: The package compiles and builds into binary rpms on all supported architectures: tested in koji
OK - SHOULD: The package functions as described
N/A - SHOULD: If scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
N/A - SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. Please see File Dependencies in the Guidelines for further information. 

I'm not happy with the URL tag, but there seems to be no upstream and it's better than nothing.

The timestamp of Source0 does not match, see

Files section: 
I'm not happy with the two folders in doc. Several Options:
1. leave the files in /usr/share/fmirror, but mark the confif files as %config and the README as %doc.
2. if you mark something as %config, it should usually should be in /etc. If it's in there it should better be %config(noreplace)
3. install the config /usr/share/doc/fmirror-%{version} and rename configs/README. Mark them as %doc, not as %config
4. install the config folder as a subfolder of /usr/share/doc/fmirror-%{version}
5. Patch the Makefile for one of the previous options

Option 2:

%configure --datadir=%{_sysconfdir}
make %{?_smp_mflags}

rm -rf %{buildroot}
make install DESTDIR=%{buildroot} INSTALL="install -p"
chmod -x %{buildroot}%{_mandir}/man1/fmirror.1
mv configs/README configs/README.configs
rm -rf %config %{buildroot}%{_sysconfdir}/%{name}/README


%doc ChangeLog COPYING README 
%config(noreplace) %{_sysconfdir}/%{name}/

Option 3:

rm -rf %{buildroot}
make install DESTDIR=%{buildroot} INSTALL="install -p"
chmod -x %{buildroot}%{_mandir}/man1/fmirror.1
# these files are packaged in doc
rm -rf %{buildroot}%{_datadir}/%{name}
mv configs/README configs/README.configs


%doc ChangeLog COPYING README 
%doc configs/*.conf
%doc configs/README.configs

Option 4:

rm -rf %{buildroot}
make install DESTDIR=%{buildroot} INSTALL="install -p"
chmod -x %{buildroot}%{_mandir}/man1/fmirror.1
# these files are packaged in doc
rm -rf %{buildroot}%{_datadir}/%{name}
rm -rf configs/CVS


%doc ChangeLog COPYING README 
%docdir configs/
%doc configs/

- Include a fedora.conf file
- Debian seems to have a couple of interesting patches, see
Comment 2 Fabian Affolter 2009-01-12 08:03:35 EST
Thanks Christoph for your review and your help.  I will incorporate the changes as soon as possible.
Comment 3 Christoph Wickert 2009-11-22 22:47:26 EST
Please let me know if you are still interested in maintaining this package.
Comment 4 Christoph Wickert 2010-04-02 18:04:35 EDT
This bug was in NEEDINFO state for months now. I will close it now as per