Bug 219930
Summary: | Review Request: lxpanel - A lightweight X11 desktop panel | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Sebastian Vahl <fedora> |
Component: | Package Review | Assignee: | Christoph Wickert <christoph.wickert> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | candyz0416, christoph.wickert |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-01-10 13:45:34 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: | 505781 |
Description
Sebastian Vahl
2006-12-17 00:04:53 UTC
(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. |