Bug 526738 - Review Request: py-radix - Radix tree data structure for Python
Summary: Review Request: py-radix - Radix tree data structure for Python
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Steve Traylen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-10-01 15:30 UTC by Matt Domsch
Modified: 2014-12-04 21:59 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-10-13 17:23:51 UTC
Type: ---
steve.traylen: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Matt Domsch 2009-10-01 15:30:29 UTC
Spec URL: http://domsch.com/linux/fedora/py-radix/py-radix.spec
SRPM URL: http://domsch.com/linux/fedora/py-radix/py-radix-0.5-2.src.rpm
Description: 
py-radix is an implementation of a radix tree for Python, which 
supports storage and lookups of IPv4 and IPv6 networks. 

The radix tree (a.k.a Patricia tree) is the data structure most 
commonly used for routing table lookups. It efficiently stores 
network prefixes of varying lengths and allows fast lookups of 
containing networks. py-radix's implementation is built solely 
for networks (the data structure itself is more general). 


I expect to use this in MirrorManager.

Comment 1 Steve Traylen 2009-10-01 19:17:03 UTC
Hi,

1) 
$ rpmlint py-radix.spec 
py-radix.spec:28: W: setup-not-quiet
py-radix.spec: E: no-cleaning-of-buildroot %install

%setup needs a -q and first line of install should be 
rm -rf $RPM_BUILD_ROOT

2) The release should have a dist tag. i.e
https://fedoraproject.org/wiki/Packaging/DistTag

3) There is no need to have Requires: python since it is
automatically worked out.
https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

Comment 2 Matt Domsch 2009-10-01 20:34:06 UTC
Thanks Steve.  All fixed, new versions posted at the same URLs above.

Comment 3 Steve Traylen 2009-10-01 20:42:34 UTC
Hi Matt,
 Given one of the items was to  add a dist tag its kind of impossible
 for them to be at the same URLs since the file name will have changed.

  More over between each of revisions could you please increase the Release
  number, otherwise it all becomes rather confusing.

Steve

Comment 4 Matt Domsch 2009-10-02 00:05:25 UTC
Steve, thanks for your comments. In general I don't like to bump revision during review, but it's not that big a deal so is done.  As for the dist tag, yes I had added it, but as for review purposes (or when not built for a particular dist), I tend to:

rpmbuild --define "dist %{nil}" ...


http://domsch.com/linux/fedora/py-radix/py-radix.spec
http://domsch.com/linux/fedora/py-radix/py-radix-0.5-3.fc10.src.rpm

Comment 5 Steve Traylen 2009-10-02 07:56:18 UTC
Thanks, Matt, I'll take a look later, if you are able to comment
review on bug #526544 that would be great since it is also python
review.

Of course I'll do this what ever, many thanks.

Steve

Comment 6 Steve Traylen 2009-10-02 17:27:22 UTC

$ rpmlint RPMS/x86_64/py-radix-0.5-3.fc11.x86_64.rpm 
py-radix.x86_64: W: incoherent-version-in-changelog 0.5-2 ['0.5-3.fc11', '0.5-3']

other than that it is looking pretty good I would say.

I'll through the review list afterwards.

Steve

Comment 7 Steve Traylen 2009-10-11 21:38:21 UTC
ping?

Comment 8 Matt Domsch 2009-10-11 22:46:24 UTC
Sorry, highly distracted this week.  If the only problem is that I haven't bumped the changelog, I'll do that at CVS checkin time.

Comment 10 Steve Traylen 2009-10-12 17:50:57 UTC
rpmlint clean:

$ rpmlint RPMS/x86_64/py-radix-* \
          SRPMS/py-radix-0.5-4.fc11.src.rpm \
          SPECS/py-radix.spec 
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

key:
+ okay
- problem
? notchecked.

Hi Matt,

+ rpmlint okay:
  3 packages and 1 specfiles checked; 0 errors, 0 warnings. 
+ Package name same as upstream tar ball.
+ Spec file called py-radix correctly.
+ License is BSD + advertising.
+ Spec file license correct.
+ License file is present and included.
+ Spec file is English and legible.
+ md5sum: URL and contained tar ball have same md5sum.
  $ md5sum py-radix-0.5.tar.gz ../SOURCES/py-radix-0.5.tar.gz 
  8c853ce312b769de627d958a1cd6e5a0  py-radix-0.5.tar.gz
  8c853ce312b769de627d958a1cd6e5a0  ../SOURCES/py-radix-0.5.tar.gz
+ Builds fine mock x86_64 devel
- Includes own copy of inet_top?
+ No static libs or .la files.
+ No devel packages.
+ %doc files are not needed at runtime.
+ Package owns its own directories or depends upon relavent packages.
+ All files are utf8.

Just one question about inet_top.c

This looks to be kind of standard and nothing special to this package
Its not available as part of another package or anything?

Comment 11 Matt Domsch 2009-10-12 19:06:05 UTC
Yes, I was concerned about that too, but the inet_ntop.c and strlcpy.c are only used on Windows.

setup.py has:

        libs = []
        src = [ 'radix.c', 'radix_python.c' ]
        if sys.platform == 'win32':
                libs += [ 'ws2_32' ]
                src += [ 'inet_ntop.c', 'strlcpy.c' ]

they're not used on Linux at all, so it's fine to keep them in the source tree and not have to patch them out.

Thanks,
Matt

Comment 12 Steve Traylen 2009-10-12 20:21:55 UTC
To be positive you could just rm -f the uneeded files after the %setup?
I think distutils gives  a warning normally for missing files so maybe create
empty ones afterwards.

Steve

Comment 14 Steve Traylen 2009-10-13 06:04:42 UTC
One little thing

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

is not being used at all so you may wish to remove it.

Anyway
APPROVED.

If you have the time there is python review here for me:
bug #527049

Comment 15 Matt Domsch 2009-10-13 11:52:53 UTC
New Package CVS Request
=======================
Package Name: py-radix
Short Description: Radix tree data structure for Python
Owners: mdomsch
Branches: F-10 F-11 F-12 EL-4 EL-5
InitialCC:

Comment 16 Kevin Fenzi 2009-10-13 16:31:39 UTC
cvs done.

Comment 17 Matt Domsch 2009-10-13 17:23:51 UTC
imported & built in dist-f13.  Closing.

Comment 18 Fedora Update System 2009-10-13 17:54:03 UTC
py-radix-0.5-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/py-radix-0.5-5.fc12

Comment 19 Fedora Update System 2009-10-13 17:55:33 UTC
py-radix-0.5-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/py-radix-0.5-5.fc11

Comment 20 Fedora Update System 2009-10-13 17:56:02 UTC
py-radix-0.5-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/py-radix-0.5-5.fc10

Comment 21 Fedora Update System 2009-10-13 17:56:47 UTC
py-radix-0.5-5.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/py-radix-0.5-5.el4

Comment 22 Fedora Update System 2009-10-13 17:57:13 UTC
py-radix-0.5-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/py-radix-0.5-5.el5

Comment 23 Matt Domsch 2014-12-04 21:25:06 UTC
Package Change Request
======================
Package Name: py-radix
New Branches: EL-7
Owners: 
InitialCC:

Comment 24 Kevin Fenzi 2014-12-04 21:59:21 UTC
Git done (by process-git-requests).


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