Bug 461619

neat.  A few initial comments.

0.  a koji scratch build would be nice. :)
1.  Use desktop-file-install for the .desktop file(s)
2.  fix License tag (ie, GPL is not valid per http://fedorproject.org/wiki/Licensing)
3.   -gui subpkg (pygtk2?, eww...) anyway, shouldn't it have additional deps, maybe something like:
Requires: pygtk2
4.  Source url, if there's no current upstream for this, please add a comment in the specfile saying so.  Long-term, would be nice to get this hosted somewhere, maybe fedorahosted?

Pardon, but switchdesk package is not same do?

>Pardon, but switchdesk package is not same do?
no, it's not the same. switchdesk only allows to switch the desktop enviroment but not the graphical X login. The switch-displaymanager is the tool to allow user to switch it.

>0.  a koji scratch build would be nice. :)
i will do today

>1.  Use desktop-file-install for the .desktop file(s)
it's fixed

>2.  fix License tag (ie, GPL is not valid per
http://fedorproject.org/wiki/Licensing)
it's fixed

>3.   -gui subpkg (pygtk2?, eww...) anyway, shouldn't it have additional deps,
>maybe something like:
>Requires: pygtk2
this requires on pygtk2 was already included in this version. I intend to write this tool in KDE4 in next version so it makes sense to rename -gui to -gnome now.

>4.  Source url, if there's no current upstream for this, please add a comment
>in the specfile saying so.  Long-term, would be nice to get this hosted
>somewhere, maybe fedorahosted?

i'm upstreamer, will upload it on fedorahosted

I leave to OK stuff from the review guidelines away and just list the bad ones:
BAD: should use macros when possible, p.e. %{_sysconfdir} for /etc
BAD: doesn't build in mock:
make -C po update-po
ls:
cannot access *.po
: No such file or directory

BAD: rpmlint warning for switch-displaymanager-1.0-1.noarch.rpm:
switch-displaymanager.noarch: W: no-dependency-on usermode

BAD: rpmlint warning for switch-displaymanager-gnome-1.0-1.noarch.rpm:
switch-displaymanager-gnome.noarch: E: description-line-too-long The switch-displaymanager-gnome package provides the GNOME graphical user interface

BAD: buildroot isn't the most preferred one: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

BAD: http://than.fedorapeople.org/switch-displaymanager.spec doesn't use desktop-file-install (comment #1)

BAD: unversioned requirement in the gnome subpackage on the main package

BAD: packages don't have license text included  (%doc)

BAD: errors when switch-displaymanager-gnome is run the first time:
> switch-displaymanager
/bin/cp: cannot stat `/etc/sysconfig/desktop': No such file or directory
/bin/cat: /etc/sysconfig/desktop.save: No such file or directory
Your default graphical display manager has successfully been switched

BAD: errors when switch-displaymanager-gnome is not installed and the commandline is used:
>switch-displaymanager KDE
/usr/sbin/switch-displaymanager: line 16: /usr/share/switch-displaymanager/switchdesk-helper: No such file or directory
/usr/sbin/switch-displaymanager: line 16: exec: /usr/share/switch-displaymanager/switchdesk-helper: cannot execute: No such file or directory


NOTE: man page from Mon Feb 14 2000 for a newly written program ?
NOTE: doesn't use smp flags during build, probably not needed because package contains only scripts

i upload new switch-displaymanager-1.0-1.src.rpm and specfile that fixes the above issues. Could you please check again? Thanks

Please increment Release and include changelogs when rev'ing the pkg, even for reviews.

I (still) don't see 'desktop-file-install' being used, or any scratch build yet. :)

One more smallish suggestion: add to -gnome subpkg
Provides: %{name}-gui = %{version}-%{release}

Once all those are done, I don't anticipate any further blockers (pending my tests that it funciontally works ok).

nevermind, looks like Karsten already grabbed the review (sorry, didn't notice), I'll leave the rest up to him. :)

Sorry, forgot to set the review flag to ?


desktop-file-install is used in the makefile during install.

looks ok now, only one warning because switch-displaymanager-gnome has no
documentation, the rest is fixed


New Package CVS Request
=======================
Package Name: switch-displaymanager
Short Description: A display manager switcher for GDM, KDM, XDM and WDM
Owners: than
Branches: F-9 F-10
InitialCC:

Just in case the name is an oversight, aren't tools like this traditionally called system-switch-* (not switch-*)?

yes, it should be renamed to system-switch-displaymanager.

New Package CVS Request
=======================
Package Name: system-switch-displaymanager
Short Description: A display manager switcher for GDM, KDM, XDM and WDM
Owners: than
Branches: F-9 F-10
InitialCC:

cvs done.

it's build in f9/f10