Bug 166551 - Review Request: synce-trayicon
Review Request: synce-trayicon
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Aurelien Bompard
David Lawrence
http://synce.sourceforge.net/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-23 05:47 EDT by Andreas Bierfert
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-12-13 18:30:20 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Andreas Bierfert 2005-08-23 05:47:10 EDT
Spec Name or Url: http://fedora.lowlatency.de/review/synce-trayicon.spec
SRPM Name or Url: http://fedora.lowlatency.de/review/synce-trayicon-0.9.0-1.src.rpm
Description:
Tray icon for use with gnome and synce
Comment 1 Linus Walleij 2005-10-18 16:20:49 EDT
Hi Andreas, could you replace:

> make install DESTDIR=$RPM_BUILD_ROOT

with:

%makeinstall

?
Comment 2 Andreas Bierfert 2005-10-18 16:34:16 EDT
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.
Comment 3 Ville Skyttä 2005-10-19 02:04:53 EDT
%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. 
Comment 4 Paul Howarth 2005-10-19 03:49:05 EDT
(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
Comment 5 Andreas Bierfert 2005-10-19 07:21:21 EDT
Yes exactly right. In this case either one works fine so...

Any other comments or is this good to go?
Comment 6 Linus Walleij 2005-10-19 08:46:34 EDT
This:
%configure --disable-static

Why --disable-static on a package that does not produce libraries?
Comment 7 Andreas Bierfert 2005-10-19 08:51:45 EDT
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...
Comment 8 Aurelien Bompard 2005-12-12 10:35:29 EST
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.
Comment 10 Aurelien Bompard 2005-12-12 12:27:25 EST
The package still needs a desktop file.
Why did you switch from make install DESTDIR= to %makeinstall ?
Comment 11 Andreas Bierfert 2005-12-13 09:45:01 EST
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
Comment 12 Aurelien Bompard 2005-12-13 18:04:59 EST
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.
Comment 13 Andreas Bierfert 2005-12-13 18:29:42 EST
imported, changed and build for fc5.

THANKS as well for this one :)

Note You need to log in before you can comment on or make changes to this bug.