Bug 452395

Robert, I'm CC'ing you beause I know you have an EeePC, so maybe you can help me
with this review... ;)

OK  | MUST: rpmlint is clean
OK  | MUST: The package must be named according to the Package…
OK  | MUST: The spec file name must match the base package…
OK  | MUST: The package must meet the Packaging Guidelines…
OK  | MUST: The package must be licensed with a Fedora approved…
OK  | MUST: The License field in the package spec file must. 
OK  | MUST: The spec file must be written in American English.
OK  | MUST: The package must successfully compile and build…
OK  | MUST: All build dependencies must be listed…
OK  | MUST: The spec file MUST handle locales properly…
OK  | MUST: Every binary RPM package which stores shared…
OK  | MUST: If the package is designed to be relocatable…
OK  | MUST: A package must own all directories that it creates
OK  | MUST: A package must not contain any duplicate files 
OK  | MUST: Permissions on files must be set properly. 
OK  | MUST: Each package must have a %clean section
OK  | MUST: Each package must consistently use macros
OK  | MUST: The package must contain code, or permissible 
N/A | MUST: Large documentation files should go in a -doc 
OK  | MUST: If a package includes something as %doc…
N/A | MUST: Header files must be in a -devel package.
N/A | MUST: Static libraries must be in a -static package.
N/A | MUST: Packages containing pkgconfig(.pc) files must…
N/A | MUST: If a package contains library files with a suffix…
N/A | MUST: In the vast majority of cases, devel packages must…
OK  | MUST: Packages must NOT contain any .la libtool archives, 
NOK | MUST: Packages containing GUI applications must include…

Shouldn't there be a .desktop file? How am I suppose to test the functionality
of this software or launch it?

NOK  | MUST: Packages must not own files or directories already

Why are you claiming ownership of 
%{_datadir}/desktop-directories/*.directory?

These are owned by redhat-menus package already. 


OK  | MUST: At the beginning of %install, each package MUST…
OK  | MUST: All filenames in rpm packages must be valid UTF-8.
OK  | SHOULD: If the source package does not include license 
-   | SHOULD: The description and summary section … translations…
OK  | SHOULD: The package builds in mock
-   | SHOULD: The package builds on all supported architectures
OK  | SHOULD: The reviewer should test that the package…
N/A | SHOULD: If scriptlets are used, those scriptlets must be sane…
N/A | SHOULD: Subpackages other than devel should usually require base…
N/A | SHOULD: The placement of pkgconfig(.pc) files depends on…
OK | SHOULD: If the package has file dependencies outside of shortlist…
OK  | MUST: All build dependencies must be listed…

Robert, do you want to do the official review? 

(In reply to comment #2)

>
> Shouldn't there be a .desktop file? How am I suppose to test the functionality
> of this software or launch it?

By simply running it from a terminal?? Works fine in _all_ desktops I've seen.

lxlauncher is supposed to be started by lxsession or whatever and running all
the time. Think of something like xfdesktop, it has no launcher ether. I could
however add a launcher to /etc/xdg/autostart, but this should IMO be disabled by
default.

My plan is to add a special EeePC Session to lxsession-lite in which lxlauchner
will be started automatically.

> NOK  | MUST: Packages must not own files or directories already
> 
> Why are you claiming ownership of 
> %{_datadir}/desktop-directories/*.directory?
> 
> These are owned by redhat-menus package already. 

AFAICS I'm not owning any other files but my own, 
%{_datadir}/desktop-directories/*.directory seems correct:
$ rpm -qf /usr/share/desktop-directories/
filesystem-2.4.13-1.fc9.i386
gnome-menus-2.22.2-1.fc9.i386
$ rpm -ql lxlauncher
/usr/bin/lxlauncher
/usr/share/desktop-directories/Learn.directory
/usr/share/desktop-directories/Math.directory
/usr/share/desktop-directories/Play.directory
/usr/share/desktop-directories/Science.directory
/usr/share/desktop-directories/Work.directory
/usr/share/doc/lxlauncher-0.2
...

The package drops 5 more Menus in there and so far the names do not conflikt are
not in redhat-menus or in gnome-menus, so there is no collision. Nevertheless I
can install them prefixed with lxde-* or alike to avoid problems long term.

> By simply running it from a terminal?? Works fine in _all_ desktops I've seen.
> 
> lxlauncher is supposed to be started by lxsession or whatever and running all
> the time. Think of something like xfdesktop, it has no launcher ether. I could
> however add a launcher to /etc/xdg/autostart, but this should IMO be disabled 
> by default.

I didn't yet have a look to xfdesktop or equivalent things, but from my current
point of view, I would agree with you.

> AFAICS I'm not owning any other files but my own, 
> %{_datadir}/desktop-directories/*.directory seems correct:

For me, it seems also correct what is currently owned. %{_datadir}/desktop-
directories/*.directory refers to all *.directory files coming from the "make 
install DESTDIR=$RPM_BUILD_ROOT". Rahul, please clarify.

> The package drops 5 more Menus in there and so far the names do not conflikt 
> are not in redhat-menus or in gnome-menus, so there is no collision. 
> Nevertheless I can install them prefixed with lxde-* or alike to avoid 
> problems long term.

I would see prefixing as optional SHOULD, this is IMHO up to you.

I would prefer a launcher. I don't have a eeepc with me to test right now but
running it on the command seems to just hang in GNOME

# lxlauncher

I don't see anything after this. 

Yes, a prefix would be good to avoid conflicts. The ownership otherwise seems
correct. 

Robert Scheck. this review is still officially assigned to you. 

Just a quick note that I figured out how lxlauncher works. 

So do you still think it needs a launcher? IMO a launcher in the menu is
nonsens, nobody will launch a lauchner from the menu just to launch another app.

The only thing that makes sense to me is /etc/xdg/autostart, but I'm not sure
how to handle that for all desktops/window managers, because some do support
/etc/xdg/autostart but do not implement "Hidden=true".

Okay, let's do the official review: I checked all my points which are mostly 
equivalent to that ones already mentioned, so I'll not re-post my list. From 
my side I've no complaints at all.

> NOK | MUST: Packages containing GUI applications must include…

I agree with the requestor that no *.desktop is required, doesn't make sense
here. Maybe the hint for the autostart can be put into a README-FEDORA or a 
similar file, but per default enabled when maybe some/not all desktops/window 
managers are supporting it, is not ideal.

> NOK  | MUST: Packages must not own files or directories already

I can't see any issue here, the result in the built RPM package looks fine.


APPROVED.

New Package CVS Request
=======================
Package Name: lxlauncher
Short Description: Open source replacement for Asus Launcher of the EeePC
Owners: cwickert
Branches: F-8 F-9
InitialCC: 
Cvsextras Commits: yes

cvs done.

lxlauncher-0.2-1.fc9 has been submitted as an update for Fedora 9

lxlauncher-0.2-1.fc8 has been submitted as an update for Fedora 8

Thanks for the review, Robert. Closing.

lxlauncher-0.2-1.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

lxlauncher-0.2-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: lxlauncher
New Branches: EL-4 EL-5
Owners: cwickert

cvs done.