[newui] Replace system-config-keyboard and remove our loadKeymap code

[Vratislav Podzimek]

PATCH 1/2 replaces the anaconda.keyboard object, which was a Keyboard object
from system-config-keyboard. The only method that was used was the activate()
method, that basically called loadkeys and setxkbmap. We now use libxklavier
to handle X layouts and we can call loadkeys ourselves. Once systemd-localed
has the capability to load X layouts in the virtual console, we could easily
replace calling loadkeys with calling systemd-localed methods via DBus.

The only problem is, that system-config-keyboard uses a table mapping console
keymaps to X layouts. But this table is not at all complete (only few keymaps
compared to the available keymaps in /lib/xkb/keymaps/i386 directory contents)
and once systemd-localed gets patched to load X layouts for console, this
mapping will not be necessary. For now, I've added the code, that tries to load
the keymap as given by user (e.g. 'cz_cp1250' or it could be an X layout) and if
it fails, it tries to load the basic variant (e.g. 'cz' for 'cz (qwerty)'). This
way, given a valid keymap, we load it, given an invalid keymap (X layout), we
load the basic variant and setup proper layout for the X session.

Could we drop console keymaps at all once systemd-localed gets patched to load X
layouts supporting only X layouts and not the legacy keymaps?

Ideas? Suggestions?

I believe PATCH 2/2 is okay, we don't use our loadKeymap code anywhere and
I see no reason to use it instead of the loadkeys command.

Vratislav Podzimek (2):
  Replace system-config-keyboard with our methods using ksdata.keyboard
  Remove our loadKeymap code from isys

anaconda                               |    6 +-
 anaconda.spec.in                       |    2 -
 pyanaconda/__init__.py                 |    2 -
 pyanaconda/isys/__init__.py            |    6 --
 pyanaconda/isys/isys.c                 |    1 -
 pyanaconda/isys/lang.c                 |  108 --------------------------------
 pyanaconda/isys/lang.h                 |   18 ------
 pyanaconda/keyboard.py                 |   67 +++++++++++++++++---
 pyanaconda/language.py                 |   12 ----
 tests/pyanaconda_test/language_test.py |   20 ------
 10 files changed, 63 insertions(+), 179 deletions(-)

[PATCH 1/2] Replace system-config-keyboard with our methods using ksdata.keyboard

[Vratislav Podzimek]

The only thing we were using from the system-config-keyboard was the
activate method that basically just calls loadkeys command. This can
be easily replaced with our own method using data from ksdata.keyboard
allowing us to remove anaconda.keyboard object.
---
 anaconda                               |    6 ++-
 anaconda.spec.in                       |    2 -
 pyanaconda/__init__.py                 |    2 -
 pyanaconda/keyboard.py                 |   67 ++++++++++++++++++++++++++++----
 pyanaconda/language.py                 |   12 ------
 tests/pyanaconda_test/language_test.py |   20 ----------
 6 files changed, 63 insertions(+), 46 deletions(-)

diff --git a/anaconda b/anaconda
index 256e7cd..2c17b58 100755
--- a/anaconda
+++ b/anaconda
@@ -725,6 +725,7 @@ if __name__ == "__main__":
     from pyanaconda import vnc
     from pyanaconda import kickstart
     from pyanaconda import ntp
+    from pyanaconda import keyboard

# to set UTF8 mode on the terminal, we need LANG to be set usefully
     if os.environ.get("LANG", "C") == "C":
@@ -901,8 +902,9 @@ if __name__ == "__main__":
     if opts.keymap:
         if not ksdata.keyboard.keyboard:
             ksdata.keyboard.keyboard = opts.keymap
-        anaconda.keyboard.set(opts.keymap)
-        anaconda.keyboard.activate()
+
+    if ksdata.keyboard.keyboard:
+        keyboard.activate_console_keymap(ksdata.keyboard.keyboard)

# now start the interface
     setupDisplay(anaconda, opts)
diff --git a/anaconda.spec.in b/anaconda.spec.in
index 2c5b246..3eb99a6 100644
--- a/anaconda.spec.in
+++ b/anaconda.spec.in
@@ -85,7 +85,6 @@ BuildRequires: zlib-devel
 BuildRequires: NetworkManager-devel >= %{nmver}
 BuildRequires: NetworkManager-glib-devel >= %{nmver}
 BuildRequires: dbus-devel >= %{dbusver}, dbus-python
