Bug 1269609 - Review Request: ari-backup - A wrapper around rdiff-backup
Review Request: ari-backup - A wrapper around rdiff-backup
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Randy Barlow
Fedora Extras Quality Assurance
Richard Shaw
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-07 13:21 EDT by Randy Barlow
Modified: 2016-01-01 01:51 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-01-01 01:51:36 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hobbes1069: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Randy Barlow 2015-10-07 13:21:08 EDT
Spec URL: https://raw.githubusercontent.com/rbarlow/ari-backup/fedora-package/distribution_packages/rpm/ari-backup.spec
SRPM URL: media.electronsweatshop.com/ari-backup-1.0.10-1.fc22.src.rpm
Description: A helpful wrapper around rdiff-backup that allows backup jobs to be simple Python modules.
Fedora Account System Username: rbarlow


This is my first package, and I need a sponsor. I am one of the contributors to this package upstream. I work full time on the Pulp Project as well, and am an experienced Python programmer.
Comment 1 Richard Shaw 2015-10-07 14:05:19 EDT
I'm tempted to take this review but I'm going to be busy until sometime next week. We'll see if anyone else jumps in. 

Until then, here's some quick feedback on your spec file:

What distro & versions are you wanting to support? Unless you're going to go back to RHEL 5 I think a lot of stuff can be removed but I would have to double check the guidelines to be sure for every case.

1. All currently supported versions of Fedora and RHEL 6+ define python_sitelib / python_sitearch:
(EPEL 6 machine)
$ rpm -E %python_sitelib
/usr/lib/python2.6/site-packages

2. Group: tags are optional

3. BuildRoot can be removed

4. rm -rf %{buildroot} in %install is only needed for RHEL < 6

5. %clean is done automatically (only needs to be specified in certain multi-source situations)

6. %defattr(-,root,root,-) is automatic, you only need to specify it for your special cases.
Comment 3 Richard Shaw 2015-10-07 22:24:53 EDT
Ok, I'm not going to post the full fedora-review output since there's some things that need fixing, but here's some excerpts.

Issues:
=======
- Permissions on files are set properly.
  Note: See rpmlint output
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions

I noticed these were intentional but it may help to describe in comments why the special permissions are necessary (more detail in rpmlint output)


- Package contains BR: python2-devel or python3-devel

I'm questioning this one, assuming this package is both python 2 and 3 compatible, and you're using %{_python}, then you would want the build requirement to be whichever python is the default for that system. If it's not python3 compatible we'll need to specify python2 everywhere.


- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /etc/ari-backup/ari-backup.conf.yaml
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles


- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file LICENSE.txt is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

If you want to support EPEL 6 the simplest solution is something like this:
%if 0%{?rhel} < 7 || 0%{?fedora} < 21
%doc LICENSE.txt
%else
%license LICENSE.txt
%endif

Rpmlint
-------
Checking: ari-backup-1.0.10-1.fc24.noarch.rpm
          ari-backup-1.0.10-1.fc24.src.rpm
