Bug 1018092 - Review Request: barman - Backup and Recovery Manager for PostgreSQL
Review Request: barman - Backup and Recovery Manager for PostgreSQL
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Peter Robinson
Fedora Extras Quality Assurance
:
Depends On: 1018090
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-11 04:14 EDT by Dale Macartney
Modified: 2015-01-09 08:07 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-02-08 05:54:02 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pbrobinson: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dale Macartney 2013-10-11 04:14:53 EDT
SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-1.fc19.src.rpm

Description: 
Barman (backup and recovery manager) is an administration
tool for disaster recovery of PostgreSQL servers written in Python.
It allows to perform remote backups of multiple servers
in business critical environments and help DBAs during the recovery phase.
Barman's most wanted features include backup catalogs, retention policies,
remote recovery, archiving and compression of WAL files and backups.
Barman is written and maintained by PostgreSQL professionals 2ndQuadrant.
Comment 2 Christopher Meng 2013-10-16 07:22:37 EDT
1. %if 0%{?rhel} == 6
%global pybasever 2.6
%else 
%global pybasever 2.7
%endif

%{!?pybasever: %define pybasever %(%{__python} -c "import sys;print(sys.version[0:3])")}

--------

I don't know if you can have a look at rpm -E %python[TAB][TAB].

2. No need to write :

