Bug 226443

Summary: Merge Review: switchdesk
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Susi Lehtola <susi.lehtola>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bashton, susi.lehtola, than
Target Milestone: ---Flags: susi.lehtola: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-08-18 20:24:07 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 Nobody's working on this, feel free to take it 2007-01-31 21:03:11 UTC
Fedora Merge Review: switchdesk

http://cvs.fedora.redhat.com/viewcvs/devel/switchdesk/
Initial Owner: than

Comment 1 Susi Lehtola 2009-03-21 22:40:07 UTC
rpmlint output:
switchdesk.noarch: W: summary-ended-with-dot A desktop environment switcher for GNOME, KDE and AnotherLevel.
switchdesk.noarch: E: tag-not-utf8 %changelog
switchdesk.noarch: W: no-url-tag
switchdesk.src: E: non-utf8-spec-file /tmp/switchdesk-4.0.9-4.fc10.1.src.rpm.25649/switchdesk.spec
switchdesk.src:31: W: unversioned-explicit-obsoletes %{name}-kde
switchdesk.src:32: W: unversioned-explicit-obsoletes %{name}-gnome
switchdesk.src:33: W: unversioned-explicit-provides %{name}-kde
switchdesk.src:34: W: unversioned-explicit-provides %{name}-gnome
switchdesk.src:187: W: macro-in-%changelog _datadir
switchdesk.src: W: summary-ended-with-dot A desktop environment switcher for GNOME, KDE and AnotherLevel.
switchdesk.src: E: tag-not-utf8 %changelog
switchdesk.src: W: no-url-tag
switchdesk-gui.noarch: W: no-documentation
switchdesk-gui.noarch: E: script-without-shebang /usr/share/switchdesk/backend.py
switchdesk-gui.noarch: W: summary-ended-with-dot A graphical interface for the Desktop Switcher.
switchdesk-gui.noarch: E: tag-not-utf8 %changelog
switchdesk-gui.noarch: W: no-url-tag
3 packages and 0 specfiles checked; 5 errors, 12 warnings.

- Need to fix the above errors & warnings.
 * Summary is inexact: switchdesk also works for Fluxbox, FWVM, Icewm etc. Maybe generalize it to "A desktop environment switcher"? Fix also description.
 * Since url is missing and we are upstream add disclaimer from http://fedoraproject.org/wiki/Packaging/SourceURL#We_are_Upstream

- Missing %doc: AUTHORS COPYING Changelog

- Enable smp make or add note if it doesn't work.

- Remove blank space from end of spec file.

Comment 2 Susi Lehtola 2009-05-20 08:45:05 UTC
ping??

Comment 3 Susi Lehtola 2009-06-14 16:14:03 UTC
ping than

Comment 4 Than Ngo 2010-07-05 11:12:39 UTC
working on this

Comment 5 Than Ngo 2010-07-07 12:33:13 UTC
all obove issues are fixed in switchdesk-4.0.9-6. could you please check it?

thanks

Comment 6 Susi Lehtola 2010-07-07 12:56:50 UTC
rpmlint output:
switchdesk.noarch: W: no-manual-page-for-binary switchdesk-helper
switchdesk-gui.noarch: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

- Consider adding INSTALL="install -p" to make install, to preserve time stamps.

- Source URL is incorrect.

- There is python stuff installed, so I think you should BuildRequires: python-devel.


Full review as follows:

MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSWORK

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. OK
MUST: Optflags are used and time stamps preserved. ~OK
MUST: Packages containing shared library files must call ldconfig. N/A

MUST: A package must own all directories that it creates or require the package that owns the directory. ~OK
- You're using "switchdesk" in the main %files, but "%{name}" in -gui. This is a bit illogical, please consider unifying the use.

MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. N/A
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
- Maybe add ChangeLog?

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 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. NEEDSWORK
- Must run e.g.
desktop-file-validate %{buildroot}/%{_datadir}/applications/foo.desktop

MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Comment 7 Than Ngo 2010-07-07 14:15:58 UTC
(In reply to comment #6)
> rpmlint output:
> switchdesk.noarch: W: no-manual-page-for-binary switchdesk-helper
this binany is backend and the user should not launch it. no manual page is needed. It's OK
> switchdesk-gui.noarch: W: no-documentation
there's no documentation atm, but i don't think it's a blocker for this review

> 3 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> - Consider adding INSTALL="install -p" to make install, to preserve time
> stamps.
> 
> - Source URL is incorrect.
> 
> - There is python stuff installed, so I think you should BuildRequires:
> python-devel.
> 
> 
> Full review as follows:
> 
> MUST: The spec file for the package is legible and macros are used
> consistently. OK
> MUST: The package must be named according to the Package Naming Guidelines. OK
> MUST: The spec file name must match the base package %{name}. OK
> MUST: The package must be licensed with a Fedora approved license and meet the 
> Licensing Guidelines. OK
> MUST: The License field in the package spec file must match the actual license.
> OK
> 
> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. NEEDSWORK
> 
> MUST: The package MUST successfully compile and build into binary rpms. OK
> MUST: The spec file MUST handle locales properly. OK
> MUST: Optflags are used and time stamps preserved. ~OK
> MUST: Packages containing shared library files must call ldconfig. N/A
> 
> MUST: A package must own all directories that it creates or require the package
> that owns the directory. ~OK
> - You're using "switchdesk" in the main %files, but "%{name}" in -gui. This is
> a bit illogical, please consider unifying the use.
> 
> MUST: Files only listed once in %files listings. OK
> MUST: Debuginfo package is complete. N/A
> MUST: Permissions on files must be set properly. OK
> MUST: Clean section exists. OK
> MUST: Large documentation files must go in a -doc subpackage. N/A
> 
> MUST: All relevant items are included in %doc. Items in %doc do not affect
> runtime of application. OK
> - Maybe add ChangeLog?
> 
> 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 'Requires: pkgconfig'. N/A
> MUST: If a package contains library files with a suffix then library files
> ending in .so must go in a -devel package. N/A
> MUST: In the vast majority of cases, devel packages must require the base
> package using a fully versioned dependency. N/A
> MUST: Packages does not contain any .la libtool archives. N/A
> 
> MUST: Desktop files are installed properly. NEEDSWORK
> - Must run e.g.
> desktop-file-validate %{buildroot}/%{_datadir}/applications/foo.desktop
> 
> MUST: No file conflicts with other packages and no general names. OK
> MUST: Buildroot cleaned before install. OK
> SHOULD: %{?dist} tag is used in release. OK
> SHOULD: If the package does not include license text(s) as separate files from
> upstream, the packager should query upstream to include it. OK
> SHOULD: The package builds in mock. OK    


fixed in switchdesk-4.0.9-7.fc13, you can download it from http://than.fedorapeople.org/

Comment 8 Susi Lehtola 2010-07-07 15:12:50 UTC
MUST:
- Desktop-file-validate must be run in %install, not in %check. See
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage

- The source URL works now, but is this a long-term solution? The sources should be moved to fedorahosted.org.

SHOULD:
- I usually like a bit more verbosity in %files; statements such as 
 %{_bindir}/*
and
 %{_mandir}/man1/*
can be a bit dangerous. You can easily fix this by changing them into
 %{_bindir}/%{name}*
and
 %{_mandir}/man1/%{name}*


The remaining issues are cosmetic. Please fix at least the MUST items before a cvs push. This package has been

APPROVED

Comment 9 Than Ngo 2010-07-07 15:53:21 UTC
(In reply to comment #8)
> MUST:
> - Desktop-file-validate must be run in %install, not in %check. See
> http://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
> 
it's fixed in switchdesk-4.0.9-8.fc14

> - The source URL works now, but is this a long-term solution? The sources
> should be moved to fedorahosted.org.
> 

no, it will be moved to fedorahosted.org

> SHOULD:
> - I usually like a bit more verbosity in %files; statements such as 
>  %{_bindir}/*
> and
>  %{_mandir}/man1/*
> can be a bit dangerous. You can easily fix this by changing them into
>  %{_bindir}/%{name}*
> and
>  %{_mandir}/man1/%{name}*
> 
> 
> The remaining issues are cosmetic. Please fix at least the MUST items before a
> cvs push. This package has been
> 
> APPROVED    

fixed in 4.0.9-8.fc14

Comment 10 Susi Lehtola 2010-08-18 20:24:07 UTC
OK, the project (switchdesk) exists now in fedoraproject.org, but there's nothing there in the repo.

Anyway, I think we can close this now.