ari-backup.noarch: E: description-line-too-long C A helpful wrapper around rdiff-backup that allows backup jobs to be simple Python modules.
ari-backup.noarch: E: no-changelogname-tag
ari-backup.noarch: W: conffile-without-noreplace-flag /etc/cron.daily/ari-backup
ari-backup.noarch: E: non-readable /etc/ari-backup/jobs.d/ari-backup-remote-lvm-demo 600
ari-backup.noarch: E: non-executable-script /etc/ari-backup/jobs.d/ari-backup-remote-lvm-demo 600 /usr/bin/env
ari-backup.noarch: E: non-readable /usr/share/doc/ari-backup/LICENSE.txt 700
ari-backup.noarch: E: non-standard-executable-perm /usr/share/doc/ari-backup/LICENSE.txt 700
ari-backup.noarch: W: spurious-executable-perm /usr/share/doc/ari-backup/LICENSE.txt
ari-backup.noarch: E: wrong-script-end-of-line-encoding /usr/share/doc/ari-backup/LICENSE.txt
ari-backup.noarch: E: non-readable /etc/ari-backup/jobs.d/ari-backup-local-lvm-demo 600
ari-backup.noarch: E: non-executable-script /etc/ari-backup/jobs.d/ari-backup-local-lvm-demo 600 /usr/bin/env
ari-backup.noarch: E: non-readable /etc/ari-backup/jobs.d/ari-backup-remote-demo 600
ari-backup.noarch: E: non-executable-script /etc/ari-backup/jobs.d/ari-backup-remote-demo 600 /usr/bin/env
ari-backup.noarch: E: non-readable /etc/cron.daily/ari-backup 700
ari-backup.noarch: E: non-standard-executable-perm /etc/cron.daily/ari-backup 700
ari-backup.noarch: E: executable-marked-as-config-file /etc/cron.daily/ari-backup
ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup 700
ari-backup.noarch: E: non-standard-dir-perm /var/lib/ari-backup 700
ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup/jobs.d 700
ari-backup.noarch: E: non-readable /etc/ari-backup/jobs.d/ari-backup-local-demo 600
ari-backup.noarch: E: non-executable-script /etc/ari-backup/jobs.d/ari-backup-local-demo 600 /usr/bin/env
ari-backup.noarch: E: non-standard-dir-perm /usr/share/doc/ari-backup 700
ari-backup.noarch: E: non-readable /usr/share/doc/ari-backup/README.md 700
ari-backup.noarch: E: non-standard-executable-perm /usr/share/doc/ari-backup/README.md 700
ari-backup.noarch: W: spurious-executable-perm /usr/share/doc/ari-backup/README.md
ari-backup.noarch: E: non-readable /etc/ari-backup/ari-backup.conf.yaml 600
ari-backup.src: E: description-line-too-long C A helpful wrapper around rdiff-backup that allows backup jobs to be simple Python modules.
ari-backup.src: E: no-changelogname-tag

Hopefully we can clear these up or justify them (in the case of the permissions).

If it's important for the executables to only be readable by root, I wonder if they should be in /usr/sbin instead?
Comment 4 Randy Barlow 2015-10-08 13:55:39 EDT
Hi Richard, thank you for the helpful notes! I've updated the spec file (at the same URL) and the SRPM as well. I've replied to a few notes from your first comment below:

