Bug 235527 - Review Request: pylibacl - POSIX.1e ACLs library wrapper for python
Review Request: pylibacl - POSIX.1e ACLs library wrapper for python
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-06 14:29 EDT by Marcin Zajaczkowski
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-06 12:06:07 EDT
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)

  None (edit)
Description Marcin Zajaczkowski 2007-04-06 14:29:38 EDT
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 14:37:13 EDT
Added FE-NEW dependency.
Comment 2 Marcin Zajaczkowski 2007-04-06 16:09:55 EDT
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-17 22:29:56 EDT
I'd be happy to review this package. Look for a full review in a bit here. 
Comment 4 Kevin Fenzi 2007-04-17 22:44:03 EDT
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 15:56:15 EDT
(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 16:11:13 EDT
Changed package name in a bug summary.
Comment 7 Kevin Fenzi 2007-04-18 23:10:01 EDT
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 14:45:26 EDT
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 17:31:01 EDT
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 12:06:07 EDT
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 04:44:02 EDT
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 00:52:06 EDT
cvs done.

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