Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.

Bug 235527

Summary: Review Request: pylibacl - POSIX.1e ACLs library wrapper for python
Product: [Fedora] Fedora Reporter: Marcin Zajaczkowski <mszpak>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideFlags: kevin: fedora-review+
kevin: fedora-cvs+
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-05-06 16:06:07 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Marcin Zajaczkowski 2007-04-06 18:29:38 UTC
Spec URL: http://timeoff.wsisiz.edu.pl/rpms/python-libacl/python-libacl.spec
SRPM URL: http://timeoff.wsisiz.edu.pl/rpms/python-libacl/python-libacl-0.2.1-2.src.rpm
Description: Python extension module for POSIX ACLs. It allows to query, list,
add and remove ACLs from files and directories.

It's a recommended runtime dependency for rdiff-backup (already in Fedora Extras).

Comment 1 Marcin Zajaczkowski 2007-04-06 18:37:13 UTC
Added FE-NEW dependency.

Comment 2 Marcin Zajaczkowski 2007-04-06 20:09:55 UTC
A path to the source package should be:
http://heanet.dl.sourceforge.net/sourceforge/pylibacl-%{version}.tar.bz2

I don't want to bump release number only because of that, so I will fix it
together with other changes suggested in a review.

Comment 3 Kevin Fenzi 2007-04-18 02:29:56 UTC
I'd be happy to review this package. Look for a full review in a bit here. 


Comment 4 Kevin Fenzi 2007-04-18 02:44:03 UTC
See below - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
5c06b62ca4ee453042c9fa3f28aa56e8  pylibacl-0.2.1.tar.bz2
5c06b62ca4ee453042c9fa3f28aa56e8  pylibacl-0.2.1.tar.bz2.1
OK - BuildRequires correct
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Is the name right here? According to the guidelines if the upstream has 'py'
in it
you can just use that name:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-8756a3bce652c376d7ba3908461b638784b6952d
so perhaps pylibacl would be acceptable instead of python-libacl?

2. Upstream doesn't seem to active, but you might ask if they can include
a copy of the GPL with the package. Not a blocker.

3. The Source URL should probibly be:
http://downloads.sourceforge.net/pylibacl/pylibacl-%{version}.tar.bz2

4. Do you need the
%defattr(0644,root,root,0755)
or does a normal
%defattr(-,root,root,-)
Install files with the correct permissions?


Comment 5 Marcin Zajaczkowski 2007-04-18 19:56:15 UTC
(In reply to comment #4)
> 1. Is the name right here? According to the guidelines if the upstream has
> 'py' in it you can just use that name:
(...)
> so perhaps pylibacl would be acceptable instead of python-libacl?

You are right. I missed a line about an exception.

> 2. Upstream doesn't seem to active, but you might ask if they can include
> a copy of the GPL with the package. Not a blocker.

Ok, I will.

> 3. The Source URL should probibly be:
> http://downloads.sourceforge.net/pylibacl/pylibacl-%{version}.tar.bz2

Changed.

> 4. Do you need the
> %defattr(0644,root,root,0755)
> or does a normal
> %defattr(-,root,root,-)
> Install files with the correct permissions?

I like to define it explicitly, to make sure that strange mask in a system
doesn't make a problem (like shared files available only by root).


New links:
http://timeoff.wsisiz.edu.pl/rpms/pylibacl/pylibacl.spec
http://timeoff.wsisiz.edu.pl/rpms/pylibacl/pylibacl-0.2.1-4.src.rpm


Thanks for your reviews


Comment 6 Marcin Zajaczkowski 2007-04-18 20:11:13 UTC
Changed package name in a bug summary.

Comment 7 Kevin Fenzi 2007-04-19 03:10:01 UTC
1. ok. 
2. ok. 
3. ok. 
4. I thought we explicitly wanted (-, root, root, -), but looking at the
guilelines it just says that there must be a defattr section for each files
section, so I guess thats ok. Note that the fedora package is always going to be
built under a controlled setup and the umask shouldn't ever be a problem. 

I don't see the need for the:
#Packager tag removed due to FE requirements
#libacl is forced by RPM
comments. Thats no blocker, but unless you need them perhaps remove them before
import? 

I don't see any further blockers here, so this package is APPROVED. 
Don't forget to close this review request once this package has been imported
and built. 


Comment 8 Marcin Zajaczkowski 2007-04-20 18:45:26 UTC
New Package CVS Request
=======================
Package Name: pylibacl
Short Description: POSIX.1e ACLs library wrapper for python
Owners: mszpak@wp.pl
Branches: FC-5 FC-6
InitialCC: 

Comment 9 Marcin Zajaczkowski 2007-04-28 21:31:01 UTC
After change of a name I added:
Provides:       python-libacl = %{version}-%{release}
Obsoletes:      python-libacl <= %{version}-%{release}

(python-libacl name was used in DAG repository and package wasn't automatically
updated).

I talked with upstream and he promised to add a file with a license text to a
tarball in the next version (if any :) ).

Because of a delay between build and publish package in a file repository there
is still no version with added Provides/Obsoletes tags. I will close this issue
when I will back from my holidays and verify that.


Comment 10 Marcin Zajaczkowski 2007-05-06 16:06:07 UTC
I took a few days (2007-05-01 08:57:00), but it should be available now.
Closing issue.


Comment 11 Marcin Zajaczkowski 2007-07-07 08:44:02 UTC
Package Change Request
======================
Package Name: pylibacl
New Branches: EL-4 EL-5
Updated EPEL Owners: mszpak@wp.pl,kevin@tummy.com


Comment 12 Kevin Fenzi 2007-07-08 04:52:06 UTC
cvs done.