(In reply to Richard Shaw from comment #1)
> What distro & versions are you wanting to support?

For now I only plan to support Fedora 22 and 23. I may expand support to RHEL 7 later, but I may also wait for RHEL 8 for simplicity. I hope to work with upstream soon to convert this project to Python 3 so supporting only Fedora to begin with would make that easier I think.

> 1. All currently supported versions of Fedora and RHEL 6+ define
> python_sitelib / python_sitearch

I removed this.

> 2. Group: tags are optional

I removed this.

> 3. BuildRoot can be removed

Done.

> 4. rm -rf %{buildroot} in %install is only needed for RHEL < 6

I removed this.

> 5. %clean is done automatically (only needs to be specified in certain
> multi-source situations)

I removed it.

> 6. %defattr(-,root,root,-) is automatic, you only need to specify it for
> your special cases.

I removed this too.

Thanks! I'll reply to your second commend separately as I didn't completely follow everything.
Comment 5 Randy Barlow 2015-10-08 14:11:30 EDT
(In reply to Richard Shaw from comment #3)
> - Permissions on files are set properly.
>   Note: See rpmlint output

I didn't realize that rpmlint was meant to be run against the packages and not the spec file, so I apologize for not catching some of this myself. rpmlint said there were no errors against the spec file when I tried that before, but I now see these same errors against the srpm and rpm. Good to know!

> I noticed these were intentional but it may help to describe in comments why
> the special permissions are necessary (more detail in rpmlint output)

I adjusted the permissions a little bit, but rpmlint is still upset at me. Since this is backup software it manages sensitive information about other computers, and that is why I felt the need to protect much of the data. I added comments to the spec file to explain why I chose the permissions I did. What do you think, given those comments?

> - Package contains BR: python2-devel or python3-devel
> 
> I'm questioning this one, assuming this package is both python 2 and 3
> compatible, and you're using %{_python}, then you would want the build
> requirement to be whichever python is the default for that system. If it's
> not python3 compatible we'll need to specify python2 everywhere.

I think I'm missing something here. I don't see the string "devel" in my spec file. Am I doing something that is bringing this requirement in automatically? This package is only Python 2 currently - do I need to do something different than I currently am doing to make that clear?

> - Package does not contain duplicates in %files.
>   Note: warning: File listed twice: /etc/ari-backup/ari-backup.conf.yaml
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

This one ended up being confusing to figure out, but I think I've got it now. It ended up being a mistake to have a path and a path's parent path listed. I reduced that section down to two lines and the complaints have disappeared.

> - If (and only if) the source package includes the text of the license(s)
>   in its own file, then that file, containing the text of the license(s)
>   for the package is included in %license.

Fixed.

> If it's important for the executables to only be readable by root, I wonder
> if they should be in /usr/sbin instead?

This package only includes one executable - the cron job. The cron job looks at /etc/ari-backup/jobs.d/ for executable files and runs any of them that it finds (this is the upstream design).

I will post the remaining rpmlint output I still see with inline comments explaining what I think about it in my next comment.
Comment 6 Randy Barlow 2015-10-08 14:34:03 EDT
Here is the current rpmlint

> $ rpmlint -i rpmbuild/RPMS/noarch/ari-backup-1.0.10-1.fc22.noarch.rpm rpmbuild/SRPMS/ari-backup-1.0.10-1.fc22.src.rpm 
> ari-backup.noarch: W: spelling-error Summary(en_US) rdiff -> riff, diff, r diff
> The value of this tag appears to be misspelled. Please double-check.

I don't think this is a problem. There is another warning about the same thing that I've omitted.


> ari-backup.noarch: W: conffile-without-noreplace-flag /etc/cron.daily/ari-backup
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here

I didn't put the noreplace on here because it's not really a configuration file, it's a daily cron job. If there are updates to the cron job, I would expect that a user would want the updated cron job. Do you think I should make it config(noreplace) as suggested by the lint?


> ari-backup.noarch: E: non-readable /etc/ari-backup/jobs.d/ari-backup-remote-lvm-demo 600
> The file can't be read by everybody. Review if this is expected.

This is a demo backup job that is really meant to help guide the user on how to create backup jobs. A normal backup job probably shouldn't be world readable because it may contain sensitive information, so the demo has realistic permissions set as well. Do you think this is reasonable? There are four of these files, so I have trimmed the other three similar notes from this output.


> ari-backup.noarch: E: non-executable-script /etc/ari-backup/jobs.d/ari-backup-remote-lvm-demo 600 /usr/bin/env
> This text file contains a shebang or is located in a path dedicated for
> executables, but lacks the executable bits and cannot thus be executed.  If
> the file is meant to be an executable script, add the executable bits,
> otherwise remove the shebang or move the file elsewhere.

This file is not executable because the cron job executes files in this folder that have the execute bit set. Since this is a demo job, we don't really want it to be executable, but we do want it to have the shebang because a real backup job needs a shebang as well. Another option might be to put these examples in /usr/share/doc/ari-backup and maybe drop a README in this job.d that mentions that users can check out examples over there. What do you think? I've also trimmed the other three complaints about the other examples.


> ari-backup.noarch: E: non-standard-executable-perm /etc/cron.daily/ari-backup 744
> A standard executable should have permission set to 0755. If you get this
> message, it means that you have a wrong executable permissions in some files
> included in your package.

I chose 744 because it's very likely that ordinary users should not be able to execute the backup jobs. Of course, if the files in the jobs.d are also not user executable this cron job won't be able to do anything anyway, so perhaps it's not harmful to give it 755 to get this complaint to disappear. What do you think?


> ari-backup.noarch: E: non-standard-dir-perm /var/lib/ari-backup 700
> A standard directory should have permission set to 0755. If you get this
> message, it means that you have wrong directory permissions in some dirs
> included in your package.

I put 0700 on this folder because it contains the backup data from other computers. Since we don't know what kind of data this might be, keeping prying eyes out of this folder is a good idea.


> ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup/jobs.d 700
> A standard directory should have permission set to 0755. If you get this
> message, it means that you have wrong directory permissions in some dirs
> included in your package.

The backup jobs may contain sensitive information about the hosts that are being backed up. At the very least, they will contain the list of inclusions and exclusions, but they might also contains arbitrary Python code to perform certain actions on the remote hosts that might need to be kept secret.


Let me know what you think, and thanks again for your assistance!
Comment 7 Randy Barlow 2015-10-08 14:42:57 EDT
(In reply to Randy Barlow from comment #6)
> > ari-backup.noarch: W: conffile-without-noreplace-flag /etc/cron.daily/ari-backup
> > A configuration file is stored in your package without the noreplace flag. A
> > way to resolve this is to put the following in your SPEC file:
> > %config(noreplace) /etc/your_config_file_here
> 
> I didn't put the noreplace on here because it's not really a configuration
> file, it's a daily cron job. If there are updates to the cron job, I would
> expect that a user would want the updated cron job. Do you think I should
> make it config(noreplace) as suggested by the lint?

I removed the %config directive and this seems to have gone away.
Comment 8 Richard Shaw 2015-10-08 14:53:30 EDT
Ok, yes it looks like most of the rpmlint issues have been resolved. Also, rpmlint doesn't always get it right so we can choose to ignore it, we just need to understand why it's ok.

Removing %config is probably the right way to go, generally %config is only used for files the admin may need to edit.

Also, I didn't worry about it this time but it's customary to bump the revision of the package when making adjustments to the spec file so it has a clear history. I usually post the new spec and SRPM link (the spec one doesn't change) and then cut-n-paste the relevant changelog entry below the links.
Comment 9 Richard Shaw 2015-10-08 15:39:55 EDT
(In reply to Randy Barlow from comment #5)
> > - Package contains BR: python2-devel or python3-devel
> > 
> > I'm questioning this one, assuming this package is both python 2 and 3
> > compatible, and you're using %{_python}, then you would want the build
> > requirement to be whichever python is the default for that system. If it's
> > not python3 compatible we'll need to specify python2 everywhere.
> 
> I think I'm missing something here. I don't see the string "devel" in my
> spec file. Am I doing something that is bringing this requirement in
> automatically? This package is only Python 2 currently - do I need to do
> something different than I currently am doing to make that clear?

Ok, python-setuptools is pulling in python2-devel for you. Since it appears that Python 3 is not supported at this time then you must specifically require Python 2 in two different places:

1. In the build requirements
BuildRequires: python2-devel

2. When calling python:

%{__python} -> %{__python2}

This is required because at some point Fedora will switch to Python 3 by default and the definition of %{__python} will change.

https://fedoraproject.org/wiki/Packaging:Python
Comment 10 Randy Barlow 2015-10-08 19:11:28 EDT
Hi again Richard! I made the fixes you suggested, and I went ahead and moved those example jobs into the docs folder so that rpmlint would be happier. I think they make sense to be documentation anyway, so IMO it works for everyone. I've raised the release to 2.

Currently, rpmlint only complains about spelling and these two errors which I think are justifiable to ignore:

ari-backup.noarch: E: non-standard-dir-perm /var/lib/ari-backup 700
ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup/jobs.d 700

Here are the new URLs:

Spec URL: https://raw.githubusercontent.com/rbarlow/ari-backup/fedora-package/distribution_packages/rpm/ari-backup.spec
SRPM URL: http://media.electronsweatshop.com/ari-backup-1.0.10-2.fc22.src.rpm

Let me know if there are further improvements I can make, and thanks for taking the time to help!
Comment 11 Richard Shaw 2015-10-09 08:49:00 EDT
Looks good!

I'm going to be out of pocket for the next few days so what I would recommend if you haven't done so already is do some informal reviews (it's a good idea to let people know it's an informal review so they know you're not doing the real review) on some packages and post the links here. 

https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Convincing_someone_to_sponsor_you

Also, what are you interests? Mostly python packaging? Are there any packages you'd like to co-maintain?
Comment 12 Randy Barlow 2015-10-10 18:24:45 EDT
(In reply to Richard Shaw from comment #11)
> I'm going to be out of pocket for the next few days so what I would
> recommend if you haven't done so already is do some informal reviews (it's a
> good idea to let people know it's an informal review so they know you're not
> doing the real review) on some packages and post the links here. 

Hi again Richard! Sure thing, I've now done one informal review here on another Python package:

https://bugzilla.redhat.com/show_bug.cgi?id=1268380#c2

I'll try to do a few more reviews in the coming days. If you have a review of my review, that would be helpful (and meta!)

> Also, what are you interests? Mostly python packaging? Are there any
> packages you'd like to co-maintain?

I do work as a full time Python developer so Python is my strength these days. In the past I have also done work in C/C++, Java, and PHP. I would not rate my Java or PHP knowlege to be current enough to be very useful, but I am still pretty good in C/C++ land.

One of my long term goals for wanting to join the Fedora project is getting my full time project included in Fedora:

http://www.pulpproject.org/

It is also a Python project, and I suspect that you might cringe if you saw it's current spec file ☺ It's a pretty complicated project with quite a few packages (it's a distributed system) so I didn't want to use it as my first package review. Once I gain some more experience I'd love to get it included in Fedora. We currently host our own repository for Fedora on repos.fedorapeople.org/pulp. I also happen to know that Fedora has some interest in possibly using Pulp to manage some of their systems so it may be beneficial to the project itself.

I also have side interests in network security and scientific computing. My C/C++ experience is from my graduate studies as a computational electromagnetism scientist. I've also done some work in natural language processing with both Python and C++ in the past.

As far as co-maintaining packages, I'd love to help in that way. I don't know of any specific package that both needs a co-maintainer and happens to be a package that I have extensive knowledge of, but I am willing to learn. Do you have a suggestion that might fall into my areas of knowledge?

Thanks again for your guidance!
Comment 13 Richard Shaw 2015-10-15 09:58:38 EDT
(In reply to Randy Barlow from comment #12)
> 
> Hi again Richard! Sure thing, I've now done one informal review here on
> another Python package:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1268380#c2

That's a good start, it wouldn't hurt to find a few more.

Here's a good way to find some, I would skip over the older ones unless you find one you are really interested in:

https://fedoraproject.org/PackageReviewStatus/NEW.html


> > Also, what are you interests? Mostly python packaging? Are there any
> > packages you'd like to co-maintain?
> 
> I do work as a full time Python developer so Python is my strength these
> days. In the past I have also done work in C/C++, Java, and PHP. I would not
> rate my Java or PHP knowlege to be current enough to be very useful, but I
> am still pretty good in C/C++ land.

There is a Fedora Python SIG you may want to join then:

https://fedoraproject.org/wiki/SIGs/Python

We can certainly work towards me becoming your sponsor but if someone in the Python SIG is intereated in becoming your sponsor then that's probably a better fit.


> One of my long term goals for wanting to join the Fedora project is getting
> my full time project included in Fedora:
> 
> http://www.pulpproject.org/

That looks pretty neat. I wouldn't mind trying it out but it may be a bit overkill for my 2 desktop and 3 laptop home network :)


> It is also a Python project, and I suspect that you might cringe if you saw
> it's current spec file ☺ It's a pretty complicated project with quite a few
> packages (it's a distributed system) so I didn't want to use it as my first
> package review. Once I gain some more experience I'd love to get it included
> in Fedora. We currently host our own repository for Fedora on
> repos.fedorapeople.org/pulp. I also happen to know that Fedora has some
> interest in possibly using Pulp to manage some of their systems so it may be
> beneficial to the project itself.

Yeah, we should use Fedora supplied packages wherever possible. If you want to start working towards getting it into Fedora the best option might be to start with COPR[1] as the only real requirement is that all the content is legal (acceptable license). The full packaging guidelines don't apply.

[1] https://copr.fedoraproject.org/coprs/
Comment 14 Randy Barlow 2015-10-16 10:48:04 EDT
Hi Richard! I haven't forgotten that I need to do some more reviews. I've been traveling for the past week, and will be traveling until mid next week as well. I'll be sure to do a few more when I return home!
Comment 15 Randy Barlow 2015-10-27 22:13:52 EDT
Hi Richard! I did one more review on another Python package here:

https://bugzilla.redhat.com/show_bug.cgi?id=1275517

As for whether you or someone from the Python SIG could sponsor me, I'd appreciate it if you would consider being my sponsor. I feel like we have had a good rapport so far!
Comment 16 Randy Barlow 2015-10-27 22:29:06 EDT
I have reviewed one more package here:

https://bugzilla.redhat.com/show_bug.cgi?id=1268090
Comment 17 Richard Shaw 2015-10-29 14:37:46 EDT
Alright, I'm formally taking the review now.

Have you already done a self introduction on the devel list? It's pretty much required to be subscribed as a packager. 

I think we've gotten the package in pretty good shape, however I think it would be a good idea to ask on the fedora devel mailing list about the permissions just to make sure there's no unwanted side effects that we can't think of.

Perhaps kill two birds with one stone.
Comment 18 Randy Barlow 2015-11-01 12:56:57 EST
Thank you Richard! I had introduced myself in early October on the devel mailing list:

https://lists.fedoraproject.org/pipermail/devel/2015-October/215426.html

I replied to my introduction (and CC'd you) to ask what others thought about my deviations from ordinary permissions.
Comment 19 Richard Shaw 2015-11-05 09:14:43 EST
Ok, one more question!

Do you want to support RHEL through EPEL? Since this is designed to run on a server it would seem to be a good fit. EPEL 7 should already work but EPEL 6 doesn't have the %license macro defined, but you can fix that with a one liner:

%files
%{!?_licensedir:%global license %doc}
%license LICENSE.txt
...
Comment 20 Randy Barlow 2015-11-06 11:29:27 EST
(In reply to Richard Shaw from comment #19)
> Do you want to support RHEL through EPEL?

I had not intended to support EPEL 6, as I would like to work with upstream to port ari-backup to be a Python 3 only application. I'm not certain that we would do that, but I'd like to leave that possibility open for now. If we don't end up doing that I could always add EPEL 6 later, right?

I do intend this to be included in EPEL 7.
Comment 21 Richard Shaw 2015-11-06 11:52:48 EST
(In reply to Randy Barlow from comment #20)
> (In reply to Richard Shaw from comment #19)
> > Do you want to support RHEL through EPEL?
> 
> I do intend this to be included in EPEL 7.

Ok, keep in mind python 3 is only available through Fedora EPEL and the package name is python34.
Comment 22 Richard Shaw 2015-11-06 12:07:21 EST
The only thing it looks like you need to fix is ownership of /etc/ari-backup. A simple "%dir %{_sysconfdir}/%{name}" before %config should do it.

I have also sponsored you into the packagers group. Use you powers for good!

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "BSD (3 clause)", "Unknown or generated". 22 files have unknown
     license. Detailed output of licensecheck in /home/build/fedora-
     review/1269609-ari-backup/licensecheck.txt
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /etc/ari-backup
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /etc/ari-backup
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 5 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: ari-backup-1.0.10-2.fc24.noarch.rpm
          ari-backup-1.0.10-2.fc24.src.rpm
ari-backup.noarch: W: spelling-error Summary(en_US) rdiff -> riff, diff, r diff
ari-backup.noarch: W: spelling-error %description -l en_US rdiff -> riff, diff, r diff
ari-backup.noarch: E: non-standard-dir-perm /var/lib/ari-backup 700
ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup/jobs.d 700
ari-backup.src: W: spelling-error Summary(en_US) rdiff -> riff, diff, r diff
ari-backup.src: W: spelling-error %description -l en_US rdiff -> riff, diff, r diff
2 packages and 0 specfiles checked; 2 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
ari-backup.noarch: E: non-standard-dir-perm /etc/ari-backup/jobs.d 700
ari-backup.noarch: E: non-standard-dir-perm /var/lib/ari-backup 700
1 packages and 0 specfiles checked; 2 errors, 0 warnings.



Requires
--------
ari-backup (rpmlib, GLIBC filtered):
    PyYAML
    config(ari-backup)
    crontabs
    python(abi)
    python-gflags
    python-setuptools
    rdiff-backup



Provides
--------
ari-backup:
    ari-backup
    config(ari-backup)



Source checksums
----------------
https://github.com/jpwoodbu/ari-backup/archive/1.0.10.tar.gz :
  CHECKSUM(SHA256) this package     : 4626b73455193b3ba67f155eca15cb8e71a9630bb9245d493213d18d2ac81be0
  CHECKSUM(SHA256) upstream package : 4626b73455193b3ba67f155eca15cb8e71a9630bb9245d493213d18d2ac81be0


Generated by fedora-review 0.5.3 (bcf15e3) last change: 2015-05-04
Command line :/bin/fedora-review -b 1269609 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api
Disabled plugins: Java, C/C++, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 23 Gwyn Ciesla 2015-11-08 11:14:13 EST
Package request has been denied with the reason: fedora-review flag is no `+` but is still `?`
Comment 24 Gwyn Ciesla 2015-11-08 11:14:40 EST
Package request has been denied with the reason: fedora-review flag is no `+` but is still `?`
Comment 25 Gwyn Ciesla 2015-11-08 11:15:08 EST
Package request has been denied with the reason: fedora-review flag is no `+` but is still `?`
Comment 26 Gwyn Ciesla 2015-11-08 11:15:41 EST
Package request has been denied with the reason: fedora-review flag is no `+` but is still `?`
Comment 27 Randy Barlow 2015-11-08 18:41:00 EST
I have fixed the ownership of /etc/ari-backup. The fixed spec file and source RPMs are here:

SPEC: https://raw.githubusercontent.com/rbarlow/ari-backup/fedora-package/distribution_packages/rpm/ari-backup.spec
SRPM: https://storm.electronsweatshop.com/index.php/s/WqRjt6SgyHKDFIH
Comment 28 Randy Barlow 2015-11-08 18:43:51 EST
Hi again Richard! I assume that it's up to you to set the + flag that Jon mentioned, and that you were waiting on my fix for that folder. Is that right? Are we all set now?

Thanks so much for your guidance as I've been learning the ropes!
Comment 29 Richard Shaw 2015-11-08 20:48:42 EST
Yeah, I don't know why his script was picking up this package review since I hadn't approved it yet...

Anyhow, it looks good now!

*** APPROVED ***
Comment 30 Gwyn Ciesla 2015-11-10 09:59:52 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/ari-backup
Comment 31 Fedora Update System 2015-11-16 23:00:49 EST
ari-backup-1.0.10-3.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-24ef0be480
Comment 32 Fedora Update System 2015-11-16 23:09:02 EST
ari-backup-1.0.10-3.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 33 Fedora Update System 2015-11-17 20:20:30 EST
ari-backup-1.0.10-3.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update ari-backup'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 34 Fedora Update System 2015-11-18 11:51:34 EST
ari-backup-1.0.10-3.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update ari-backup'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-24ef0be480
Comment 35 Fedora Update System 2015-11-25 16:52:27 EST
ari-backup-1.0.10-3.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 36 Fedora Update System 2015-12-13 22:39:13 EST
ari-backup-1.0.10-3.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 37 Fedora Update System 2015-12-14 07:19:10 EST
ari-backup-1.0.10-3.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update ari-backup'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 38 Fedora Update System 2015-12-14 14:24:52 EST
ari-backup-1.0.10-3.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 39 Fedora Update System 2015-12-15 02:20:37 EST
ari-backup-1.0.10-3.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update ari-backup'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-120a2062db
Comment 40 Fedora Update System 2015-12-31 18:29:22 EST
ari-backup-1.0.10-3.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

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