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 - Review Request: synce-trayicon
Summary: Review Request: synce-trayicon
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Aurelien Bompard
QA Contact: David Lawrence
URL: http://synce.sourceforge.net/
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-08-23 09:47 UTC by Andreas Bierfert
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2005-12-13 23:30:20 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Andreas Bierfert 2005-08-23 09:47:10 UTC
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 20:20:49 UTC
Hi Andreas, could you replace:

> make install DESTDIR=$RPM_BUILD_ROOT

with:

%makeinstall

?

Comment 2 Andreas Bierfert 2005-10-18 20:34:16 UTC
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 06:04:53 UTC
%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 07:49:05 UTC
(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 11:21:21 UTC
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 12:46:34 UTC
This:
%configure --disable-static

Why --disable-static on a package that does not produce libraries?

Comment 7 Andreas Bierfert 2005-10-19 12:51:45 UTC
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 15:35:29 UTC
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 17:27:25 UTC
The package still needs a desktop file.
Why did you switch from make install DESTDIR= to %makeinstall ?

Comment 11 Andreas Bierfert 2005-12-13 14:45:01 UTC
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 23:04:59 UTC
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 23:29:42 UTC
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.