Spec URL: http://than.fedorapeople.org/switch-displaymanager.spec SRPM URL: http://than.fedorapeople.org/switch-displaymanager-1.0-1.src.rpm Description: The Display Manager Switcher is a tool which enables users to easily switch between various deskplay managers that they have installed. The tool includes support for GDM, KDM, XDM and WDM. Install switch-displaymanager if you need a tool for switching between display managers.
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