First comment ahead of formal review.  The package contains a mismash of different licensed content.

Since the binary package includes shell script executable named setupcon which from the COPYRIGHT file and from examination of the script itself is licensed under MIT.

GPLv2+ and MIT and Public Domain  seems to be most appropriate for the spec license field.

-jef

Summary:
Fix items 1 and 2 below and I'll be able to accept it.

Items that need to be addressed:

1) License field needs to be updated to read
GPLv2+ and MIT and Public Domain  due to license on the setupcon script.
This is a blocker

2) No need to for the package to own /etc/default
/etc/default is owned by glibc  which is part of the requirement chain through the requirement on the kdb package. All other directory ownership looks good.
This is a blocker

Other items
3) rpmlint against the rpm has one E associated with incorrect FSF address from the ckbcomp utility boilerplate. You are encouraged to notify the upstream for this package to fix it, and you can correct it with a patch in your package..but are not required to do so.

4) The supplied manpages still refer to /usr/local/etc/default instead of /etc/default/ in multiple place. See if you can enhance the paths patch to fix that.

5) I dont understand the notes in the spec concerning xkeyboard-config as a possible requirement. Can you explain to me what the problem is in a little more detail? Not a blocker, but I don't have enough info to make a recomendation.

6)
setupcon -v  gives me some interesting feedback concerning keymap deny errors for the loadkeys operation which suggests to me that the loadkeys operation its not working as expected. Not sure what's going on there..but its not falling over with a crash condition. You want to sort this out as part of this review?
I can supply the output on my F16 system.


Other Must checklist items:
rpmlint: console-setup-1.76-1.fc16.noarch.rpm 
console-setup.noarch: W: spelling-error Summary(en_US) keymaps -> key maps, key-maps, makeups
console-setup.noarch: E: incorrect-fsf-address /usr/bin/ckbcomp


source checksum verifies
upstream checksum is:
38cbc433c40d80f164097e7acaf57b3e  console-setup_1.76.tar.gz

applied patch to fix up file paths seems reasonable.  There might be a more clever way to do that patch, but that's not a blocker for inclusion.

package naming looks good according to guidelines.

Compiles cleanly on F16 and against koji scratch build

no locale files
no libraries
no devel files
no large docs
docs looks good
 
spec file is legible and has consistent use of macros.

setupcon  appears to run without faulting on a console tty

(In reply to comment #2)

Thanks Jef, I updated the files in
http://vcrhonek.fedorapeople.org/console-setup/

> Summary:
> Fix items 1 and 2 below and I'll be able to accept it.
> 
> Items that need to be addressed:
> 
> 1) License field needs to be updated to read
> GPLv2+ and MIT and Public Domain  due to license on the setupcon script.
> This is a blocker

Fixed.

> 
> 2) No need to for the package to own /etc/default
> /etc/default is owned by glibc  which is part of the requirement chain
> through the requirement on the kdb package. All other directory ownership
> looks good.
> This is a blocker

Fixed.

> 
> Other items
> 3) rpmlint against the rpm has one E associated with incorrect FSF address
> from the ckbcomp utility boilerplate. You are encouraged to notify the
> upstream for this package to fix it, and you can correct it with a patch in
> your package..but are not required to do so.

Fixed and upstream contacted.

> 
> 4) The supplied manpages still refer to /usr/local/etc/default instead of
> /etc/default/ in multiple place. See if you can enhance the paths patch to
> fix that.

Fixed. I also reviewed the manpages and add paths to consolefonts/consoletrans/keymaps shipped within kbd package.

> 
> 5) I dont understand the notes in the spec concerning xkeyboard-config as a
> possible requirement. Can you explain to me what the problem is in a little
> more detail? Not a blocker, but I don't have enough info to make a
> recomendation.

Main purpose of adding this package is to unify keymaps between X Window System and text console, see:
https://bugzilla.redhat.com/show_bug.cgi?id=680990

X Window System keymaps are shipped in xkeyboard-config package and console-setup should use it by default to achieve the goal.

Upstream tarball of console-setup contains additional set of X Window System keymaps, which I removed in spec file (ckbcomp would prefer them in another case), but maybe they could be useful for someone, so we can consider to have it in optional subpackage.

> 
> 6)
> setupcon -v  gives me some interesting feedback concerning keymap deny
> errors for the loadkeys operation which suggests to me that the loadkeys
> operation its not working as expected. Not sure what's going on there..but
> its not falling over with a crash condition. You want to sort this out as
> part of this review?
> I can supply the output on my F16 system.

Sure, please let me see the output. Works fine for me with default configuration on F17 system:

