Bug 526738
| Summary: | Review Request: py-radix - Radix tree data structure for Python | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Matt Domsch <matt_domsch> |
| Component: | Package Review | Assignee: | Steve Traylen <steve.traylen> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | fedora-package-review, nb, notting, steve.traylen |
| Target Milestone: | --- | Flags: | steve.traylen:
fedora-review+
kevin: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2009-10-13 17:23:51 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Matt Domsch
2009-10-01 15:30:29 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 Thanks Steve. All fixed, new versions posted at the same URLs above. 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 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
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 $ 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 ping? 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. Changelog updated, version bumped. http://domsch.com/linux/fedora/py-radix/py-radix.spec http://domsch.com/linux/fedora/py-radix/py-radix-0.5-4.fc10.src.rpm 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?
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
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 Done. http://domsch.com/linux/fedora/py-radix/py-radix.spec http://domsch.com/linux/fedora/py-radix/py-radix-0.5-5.fc10.src.rpm
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
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: cvs done. imported & built in dist-f13. Closing. 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 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 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 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 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 Package Change Request ====================== Package Name: py-radix New Branches: EL-7 Owners: InitialCC: Git done (by process-git-requests). |