Bug 461619
Summary: | Review Request: system-switch-displaymanager - A display manager switcher for GDM, KDM, XDM and WDM | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Than Ngo <than> |
Component: | Package Review | Assignee: | Karsten Hopp <karsten> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, karsten, notting, pahan, rdieter, ville.skytta |
Target Milestone: | --- | Flags: | karsten:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-12-13 21:36:25 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: |
Description
Than Ngo
2008-09-09 14:38:40 UTC
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 |