Bug 206266
Summary: | Review Request: transmission - lightweight GTK+ BitTorrent client | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Denis Leroy <denis> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | admiller, b.bellec, e.bulle, panemade |
Target Milestone: | --- | Flags: | gwync:
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: | 2006-09-26 07:02:09 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: | |||
Bug Blocks: | 163779 |
Description
Denis Leroy
2006-09-13 12:12:12 UTC
{Not Official Reviewer} packaging looks ok. + Mockbuild is successfull for i386 FC6 with some warnings + rpmlint on binary rpm is silent + dist tag is present + Buildroot is correct + source URL is correct + BR is correct + License used is MIT + License file LICENSE is included + desktop file is handled correctly + MD5 sum on tarball is matching upstream tarball 1156a88c77ab71782b9261881ea13811 Transmission-0.6.1.tar.gz + No duplicate files Application got installed correclty. Desktop icon is visible also. OK - Spec file matches base package name. OK - Meets Packaging Guidelines. OK - License (MIT) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: 1156a88c77ab71782b9261881ea13811 Transmission-0.6.1.tar.gz 1156a88c77ab71782b9261881ea13811 Transmission-0.6.1.tar.gz.1 OK - Package compiles and builds on at least one arch. OK - BuildRequires correct OK - Spec handles locales/find_lang See below - Spec has needed ldconfig in post and postun OK - Package owns all the directories it creates. OK - Package has no duplicate files in %files. OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Spec has consistant macro usage. OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package is a GUI app and has a .desktop file OK - Package doesn't own any directories other packages own. OK - No rpmlint output. SHOULD Items: OK - Should include License or ask upstream to include it. OK - Should build in mock. Issues: 1. You have: Requires(post): /sbin/ldconfig Requires(postun): /sbin/ldconfig But there are no libraries in this package, and you don't call ldconfig in post/postun. Should remove those? 2. Your desktop-file-install and update-desktop-database calls don't seem to quite match the guidelines on: http://fedoraproject.org/wiki/Packaging/ScriptletSnippets and http://fedoraproject.org/wiki/Packaging/Guidelines#desktop 3. You shouldn't use %makeinstall if you can avoid it. See: http://fedoraproject.org/wiki/Packaging/Guidelines#head- fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002 Can you use make DESTDIR=%{buildroot} install or make DESTDIR=$RPM_BUILD_ROOT install? Thanks for the review :-). Your points addressed below: 1. Fixed. 2. update-desktop-database calls fixed! The desktop-file-install looks good to me. It doesn't have the --add-category Fedora option since the category is already present in the desktop file (which is embedded into the spec). Many packages embed the desktop file into the spec directly, while others check it into CVS and add it as an extra Source: line. I'm not sure if one way is preferred over the other... 3. The use of %makeinstall here is intentional. Transmission does not use autoconf/automake and therefore does not honor DESTDIR, but rather uses a custom makefile system. My understanding was that %makeinstall is targeted at cases like this one, i.e. because it provides more variable definitions, more than what is usually needed for autoconf/automake... ? http://www.poolshark.org/src/transmission-0.6.1-2.src.rpm http://www.poolshark.org/src/transmission.spec 1. ok. Looks good. 2. Looks fine. I can't see that it matters too much either way if the desktop is a source file or part of the spec. 3. Well, %makeinstall should be avoided where possible for the reasons outlined above, but in this case it looks like you do need to use it. ;( All the blockers I see are fixed, this package is APPROVED. Don't forget to close this bug NEXTRELEASE once it's been imported and built. Built. thanks for your review :-) *** Bug 497806 has been marked as a duplicate of this bug. *** Package Change Request ====================== Package Name: transmission New Branches: epel7 Owners: maxamillion Git done (by process-git-requests). |