%{!?python_sitelib: %define python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib()")}
%{!?python_sitearch: %define python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print get_python_lib(1)")}

python2-devel is enough. Also missing python-setuptools.

3. Remove BuildRoot:/rm -fr $RPM_BUILD_ROOT/%clean/%defattr(-,root,root)

4. Shouldn't ship INSTALL file.

5. %{__python}-->%{__python2}
%{python_sitelib}-->%{python2_sitelib}

6. Never mark manpages as %doc.

7. %config %{_sysconfdir}/bash_completion.d/

This dir is not owned by this package, please fulfill the path.

%config %{_sysconfdir}/bash_completion.d/barman

8. /var --> %_localstatedir

9. Put %pre before %changelog.

10. I never use EOF in RPM, I always use Source1,2... to add them(optional)

11. %setup -n barman-%{version} -q

Just 

%setup -q is OK.
Comment 3 Dale Macartney 2013-10-16 09:08:20 EDT
Thanks Christopher, feedback is greatly appreciated. 

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-3.fc19.src.rpm

Points 8 and 9 aren't too clear though if you wouldn't mind clarifying. 

package builds locally with rpmbuild without error, however fails when doing a koji scratch build. 

sh: /usr/bin/python: No such file or directory

http://koji.fedoraproject.org/koji/taskinfo?taskID=6065150
Comment 5 Peter Robinson 2013-10-16 16:02:09 EDT
> 3. Remove BuildRoot:/rm -fr $RPM_BUILD_ROOT/%clean/%defattr(-,root,root)

Those bits are needed if you wish to support EL6 using a single spec file
Comment 6 Ken Dreyer 2013-10-16 16:07:51 EDT
(In reply to Peter Robinson from comment #5)
> > 3. Remove BuildRoot:/rm -fr $RPM_BUILD_ROOT/%clean/%defattr(-,root,root)
> 
> Those bits are needed if you wish to support EL6 using a single spec file

EL6 or EL5?
Comment 7 Dale Macartney 2013-10-16 16:14:03 EDT
items put back in spec to cover all bases! 

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-4.fc19.src.rpm
Comment 8 Christopher Meng 2013-10-16 19:45:17 EDT
(In reply to Peter Robinson from comment #5)
> > 3. Remove BuildRoot:/rm -fr $RPM_BUILD_ROOT/%clean/%defattr(-,root,root)
> 
> Those bits are needed if you wish to support EL6 using a single spec file

Who taught you this? 

These lines can be safely dropped since Fedora 10, I assume you were talking about EL5.

But I don't think we really need to support EL5
Comment 9 Volker Fröhlich 2013-11-25 04:54:01 EST
rm -fr $RPM_BUILD_ROOT/%clean are necessary for EL5. https://fedoraproject.org/wiki/EPEL:Packaging?rd=Packaging:EPEL#Cleaning_BuildRoot_in_.25clean

defattr only for EL4. That was found on a mailing list discussion.

>But I don't think we really need to support EL5

That's really the packager's choice.
Comment 10 Dale Macartney 2014-01-13 08:36:11 EST
As Fedora 18 goes EOL tomorrow, I'd like to maintain F19 and EL6 and newer. I've removed those older options for EL5. 

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-5.fc19.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=6395843
Comment 11 Dale Macartney 2014-01-13 08:52:15 EST
Sorry... incorrect file name. 

SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-5.fc20.src.rpm
Comment 12 Peter Robinson 2014-01-13 09:36:30 EST
Right, it's mostly there. The only things I think that need to be addressed are:
- output of rpmlink below. I think some of those are spurious.
- The license I believe (according to licensecheck at least) should be GPLv3+
- Did you check the Users/Groups guidelines for the barman user?
http://fedoraproject.org/wiki/Packaging:UsersAndGroups

- rpmlint output

rpmlint barman-1.2.3-5.fc20.src.rpm barman-1.2.3-5.fc20.noarch.rpm
barman.src: W: strange-permission barman.logrotate 0600L
barman.src: W: strange-permission barman-1.2.3.tar.gz 0600L
barman.src: W: strange-permission barman.cron 0600L
barman.src: W: strange-permission barman.spec 0600L
barman.src:11: W: mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 1)
barman.noarch: W: conffile-without-noreplace-flag /etc/bash_completion.d/barman/barman.bash_completion
barman.noarch: W: conffile-without-noreplace-flag /etc/logrotate.d/barman
barman.noarch: W: conffile-without-noreplace-flag /etc/cron.d/barman
barman.noarch: W: non-standard-uid /var/lib/barman barman
barman.noarch: W: non-standard-gid /var/lib/barman barman
barman.noarch: E: non-standard-dir-perm /var/lib/barman 0700L
barman.noarch: W: non-standard-uid /var/log/barman barman
barman.noarch: W: non-standard-gid /var/log/barman barman
barman.noarch: W: non-standard-uid /var/log/barman/barman.log barman
barman.noarch: W: non-standard-gid /var/log/barman/barman.log barman
2 packages and 0 specfiles checked; 1 errors, 14 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
  looks like it should be GPLv3+ 
# licensecheck -r barman-1.2.3/
barman-1.2.3/barman/xlog.py: GPL (v3 or later)
barman-1.2.3/barman/version.py: GPL (v3 or later)
barman-1.2.3/barman/server.py: GPL (v3 or later)
barman-1.2.3/barman/compression.py: GPL (v3 or later)
barman-1.2.3/barman/retention_policies.py: GPL (v3 or later)
barman-1.2.3/barman/backup.py: GPL (v3 or later)
barman-1.2.3/barman/lockfile.py: GPL (v3 or later)
barman-1.2.3/barman/cli.py: GPL (v3 or later)
barman-1.2.3/barman/command_wrappers.py: GPL (v3 or later)
barman-1.2.3/barman/config.py: GPL (v3 or later)
barman-1.2.3/barman/__init__.py: GPL (v3 or later)
barman-1.2.3/setup.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/xlog.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/version.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/server.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/compression.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/retention_policies.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/backup.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/lockfile.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/cli.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/command_wrappers.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/config.py: GPL (v3 or later)
barman-1.2.3/build/lib/barman/__init__.py: GPL (v3 or later)

+ license matches the actual package license
+ latest version packaged
+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm
  5348cf3ea707b43f8a98061d0d257f2d  barman-1.2.3.tar.gz
+ package successfully builds on at least one architecture
  tested using koji scratch build
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and %postun+ does not use Prefix: /usr
n/a package owns all directories it creates
n/a no duplicate files in %files
+ Package perserves timestamps on install
+ Permissions on files must be set properly 
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package runtime 
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream to include it
n/a translations of description and summary for non-English languages, if available
+ reviewer should build the package in mock/koji
n/a the package should build into binary RPMs on all supported architectures
n/a review should test the package functions as described
+ scriptlets should be sane
n/a non -devel packages should require fully versioned base
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /usr/bin or /usr/sbin
+ Package should have man files
Comment 13 Dale Macartney 2014-01-14 16:27:33 EST
license type corrected and rpmlint errors resolved. 

[mac@rhodey ~]$ rpmlint rpmbuild/SPECS/barman.spec rpmbuild/SRPMS/barman-1.2.3-7.fc20.src.rpm
barman.src: W: strange-permission barman.logrotate 0600L
barman.src: W: strange-permission barman-1.2.3.tar.gz 0600L
barman.src: W: strange-permission barman.cron 0600L
barman.src: W: strange-permission barman.spec 0600L
1 packages and 1 specfiles checked; 0 errors, 4 warnings.
[mac@rhodey ~]$ 

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-7.fc20.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=6405897
Comment 14 Dale Macartney 2014-01-15 13:38:01 EST
chmod 644 rpmbuild/SOURCES/barman* seems to resolve the rpmlint errors. 

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-8.fc20.src.rpm

[mac@rhodey ~]$ rpmlint rpmbuild/SPECS/barman.spec rpmbuild/SRPMS/barman-1.2.3-8.fc20.src.rpm
1 packages and 1 specfiles checked; 0 errors, 0 warnings.
[mac@rhodey ~]$ 

http://koji.fedoraproject.org/koji/taskinfo?taskID=6411104
Comment 15 Dale Macartney 2014-01-15 14:08:25 EST
Forgot change log. 

[mac@rhodey ~]$ rpmlint rpmbuild/SPECS/barman.spec rpmbuild/SRPMS/barman-1.2.3-9.fc20.src.rpm rpmbuild/RPMS/noarch/barman-1.2.3-9.fc20.noarch.rpm 
barman.noarch: W: non-standard-uid /var/lib/barman barman
barman.noarch: W: non-standard-gid /var/lib/barman barman
barman.noarch: W: non-standard-uid /var/log/barman barman
barman.noarch: W: non-standard-gid /var/log/barman barman
barman.noarch: W: non-standard-uid /var/log/barman/barman.log barman
barman.noarch: W: non-standard-gid /var/log/barman/barman.log barman
2 packages and 1 specfiles checked; 0 errors, 6 warnings.
[mac@rhodey ~]$

SPEC: http://dbmacartney.fedorapeople.org/barman/barman.spec
SRPM: http://dbmacartney.fedorapeople.org/barman/barman-1.2.3-9.fc20.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=6411170
Comment 16 Peter Robinson 2014-01-15 18:45:20 EST
That looks fine. 

APPROVED
Comment 17 Dale Macartney 2014-01-15 19:27:36 EST
New Package GIT Request
=======================
Package Name: barman
Short Description: Backup and Recovery Manager for PostgreSQL
Owners: dbmacartney
Branches: F-20 F-19 EL-6
InitialCC:
Comment 18 Gwyn Ciesla 2014-01-16 07:48:31 EST
Git done (by process-git-requests).
Comment 19 Dale Macartney 2015-01-05 21:22:09 EST
New Package GIT Request
=======================
Package Name: barman
Short Description: Backup and Recovery Manager for PostgreSQL
Owners: dbmacartney
Branches: epel7
InitialCC:
Comment 20 Gwyn Ciesla 2015-01-06 08:43:17 EST
Git done (by process-git-requests).
Comment 21 Dale Macartney 2015-01-08 19:19:26 EST
Resubmitting. It seems the branch didn't get created. 

Package Change Request
======================
Package Name: barman
New Branches: epel7	
Owners: dbmacartney
Comment 22 Gwyn Ciesla 2015-01-09 08:07:54 EST
Git done (by process-git-requests).

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