Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 178169
Summary: | Review Request: jigdo | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Ian Burrell <ianburrell> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-extras-list |
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: | 2006-03-22 19:28:35 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: | |||
Bug Depends On: | 178310 | ||
Bug Blocks: | 163779 |
Description
Ian Burrell
2006-01-18 07:29:32 UTC
Not a review, but you've got some duplicate BuildRequires. According to fedora-qa script: Duplicate BuildRequires: zlib-devel (by openssl-devel), openssl-devel (by curl-devel), atk-devel (by gtk2-devel), glib2-devel (by gtk2-devel), pango-devel (by gtk2-devel), XFree86-devel (by gtk2-devel), freetype-devel (by pango-devel) Here's a review: MUST items: OK Package name. OK Spec file name. OK License good (GPL). OK License in spec matches. OK License is included (GPL COPYING) OK Spec english/readable OK Sources match upstream (md5: 031756ff6c7084a139dc9550a27f6906) OK No packages in BR that are in the exceptions list. OK Correctly uses find_lang OK No dynamic libs, so no ldconfig needed. OK Builds on fc4. OK No rpmlint output (on fc4 build). Good! OK Package owns all it's created directories. OK No duplicate files in files section. OK Permissions look good. OK Clean section looks good. OK desktop file looks good and good install. OK Runs fine on my fc4 machine. ISSUES: 1. Doesn't build in current devel/mock. It has two unavailable BuildRequires: No Package Found for w3c-libwww-devel >= 0:5.3.2 No Package Found for XFree86-devel The w3c-libwww package no longer seems available in core in devel/fc5. XFree86-devel has been replaced with modular xorg packages. You may need to create and import a w3c-libwww package before this one if that support is needed by this package. Can you see if that support is optional, and/or some new package now provides that support for fc5? Also, if you could review/comment on some other in review packages to show your understanding of the package guidelines, that would assist in deciding to sponsor you. ;) You can find a list in bugzilla at: https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-REVIEW&hide_resolved=1 w3c-libwww is bug 178310 I modified the spec to remove the duplicate BuildRequires. Removing XFree86-devel means the same spec builds on devel after the w3c-libwwww-devel in 178310 is installed. I added a patch to build on gcc 4.1. Spec: http://znark.com/fedora/extras/jigdo.spec SRPM: http://znark.com/fedora/extras/jigdo-0.7.2-2.src.rpm The specfile shows that it owns everything in /usr/share/applications (%{_datadir}/applications/*), as well as all of the standard binaries (%{_bindir}/*), which is probably very bad. ;-) Also, your %files section contains: %dir %{_datadir}/%{name} %{_datadir}/%{name}/* Wouldn't simply owning the directory be simpler? %{_datadir}/%{name} My two cents. >The specfile shows that it owns everything in /usr/share/applications
>(%{_datadir}/applications/*), as well as all of the standard binaries
>(%{_bindir}/*), which is probably very bad. ;-)
Those globs are expanded at build time under the $RPM_BUILD_ROOT, so they only
match files that are installed in the %install step of the rpm building. :)
In this case they expand to:
/usr/share/applications/fedora-jigdo.desktop
/usr/bin/jigdo
/usr/bin/jigdo-file
/usr/bin/jigdo-lite
Which is fine.
Some folks like using * in files to match a bunch of files, while others prefer
to list each file out. It's just a matter of taste as long as there are no
duplicate files listed or files/dirs that are owned by another package already.
I thought just listing the directory would work. And it did work for building on FC4. But when I built the package on rawhide, it complained about the files in usr/shared/jigdo not being included. So I put in the glob. (In reply to comment 6) Ack. I had not realized that. Thanks, Kevin! Hey Ian. Can you build against the latest w3c-libwww from https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178310 and confirm that the package still works as expected? w3c-libwww is now in extras. Can you confirm that this package functions as expected compiled against that package? In the gcc41 patch you move the outOfMemory function around. Is that really needed? I don't see any changes made to the function, just moving it. Sorry about taking so long to respond. I just discovered that jigdo doesn't use w3c-libwww. I hadn't noticed until that the binary package didn't depend on w3c-libwww package. It looks like upstream replaced using libwww with libcurl in version 0.7.2. I just rebuilt it without the BuildRequires on w3c-libwww-devel and the result works. I posted the new spec and srpm. http://znark.com/fedora/extras/jigdo.spec http://znark.com/fedora/extras/jigdo-0.7.2-1.src.rpm The change is actually moving the cmdOptions method and the enum out of the local namespace block while keeping outOfMemory inside the namespace. GCC 4.1 seems to have stricter rules about namespaces. Sorry for the delay in getting back to you on this review. This version looks good. The removal of the w3c-libwww-devel BR looks good. The namespace move makes sense. GCC 4.1 seems stricter in general on g++. Everything looks good. This package is APPROVED. I would be happy to sponsor you. Please continue the process from the "Get a Fedora Account" section at: http://fedoraproject.org/wiki/Extras/Contributors If you have any questions at all, please don't hesitate to drop me an email. Built for extras devel. |