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 166551
Summary: | Review Request: synce-trayicon | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Andreas Bierfert <andreas.bierfert> |
Component: | Package Review | Assignee: | Aurelien Bompard <gauret> |
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 | ||
URL: | http://synce.sourceforge.net/ | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2005-12-13 23:30:20 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
Andreas Bierfert
2005-08-23 09:47:10 UTC
Hi Andreas, could you replace:
> make install DESTDIR=$RPM_BUILD_ROOT
with:
%makeinstall
?
yes works as well but I doubt that its of much use here. afaik make install DESTDIR... is still part of the fedora-extras spec template... But if you really want it I guess I can change it. I don't care either way on this one. %makeinstall is a hack which was needed back when autotools didn't have a sane way of doing staged installs. Nowadays the official documented way to do that is to use DESTDIR. It's not _that_ important and some packages will only install properly with one of the above, but when given a choice, DESTDIR is often seen as the "cleaner" way. And both occasionally reveal different upstream autotools usage bugs. (In reply to comment #3) > %makeinstall is a hack which was needed back when autotools didn't have a sane > way of doing staged installs. Nowadays the official documented way to do that > is to use DESTDIR. It's not _that_ important and some packages will only > install properly with one of the above, but when given a choice, DESTDIR is > often seen as the "cleaner" way. And both occasionally reveal different > upstream autotools usage bugs. Using DESTDIR is cleaner because the Makefile doesn't see the path of the buildroot prepended to the "libdir", "bindir" etc. variables, unlike with %makeinstall. Some badly-written Makefiles might write the values of these variables into configuration files or even the target binary itself, which would then not work properly when installed on a real system, outside the buildroot. e.g. Using DESTDIR, you have: bindir=/usr/bin DESTDIR=/path/to/buildroot binaries installed to /path/to/buildroot/usr/bin (OK) anything using $(bindir) in the Makefile sees the "correct" value Using %makeinstall, you have: bindir=/path/to/buildroot/usr/bin binaries installed to /path/to/buildroot/usr/bin (OK) anything using $(bindir) in the Makefile sees the "wrong" value Yes exactly right. In this case either one works fine so... Any other comments or is this good to go? This: %configure --disable-static Why --disable-static on a package that does not produce libraries? Just a habit and actully part of my standart specs... ;) maybe this should go but on the contrary => expansion of %configure has lots of unused stuff as well... Needs work: * Missing BR: libgnomeui-devel, libgtop2-devel * The package should contain a .desktop file (wiki: PackagingGuidelines#desktop) Minor: * Duplicate BuildRequires: zlib-devel (by libxml2-devel), atk-devel (by gtk2-devel), pango-devel (by gtk2-devel), libbonobo-devel (by libgnome-devel), ORBit2-devel (by libgnome-devel), libxml2-devel (by libgnome-devel), gtk2-devel (by libglade2-devel), libglade2-devel (by libgnomeui-devel), libgnome-devel (by libgnomeui-devel), libart_lgpl-devel (by libgnomeui-devel) You may clean that up if you want, but it's really minor. Here is a tuned version: http://fedora.lowlatency.de/review/synce-trayicon-0.9.0-2.src.rpm http://fedora.lowlatency.de/review/synce-trayicon.spec The package still needs a desktop file. Why did you switch from make install DESTDIR= to %makeinstall ? make install did not work but I was commented out in the first version as well wasn't it? http://fedora.lowlatency.de/review/synce-trayicon-0.9.0-3.src.rpm http://fedora.lowlatency.de/review/synce-trayicon.spec I would add these two lines to the desktop file : GenericName=Tray icon for SynCE devices Icon=synce/synce-color-small.png To have a better description and a nice icon. Apart from that, packaging looks good and the application works Review for release 3: * RPM name is OK * Source synce-trayicon-0.9.0.tar.gz is the same as upstream * Builds fine in mock * rpmlint of synce-trayicon looks OK * File list of synce-trayicon looks OK * Works fine APPROVED Please apply the improvements to the desktop file before requesting a build. imported, changed and build for fc5. THANKS as well for this one :) |