Bug 219930

(In reply to comment #0)
> SRPM URL: http://deadbabylon.de/fedora/extras/lxpanel/lxpanel-0.2.4-2.src.rpm

Error 404.

I took your spec and built the package, but there are some things it don't like:

- Source0: should not use a mirror, instead use 
dl.sourceforge.net/sourceforge/lxde/%{name}-%{version}.tar.gz

- BR startup-notification-devel?

- In the default config two starters (firefox and pcman-fm) are not working.
Please take a look at Chung-Yen's package at
ftp://cle.linux.org.tw/pub/fedora/cle/6/SRPMS/lxpanel-0.2.4-1.fc6.src.rpm
for a fix.

Some othe more thoughts: I think someone should also roll a pcman-fm package, we
should package the whole lx-desktop in extras. I have an icewm package that I
can offer. You and Chung-Yen should unite your powers on pcman-fm, lxpanel,
lxsession and lxmusic. What do you think?

As long as we don't have no pcman-fm package I suggest this starter should be
removed from the default config.

(In reply to comment #1)
> (In reply to comment #0)
> > SRPM URL: 
http://deadbabylon.de/fedora/extras/lxpanel/lxpanel-0.2.4-2.src.rpm
> 
> Error 404.

My fault. Fixed it.
 
> I took your spec and built the package, but there are some things it don't 
like:
> 
> - Source0: should not use a mirror, instead use 
> dl.sourceforge.net/sourceforge/lxde/%{name}-%{version}.tar.gz

Corrected.


> - BR startup-notification-devel?

Added.

> - In the default config two starters (firefox and pcman-fm) are not working.
> Please take a look at Chung-Yen's package at
> ftp://cle.linux.org.tw/pub/fedora/cle/6/SRPMS/lxpanel-0.2.4-1.fc6.src.rpm
> for a fix.

Added the patch.


> Some othe more thoughts: I think someone should also roll a pcman-fm 
package, we
> should package the whole lx-desktop in extras. I have an icewm package that 
I
> can offer. You and Chung-Yen should unite your powers on pcman-fm, lxpanel,
> lxsession and lxmusic. What do you think?

I didn't know that someone else ist working on it. I will ask him what he 
would think. 
Package the whole LXDE-Desktop could be interesting for others. I only use 
lxpanel for myself and have never used lxmusic. Will have a look on it.

> As long as we don't have no pcman-fm package I suggest this starter should 
be
> removed from the default config.

At first I've thought to replace pcmanfm.desktop with 
gnome-nautilus-home.desktop. But perhaps this could irritate some users when 
updating the package later and replace it again with pcmanfm.desktop.


For the moment:
SPEC Url: http://deadbabylon.de/fedora/extras/lxpanel/lxpanel.spec
SRPM Url: http://deadbabylon.de/fedora/extras/lxpanel/lxpanel-0.2.4-3.src.rpm

Changelog:
- BR: startup-notification-devel
- Added Patch1 from Chung-Yen to fix wrong starters in default config

Sebastian, have you heard something from Chung-Yen? If not, I'm going to review
this package tomorrow.

(In reply to comment #3)
> Sebastian, have you heard something from Chung-Yen? If not, I'm going to review
> this package tomorrow.

Sadly, I've heard nothing.

lxmusic needs xmms2 (but xmms2 is not included in FE), so I prefer not to include lxmusic
lxsession seems not work in FC6 (I have tried it before)
So, I think only lxpanel and pcmanfm in Extras is enough.

(In reply to comment #5)
> lxmusic needs xmms2 (but xmms2 is not included in FE), so I prefer not to
include lxmusic

Yeah, I already tried to patch it for bmp/audacious, but it was too much work
for only a tiny program.

> lxsession seems not work in FC6 (I have tried it before)
> So, I think only lxpanel and pcmanfm in Extras is enough.

So is it ok for you if Sebastian takes over lxpanel? You could submit your
pcmanfm package for review then, if you like.

REVIEW for 
80f9ae6864029fd3b0635e71f486c538  lxpanel-0.2.4-3.src.rpm

OK - rpmlint -i lxpanel-0.2.4-3.src.rpm 
W: lxpanel non-coherent-filename lxpanel-0.2.4-3.src.rpm
The file which contains the package should be named
<NAME>-<VERSION>-<RELEASE>.<ARCH>.rpm.

This is odd, I don't see no error here. Something ether is wrong with rpmlint or
with your build environment. I don't see this error when I rebuild the srpm
locally or in mock, so I choose to ignore it in this review, but can you send me
your .rpmmarcos file so we can investigate this further? Does the error go away,
when you rebuild the package once again locally?

OK - package and spec named according to the package naming guidelines
OK - package meets packaging guidelines
OK - license is open-source compatible, but COPYING looks dual licensed for me.
First part is a BSD like license, second GPLv2.
OK - Since GPL is more restrictive than BSD the whole package becomes GPL. So
the license field in the spec is ok.
OK - COPYING included in source and correctly installed in %doc
OK - spec is in American English
OK - spec is legible

MINOR NOTE - line warps in long fields like %description are usually done after
79 characters.

OK - source in srpm matches upstream by md5
37d0e9f2993fc63d9e7e1684552e10b4
        OK - package compiles and build into binaries on core 6 i386
OK - no known ExcludeArchs
OK - BuildRequires sane
OK - locales handled correctly
OK - no shared libs to worry about
OK - package is not relocatable
OK - package owns all directories it creates

MINOR NOTE - Instead of 
%dir %{_datadir}/lxpanel/
        %{_datadir}/lxpanel/*
        %dir %{_libdir}/lxpanel/
        %{_libdir}/lxpanel/*
        you could simply use
%{_datadir}/lxpanel/
        %{_libdir}/lxpanel/
        since you include everything in these directories anyway. The slashes at
the end of the line is only for humans to indicate its a dir.

OK - no duplicates in %files
OK - file permissions and %defattr correct
OK - valid clean section
OK - macro usage consistent
OK - code, not content
OK - no large docs
OK - docs don't affect runtime
OK - no header files, static libs or *.pc files
OK - no libtool archives
OK - IMO no desktop file is needed since it's panel and not what I call a
typical program/standalone application. 
OK - package doesn't own directories already owned by other files
OK - package builds in mock (devel)
OK - lxpanel works fine, but lxpanelctl is buggy. I can't add more starter
because the "Select Application"-Dialog doesn't list the files in
/usr/share/applications. Also hitting return in the location bar doesn't work.
Looking at src/plugins/launchbar.c I think this is a known issue (see the FIXME
in line 490) and isn't really meant to work atm.

So from a reviewers point of view everything looks fine except rpmlint. If the
rpmlint error on the srpm can be fixed with a simple rebuild build you can
consider this package to be APPROVED.

(In reply to comment #7)

> OK - rpmlint -i lxpanel-0.2.4-3.src.rpm 
> W: lxpanel non-coherent-filename lxpanel-0.2.4-3.src.rpm
> The file which contains the package should be named
> <NAME>-<VERSION>-<RELEASE>.<ARCH>.rpm.

Mhh. Strange. I can't remember which .rpmmacros I have used for this package. 
So I've uploaded a new version which is the result of the mock build. Could you 
test it for this issue (just to play safe this time)?

> First part is a BSD like license, second GPLv2.
> OK - Since GPL is more restrictive than BSD the whole package becomes GPL. So
> the license field in the spec is ok.

The GPL is also the license at gnomefiles.org:
http://www.gnomefiles.org/app.php/LXPanel


> MINOR NOTE - line warps in long fields like %description are usually done 
after
> 79 characters.

Fixed.

> MINOR NOTE - Instead of 
> %dir %{_datadir}/lxpanel/
>         %{_datadir}/lxpanel/*
>         %dir %{_libdir}/lxpanel/
>         %{_libdir}/lxpanel/*
>         you could simply use
> %{_datadir}/lxpanel/
>         %{_libdir}/lxpanel/

Ok. Fixed.

> OK - IMO no desktop file is needed since it's panel and not what I call a
> typical program/standalone application. 

Also think so. gnome-panel and kicker also have no desktop file.

> OK - lxpanel works fine, but lxpanelctl is buggy. I can't add more starter
> because the "Select Application"-Dialog doesn't list the files in
> /usr/share/applications. Also hitting return in the location bar doesn't 
work.
> Looking at src/plugins/launchbar.c I think this is a known issue (see the 
FIXME
> in line 490) and isn't really meant to work atm.

You're right. Seems to be already filed as a bug:
http://sourceforge.net/tracker/index.php?func=detail&aid=1623222&group_id=180858&atid=894869


SPEC Url: http://deadbabylon.de/fedora/extras/lxpanel/lxpanel.spec
SRPM Url: 
http://deadbabylon.de/fedora/extras/lxpanel/lxpanel-0.2.4-4.fc6.src.rpm

Changelog:
* Mon Jan 08 2007 Sebastian Vahl <fedora> - 0.2.4-4
- Fixed some minor issues from the review process (#219930)

(In reply to comment #8)

> So I've uploaded a new version which is the result of the mock build. Could you 
> test it for this issue (just to play safe this time)?

Looks fine, ACCEPTED.

Don't forget to change the default starters patch once we have a pcmanfm package.

One thine you may need to know:
the lxde author says he will no more develop and maintain these programs

So there is no NEXT version of lxde.
That is one reason why I don't want to maintain any of lxde packages.

(In reply to comment #10)
> One thine you may need to know:
> the lxde author says he will no more develop and maintain these programs
> 
> So there is no NEXT version of lxde.
> That is one reason why I don't want to maintain any of lxde packages.

Mhh. Not good. Didn't know that. I've written to the maintainer of lxpanel and
asked him about that. After I get an answer I will decide what to do with
lxpanel in extras.