Bug 166551

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 :)