# setupcon -v
Can not find the active virtual consoles, assuming ACTIVE_CONSOLES="/dev/tty1 /dev/tty2 /dev/tty3 /dev/tty4 /dev/tty5 /dev/tty6"
The charmap is UTF-8
BackSpace is ^?
Executing utf_start /dev/tty1.
Configuring /dev/tty1 in Unicode mode.
Executing utf_start /dev/tty2.
Configuring /dev/tty2 in Unicode mode.
Executing utf_start /dev/tty3.
Configuring /dev/tty3 in Unicode mode.
Executing utf_start /dev/tty4.
Configuring /dev/tty4 in Unicode mode.
Executing utf_start /dev/tty5.
Configuring /dev/tty5 in Unicode mode.
Executing utf_start /dev/tty6.
Configuring /dev/tty6 in Unicode mode.
Loading 512-char 8x16 font from file /usr/share/consolefonts/Uni2-TerminusBold16.psf.gz
Loading Unicode mapping table...
setfont: graphics console /dev/tty2 skipped
Loading 512-char 8x16 font from file /usr/share/consolefonts/Uni2-TerminusBold16.psf.gz
Loading Unicode mapping table...
Loading 512-char 8x16 font from file /usr/share/consolefonts/Uni2-TerminusBold16.psf.gz
Loading Unicode mapping table...
Loading 512-char 8x16 font from file /usr/share/consolefonts/Uni2-TerminusBold16.psf.gz
Loading Unicode mapping table...
Loading 512-char 8x16 font from file /usr/share/consolefonts/Uni2-TerminusBold16.psf.gz
Loading Unicode mapping table...
For /dev/tty1:
Executing kbd_mode -u.
For /dev/tty2:
Executing kbd_mode -u.
For /dev/tty3:
Executing kbd_mode -u.
For /dev/tty4:
Executing kbd_mode -u.
For /dev/tty5:
Executing kbd_mode -u.
For /dev/tty6:
Executing kbd_mode -u.
Executing loadkeys /tmp/tmpkbd.Iyr2Zn.
Loading /tmp/tmpkbd.Iyr2Z

> 
> 
> Other Must checklist items:
> rpmlint: console-setup-1.76-1.fc16.noarch.rpm 
> console-setup.noarch: W: spelling-error Summary(en_US) keymaps -> key maps,
> key-maps, makeups
> console-setup.noarch: E: incorrect-fsf-address /usr/bin/ckbcomp

I changed it to key maps to make rpmlint happy, it probably doesn't matter.

> 
> 
> source checksum verifies
> upstream checksum is:
> 38cbc433c40d80f164097e7acaf57b3e  console-setup_1.76.tar.gz
> 
> applied patch to fix up file paths seems reasonable.  There might be a more
> clever way to do that patch, but that's not a blocker for inclusion.
> 
> package naming looks good according to guidelines.
> 
> Compiles cleanly on F16 and against koji scratch build
> 
> no locale files
> no libraries
> no devel files
> no large docs
> docs looks good
>  
> spec file is legible and has consistent use of macros.
> 
> setupcon  appears to run without faulting on a console tty

Okay changes look good.

Package Approved

Followup on the setupcon script


Running setupcon -v  as root seems to run fine.

Running setupcon -v as user throws the loadkey errors.

Reading the manpage and the README in the docs/ its not clear that setupcon is suppose to be a privledged operation.  See attached output from setupcon -v while as user

Created attachment 594637 [details]
stderr output of setupcon -v   run as normal user

Hey, just a heads up it seems you might want to talk with the systemd-devels about console-setup integration with systemd.

Relevant message from relevant thread:
http://lists.freedesktop.org/archives/systemd-devel/2012-June/005688.html

New Package SCM Request
=======================
Package Name: console-setup
Short Description: Tools for configuring the console using X Window System keymaps
Owners: vcrhonek
Branches: f16 f17
InitialCC:

Git done (by process-git-requests).

(In reply to comment #5)
> Followup on the setupcon script
> 
> 
> Running setupcon -v  as root seems to run fine.
> 
> Running setupcon -v as user throws the loadkey errors.
> 
> Reading the manpage and the README in the docs/ its not clear that setupcon
> is suppose to be a privledged operation.  See attached output from setupcon
> -v while as user

In most cases the setupcon script should be invoked by privileged user (because loading new keymap with "loadkeys" requires it), but regular user can use it too with certain options (e.g. "setupcon -f -v" will just change font on users current tty).

(In reply to comment #10)

Yeah that's what I figured, but I didn't read anything in any of the obvious documentation about typical privledge operation. So just trying to be sure I was understanding what I was seeing.


-jef