Bug 235527 - Review Request: pylibacl - POSIX.1e ACLs library wrapper for python
Summary: Review Request: pylibacl - POSIX.1e ACLs library wrapper for python
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-04-06 18:29 UTC by Marcin Zajaczkowski
Modified: 2007-11-30 22:12 UTC (History)
0 users

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: ---
kevin: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

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.


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