-BuildRequires: system-config-keyboard >= %{sckeyboardver}
 %ifarch %livearches
 BuildRequires: desktop-file-utils
 %endif
@@ -126,7 +125,6 @@ Requires: python-cryptsetup >= %{pythoncryptsetupver}
 Requires: mdadm
 Requires: lvm2
 Requires: util-linux >= 2.15.1
-Requires: system-config-keyboard >= %{sckeyboardver}
 Requires: dbus-python
 Requires: python-pwquality
 Requires: python-bugzilla
diff --git a/pyanaconda/__init__.py b/pyanaconda/__init__.py
index eb44ca9..4eb3179 100644
--- a/pyanaconda/__init__.py
+++ b/pyanaconda/__init__.py
@@ -44,7 +44,6 @@ _ = lambda x: gettext.ldgettext("anaconda", x)
 class Anaconda(object):
     def __init__(self):
         import desktop, firewall, security
-        import system_config_keyboard.keyboard as keyboard
         from flags import flags

self._backend = None
@@ -60,7 +59,6 @@ class Anaconda(object):
         self._instLanguage = None
         self._intf = None
         self.isHeadless = False
-        self.keyboard = keyboard.Keyboard()
         self.ksdata = None
         self.mediaDevice = None
         self.methodstr = None
diff --git a/pyanaconda/keyboard.py b/pyanaconda/keyboard.py
index d0497d6..9a6c21f 100755
--- a/pyanaconda/keyboard.py
+++ b/pyanaconda/keyboard.py
@@ -31,6 +31,11 @@ for listing and various modifications of keyboard layouts settings.
 """

import os
+import re
+from pyanaconda import iutil
+
+import logging
+log = logging.getLogger("anaconda")

class KeyboardConfigError(Exception):
     """Exception class for keyboard configuration related problems"""
@@ -100,30 +105,76 @@ def get_layouts_xorg_conf(keyboard):

def write_layouts_config(keyboard, root):
     """
-    Function that writes a file with layouts configuration to
-    $root/etc/X11/xorg.conf.d/01-anaconda-layouts.conf
+    Function that writes files with layouts configuration to
+    $root/etc/X11/xorg.conf.d/01-anaconda-layouts.conf and
+    $root/etc/sysconfig/keyboard.

@param keyboard: ksdata.keyboard object
     @param root: path to the root of the installed system

