Bug 486698

Summary: Review Request: fedora-setup-keyboard - Hal keyboard layout callout
Product: [Fedora] Fedora Reporter: Adel Gadllah <adel.gadllah>
Component: Package ReviewAssignee: Jens Petersen <petersen>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: che666, fedora-package-review, i18n-bugs, notting, peter.hutterer, petersen
Target Milestone: ---Flags: petersen: 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: 2009-03-03 22:45:02 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 483817    

Description Adel Gadllah 2009-02-21 03:01:48 EST
Spec URL: http://193.200.113.196/apache2-default/rpm/fedora-setup-keyboard.spec
SRPM URL: http://193.200.113.196/apache2-default/rpm/fedora-setup-keyboard-0.2-1.fc10.src.rpm
Description: 
fedora-setup-keyboard gets invoked by hal to apply the keyboard layout
defined in /etc/sysconfig/keyboard.


rpmlint output:
fedora-setup-keyboard.x86_64: W: no-documentation
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

(There are no docs)

koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1144341

NOTE: The files in this packages conflict with the ones shipped in xorg-x11-server-Xorg. This is intentional, it is going to replace them (X will than depend on this package).
Comment 1 Rudolf Kastl 2009-02-23 04:18:35 EST
why doesent it obsolete the package if it is going to replace it?
Comment 2 Adel Gadllah 2009-02-23 08:14:09 EST
(In reply to comment #1)
> why doesent it obsolete the package if it is going to replace it?

Because it doesn't. It does replace FILES shipped in the xserver package.
Comment 3 Jens Petersen 2009-02-24 18:58:49 EST
I think the package should probably "Requires: hal" for the %{_datadir}/hal/fdi/policy/10osvendor directory.

A license file is really required too.
Comment 4 Jens Petersen 2009-02-24 19:01:10 EST
Otherwise rpmlint output is clean - a license file would also get rid of the no documentation warning. :)
Comment 5 Jens Petersen 2009-02-24 19:05:07 EST
> NOTE: The files in this packages conflict with the ones shipped in
> xorg-x11-server-Xorg. This is intentional, it is going to replace them (X will
> than depend on this package).

It might be worth adding a "Conflicts: xorg-x11-server-Xorg < (last fedora-setup-keyboard.py version-(release+1))" for this.
Comment 6 Adel Gadllah 2009-02-25 13:01:05 EST
(In reply to comment #5)
> > NOTE: The files in this packages conflict with the ones shipped in
> > xorg-x11-server-Xorg. This is intentional, it is going to replace them (X will
> > than depend on this package).
> 
> It might be worth adding a "Conflicts: xorg-x11-server-Xorg < (last
> fedora-setup-keyboard.py version-(release+1))" for this.

Well can't do this unless the xorg package is changed to use this fedora-setup-keyboard. (As xorg builds might be made during this review), besides it already conflicts with this packaged via file conflict.

Fixed the other 2 issues (license file and hal requirement)

http://193.200.113.196/apache2-default/rpm/fedora-setup-keyboard.spec
http://193.200.113.196/apache2-default/rpm/fedora-setup-keyboard-0.3-1.fc10.src.rpm
Comment 7 Peter Hutterer 2009-02-25 17:53:19 EST
I'll add the conflicts line once the package is available and the xorg-x11-server package got its requires on it.
Comment 8 Jens Petersen 2009-03-01 19:23:12 EST
Apologies for the slow response - somehow missed your updates in my bugzilla folder last week.

Thanks for the update.  Here is my review:

 +:ok, !:needs fixing

MUST Items:
[+] MUST: rpmlint output

rpmlint is now clean. :)

[*] MUST: Package Naming Guidelines
[+] MUST: spec file name must match base package %{name}
[+] MUST: Packaging Guidelines.
[+] MUST: Licensing Guidelines
[!] MUST: License field in the package spec file must match actual license.

The license is MIT not BSD!

[+] MUST: include license files in %doc if available in source
[+] MUST: The spec file must be written in American English and be legible.
[+] MUST: source md5sum matches upstream release

6bcb3d6a9f31eddd69aac5df3b50dd98  fedora-setup-keyboard-0.3.tar.bz2

[+] MUST: must successfully compile and build into binary rpms on one main arch
[+] MUST: All build dependencies must be listed in BuildRequires
[no shared libs]
[+] MUST: A package must own all directories that it creates.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros
[+] MUST: The package must contain code, or permissable content.
[+] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[no devel files]
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[+] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[+] SHOULD: The reviewer should test that the package builds in mock.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1212001

[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.


Please be sure to fix the License field before importing and
the package is APPROVED for inclusion in Fedora.
Comment 9 Adel Gadllah 2009-03-02 02:19:30 EST
> Please be sure to fix the License field before importing and
> the package is APPROVED for inclusion in Fedora.

Fixed in 0.3-2, thanks for the review.
Comment 10 Adel Gadllah 2009-03-02 02:21:13 EST
New Package CVS Request
=======================
Package Name: fedora-setup-keyboard
Short Description: Hal keyboard layout callout
Owners: drago01, whot
Branches: F-10
InitialCC:
Comment 11 Kevin Fenzi 2009-03-02 19:36:18 EST
cvs done.