"""

-    conf_dir = os.path.normpath(root + "/etc/X11/xorg.conf.d")
-    conf_file = "01-anaconda-keyboard.conf"
+    xconf_dir = os.path.normpath(root + "/etc/X11/xorg.conf.d")
+    xconf_file = "01-anaconda-keyboard.conf"
+
+    sysconf_file = "/etc/sysconfig/keyboard"

try:
-        if not os.path.isdir(conf_dir):
-            os.makedirs(conf_dir)
+        if not os.path.isdir(xconf_dir):
+            os.makedirs(xconf_dir)

except OSError as oserr:
         raise KeyboardConfigError("Cannot create directory xorg.conf.d")

try:
-        with open(os.path.join(conf_dir, conf_file), "w") as f:
+        with open(os.path.join(xconf_dir, xconf_file), "w") as f:
             f.write(get_layouts_xorg_conf(keyboard))

+        with open(sysconf_file, "w") as f:
+            f.write('KEYTABLE="%s"\n' % keyboard.keyboard)
+
     except IOError as ioerr:
-        raise KeyboardConfigError("Cannot write keyboard configuration file")
+        raise KeyboardConfigError("Cannot write keyboard configuration files")
+
+def activate_console_keymap(keymap):
+    """
+    Try to setup a given keymap as a console keymap. If there is no such
+    keymap, try to setup a basic variant (e.g. 'cz' instead of 'cz (qwerty)').
+
+    @param keymap: a keymap
+    @type keymap: string
+    @raise KeyboardConfigError: if loadkeys command is not available
+    @return: False if failed to activate both the given keymap and its basic
+             variant, True otherwise
+
+    """
+
+    try:
+        #TODO: replace with calling systemd-localed methods once it can load
+        #      X layouts
+        ret = iutil.execWithRedirect("loadkeys", [keymap], stdout="/dev/tty5",
+                                     stderr="/dev/tty5")
+    except OSError as oserr:
+        msg = "'loadkeys' command not available (%s)" % oserr.strerror
+        raise KeyboardConfigError(msg)
+
+    if ret != 0:
+        log.error("Failed to activate keymap %s" % keymap)
+
+        #failed to activate the given keymap, extract and try
+        #the basic keymap -- e.g. 'cz-cp1250' -> 'cz'
+        parts = re.split(r'[- _(]', keymap, 1)
+        keymap = parts[0]
+
+        ret = iutil.execWithRedirect("loadkeys", [keymap], stdout="/dev/tty5",
+                                     stderr="/dev/tty5")
+
+        if ret != 0:
+            log.error("Failed to activate basic variant %s" % keymap)
+        else:
+            log.error("Activated basic variant %s, instead" % keymap)
+
+    return ret == 0

def item_str(s):
     """Convert a zero-terminated byte array to a proper str"""
diff --git a/pyanaconda/language.py b/pyanaconda/language.py
index f1663d7..fb8d35e 100644
--- a/pyanaconda/language.py
+++ b/pyanaconda/language.py
@@ -28,7 +28,6 @@ import gettext
 from pyanaconda.constants import ROOT_PATH, DEFAULT_LANG
 import localeinfo
 from simpleconfig import SimpleConfigFile
-import system_config_keyboard.keyboard as keyboard

import logging
 log = logging.getLogger("anaconda")
@@ -165,17 +164,6 @@ class Language(object):
     def getCurrentLangSearchList(self):
         return localeinfo.expandLangs(self.systemLang) + ['C']

-    def getDefaultKeyboard(self):
-        try:
-            return self.localeInfo[self.systemLang][3]
-        except KeyError:
-            try:
-                kbd = keyboard.Keyboard()
-                kbd.read(ROOT_PATH)
-                return kbd.get()
-            except:
-                return self.localeInfo[self._default][3]
-
     def getDefaultTimeZone(self):
         try:
             return self.localeInfo[self.systemLang][4]
diff --git a/tests/pyanaconda_test/language_test.py b/tests/pyanaconda_test/language_test.py
index 5c895b2..2325a73 100644
--- a/tests/pyanaconda_test/language_test.py
+++ b/tests/pyanaconda_test/language_test.py
@@ -142,26 +142,6 @@ class LanguageTest(mock.TestCase):
         ret = lang.getCurrentLangSearchList()
         self.assertEqual(set(ret), set(['cs_CZ.UTF-8', 'cs_CZ', 'cs', 'C']))

-    def get_default_keyboard_default_test(self):
-        import pyanaconda.language
-        lang = pyanaconda.language.Language()
-        ret = lang.getDefaultKeyboard()
-        self.assertEqual(ret, 'us')
-
-    def get_default_keyboard_after_set_test(self):
-        import pyanaconda.language
-        lang = pyanaconda.language.Language()
-        lang.systemLang = 'cs'
-        ret = lang.getDefaultKeyboard()
-        self.assertEqual(ret, 'cz-lat2')
-
-    def get_default_keyboard_with_cs_CZ_locale_test(self):
-        import pyanaconda.language
-        pyanaconda.language.os.environ = {'LANG': 'cs'}
-        lang = pyanaconda.language.Language()
-        ret = lang.getDefaultKeyboard()
-        self.assertEqual(ret, 'cz-lat2')
-
     def get_default_time_zone_default_test(self):
         import pyanaconda.language
         pyanaconda.language.os.path.exists = mock.Mock(return_value=False)

[PATCH 1/2] Replace system-config-keyboard with our methods using ksdata.keyboard

[Chris Lumens]

> @@ -901,8 +902,9 @@ if __name__ == "__main__":
>      if opts.keymap:
>          if not ksdata.keyboard.keyboard:
>              ksdata.keyboard.keyboard = opts.keymap
>    anaconda.keyboard.set(opts.keymap)
>    anaconda.keyboard.activate()
>
> if ksdata.keyboard.keyboard:
>    keyboard.activate_console_keymap(ksdata.keyboard.keyboard)
>
> # now start the interface
>    setupDisplay(anaconda, opts)

Could this be moved up to right under the following comment in anaconda:

# Some kickstart commands must be executed immediately, as they
    # affect how anaconda operates.

Also I guess at some point, we're going to need to have ksdata and
command line opts play well with each other.
> +def activate_console_keymap(keymap):
> """
> Try to setup a given keymap as a console keymap. If there is no such
> keymap, try to setup a basic variant (e.g. 'cz' instead of 'cz (qwerty)').
> @param keymap: a keymap
> @type keymap: string
> @raise KeyboardConfigError: if loadkeys command is not available
> @return: False if failed to activate both the given keymap and its basic
>         variant, True otherwise
> """
> try:
>    #TODO: replace with calling systemd-localed methods once it can load
>    #      X layouts
>    ret = iutil.execWithRedirect("loadkeys", [keymap], stdout="/dev/tty5",
>                                 stderr="/dev/tty5")
> except OSError as oserr:
>    msg = "'loadkeys' command not available (%s)" % oserr.strerror
>    raise KeyboardConfigError(msg)
> if ret != 0:
>    log.error("Failed to activate keymap %s" % keymap)
>    #failed to activate the given keymap, extract and try
>    #the basic keymap -- e.g. 'cz-cp1250' -> 'cz'
>    parts = re.split(r'[- _(]', keymap, 1)
>    keymap = parts[0]

You should protect against parts being empty here.

- Chris

[PATCH 1/2] Replace system-config-keyboard with our methods using ksdata.keyboard

[Vratislav Podzimek]

On Wed, 2012-07-25 at 12:24 -0400, Chris Lumens wrote:
> > @@ -901,8 +902,9 @@ if __name__ == "__main__":
> >      if opts.keymap:
> >          if not ksdata.keyboard.keyboard:
> >              ksdata.keyboard.keyboard = opts.keymap
> >    anaconda.keyboard.set(opts.keymap)
> >    anaconda.keyboard.activate()
> >
> > if ksdata.keyboard.keyboard:
> >    keyboard.activate_console_keymap(ksdata.keyboard.keyboard)
> >
> > # now start the interface
> >    setupDisplay(anaconda, opts)
>
> Could this be moved up to right under the following comment in anaconda:
> # Some kickstart commands must be executed immediately, as they
> # affect how anaconda operates.

No problem.
> Also I guess at some point, we're going to need to have ksdata and
> command line opts play well with each other.

Agreed. What will be the policy when the boot option and kickstart
command for one thing are both used?
> > +def activate_console_keymap(keymap):
> > """
> > Try to setup a given keymap as a console keymap. If there is no such
> > keymap, try to setup a basic variant (e.g. 'cz' instead of 'cz (qwerty)').
> > @param keymap: a keymap
> > @type keymap: string
> > @raise KeyboardConfigError: if loadkeys command is not available
> > @return: False if failed to activate both the given keymap and its basic
> >         variant, True otherwise
> > """
> > try:
> >    #TODO: replace with calling systemd-localed methods once it can load
> >    #      X layouts
> >    ret = iutil.execWithRedirect("loadkeys", [keymap], stdout="/dev/tty5",
> >                                 stderr="/dev/tty5")
> > except OSError as oserr:
> >    msg = "'loadkeys' command not available (%s)" % oserr.strerror
> >    raise KeyboardConfigError(msg)
> > if ret != 0:
> >    log.error("Failed to activate keymap %s" % keymap)
> >    #failed to activate the given keymap, extract and try
> >    #the basic keymap -- e.g. 'cz-cp1250' -> 'cz'
> >    parts = re.split(r'[- _(]', keymap, 1)
> >    keymap = parts[0]
>
> You should protect against parts being empty here.

Good catch, I will add the check before pushing.

Apart from those two comments, do these two patches look alright?

[PATCH 1/2] Replace system-config-keyboard with our methods using ksdata.keyboard

[Chris Lumens]

> > Also I guess at some point, we're going to need to have ksdata and
> > command line opts play well with each other.
>
> Agreed. What will be the policy when the boot option and kickstart
> command for one thing are both used?

We should try to preserve whatever behavior we've kept in the past, so
long as there is some consistency to it.  I don't know what that
behavior would be off the top of my head.
> Apart from those two comments, do these two patches look alright?

Yep.

- Chris

[PATCH 2/2] Remove our loadKeymap code from isys

[Vratislav Podzimek]

It is used only in the text mode and it should be possible to replace
it with calling 'loadkeys' command in the new text mode.
---
 pyanaconda/isys/__init__.py |    6 ---
 pyanaconda/isys/isys.c      |    1 -
 pyanaconda/isys/lang.c      |  108 -------------------------------------------
 pyanaconda/isys/lang.h      |   18 --------
 4 files changed, 133 deletions(-)

diff --git a/pyanaconda/isys/__init__.py b/pyanaconda/isys/__init__.py
index fc766fc..63edbf6 100755
--- a/pyanaconda/isys/__init__.py
+++ b/pyanaconda/isys/__init__.py
@@ -171,12 +171,6 @@ def swapoff (path):
 def swapon (path):
     return _isys.swapon (path)

-## Load a keyboard layout for text mode installs.
-# @param keymap The keyboard layout to load.  This must be one of the values
-#               from rhpl.KeyboardModels.
-def loadKeymap(keymap):
-    return _isys.loadKeymap (keymap)
-
 def resetResolv():
     return _isys.resetresolv()

diff --git a/pyanaconda/isys/isys.c b/pyanaconda/isys/isys.c
index 8c4eb4b..35b2f33 100644
--- a/pyanaconda/isys/isys.c
+++ b/pyanaconda/isys/isys.c
@@ -126,7 +126,6 @@ static PyMethodDef isysModuleMethods[] = {
     { "resetresolv", (PyCFunction) doResetResolv, METH_VARARGS, NULL },
     { "swapon",  (PyCFunction) doSwapon, METH_VARARGS, NULL },
     { "swapoff",  (PyCFunction) doSwapoff, METH_VARARGS, NULL },
-    { "loadKeymap", (PyCFunction) doLoadKeymap, METH_VARARGS, NULL },
     { "vtActivate", (PyCFunction) doVtActivate, METH_VARARGS, NULL},
     { "isPseudoTTY", (PyCFunction) doisPseudoTTY, METH_VARARGS, NULL},
     { "isVioConsole", (PyCFunction) doisVioConsole, METH_NOARGS, NULL},
diff --git a/pyanaconda/isys/lang.c b/pyanaconda/isys/lang.c
index 07dee56..c38b1bb 100644
--- a/pyanaconda/isys/lang.c
+++ b/pyanaconda/isys/lang.c
@@ -56,111 +56,3 @@ int isysSetUnicodeKeymap(void) {
     return 0;
 }

-/* the file pointer must be at the beginning of the section already! */
-int loadKeymap(gzFile stream) {
-    int console;
-    int kmap, key;
-    struct kbentry entry;
-    int keymaps[MAX_NR_KEYMAPS];
-    int count = 0;
-    unsigned int magic;
-    short keymap[NR_KEYS];
-    struct stat sb;
-
-#if defined (__s390__) || defined (__s390x__)
-    return 0;
-#endif
-    if (isVioConsole())
-        return 0;
-
-    /* assume that if we're already on a pty loading a keymap is silly */
-    fstat(0, &sb);
-    if (major(sb.st_rdev) == 3 || major(sb.st_rdev) == 136)
-	return 0;
-
-    if (gzread(stream, &magic, sizeof(magic)) != sizeof(magic))
-	return -EIO;
-
-    if (magic != KMAP_MAGIC) return -EINVAL;
-
-    if (gzread(stream, keymaps, sizeof(keymaps)) != sizeof(keymaps))
-	return -EINVAL;
-
-    console = open("/dev/tty0", O_RDWR);
-    if (console < 0)
-	return -EACCES;
-
-    for (kmap = 0; kmap < MAX_NR_KEYMAPS; kmap++) {
-	if (!keymaps[kmap]) continue;
-
-	if (gzread(stream, keymap, sizeof(keymap)) != sizeof(keymap)) {
-	    close(console);
-	    return -EIO;
-	}
-
-	count++;
-	for (key = 0; key < NR_KEYS; key++) {
-	    entry.kb_index = key;
-	    entry.kb_table = kmap;
-	    entry.kb_value = keymap[key];
-	    if (KTYP(entry.kb_value) != KT_SPEC) {
-		if (ioctl(console, KDSKBENT, &entry)) {
-		    int ret = errno;
-		    close(console);
-		    return ret;
-		}
-	    }
-	}
-    }
-    close(console);
-    return 0;
-}
-
-int isysLoadKeymap(char * keymap) {
-    int num = -1;
-    int rc;
-    gzFile f;
-    struct kmapHeader hdr;
-    struct kmapInfo * infoTable;
-    char buf[16384]; 			/* I hope this is big enough */
-    int i;
-
-    f = gzopen("/etc/keymaps.gz", "r");
-    if (!f) return -EACCES;
-
-    if (gzread(f, &hdr, sizeof(hdr)) != sizeof(hdr)) {
-	gzclose(f);
-	return -EINVAL;
-    }
-
-    i = hdr.numEntries * sizeof(*infoTable);
-    infoTable = alloca(i);
-    if (gzread(f, infoTable, i) != i) {
-	gzclose(f);
-	return -EIO;
-    }
-
-    for (i = 0; i < hdr.numEntries; i++)
-	if (!strcmp(infoTable[i].name, keymap)) {
-	    num = i;
-	    break;
-	}
-
-    if (num == -1) {
-	gzclose(f);
-	return -ENOENT;
-    }
-
-    for (i = 0; i < num; i++) {
-	if (gzread(f, buf, infoTable[i].size) != infoTable[i].size) {
-	    gzclose(f);
-	    return -EIO;
-	}
-    }
-
-    rc = loadKeymap(f);
-
-    gzclose(f);
-
-    return rc;
-}
diff --git a/pyanaconda/isys/lang.h b/pyanaconda/isys/lang.h
index b5ff4da..aac821a 100644
--- a/pyanaconda/isys/lang.h
+++ b/pyanaconda/isys/lang.h
@@ -20,24 +20,6 @@
 #ifndef ISYS_LANG_H
 #define ISYS_LANG_H

-#include <zlib.h>
-
-/* define ask [e-mail] where this came from */
-#define KMAP_MAGIC 0x8B39C07F
-#define KMAP_NAMELEN 40         /* including '\0' */
-
-struct kmapHeader {
-    int magic;
-    int numEntries;
-};
-        
-struct kmapInfo {
-    int size;
-    char name[KMAP_NAMELEN];
-};
-
-int loadKeymap(gzFile stream);
-int isysLoadKeymap(char * keymap);
 int isysSetUnicodeKeymap(void);

#endif

[Chris Lumens]

> Could we drop console keymaps at all once systemd-localed gets patched to load X
> layouts supporting only X layouts and not the legacy keymaps?

I think we will always have to make sure the console keyboard layout is
set to something, though.  Think about this:  you do an install and set
your keyboard to whatever layout.  Later during the install, you decide
you want to switch over to tty2 and do something.  However if we don't
set the keyboard layout there, you'll have completely different
settings.  It would be pretty weird.

- Chris

[Vratislav Podzimek]

On Wed, 2012-07-25 at 12:28 -0400, Chris Lumens wrote:
> > Could we drop console keymaps at all once systemd-localed gets patched to load X
> > layouts supporting only X layouts and not the legacy keymaps?
>
> I think we will always have to make sure the console keyboard layout is
> set to something, though.  Think about this:  you do an install and set
> your keyboard to whatever layout.  Later during the install, you decide
> you want to switch over to tty2 and do something.  However if we don't
> set the keyboard layout there, you'll have completely different
> settings.  It would be pretty weird.

I've expressed myself badly. I meant we could drop keymaps
from /lib/kbd/keymaps/i386/ and use only X layouts for both X and
console, once systemd-localed is able to load X layouts as console
layouts.

[Chris Lumens]

> I've expressed myself badly. I meant we could drop keymaps
> from /lib/kbd/keymaps/i386/ and use only X layouts for both X and
> console, once systemd-localed is able to load X layouts as console
> layouts.

Ahh, I didn't know that was possible.  If so, I'd certainly be fine with
getting rid of the keymaps directory.

- Chris