xserver: check case of multiple keyboard layouts #26984

lheckemann · 2017-06-30T07:05:47Z

Motivation for this change

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2017-06-30T07:05:49Z

@lheckemann, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @aszlig and @nbp to be potential reviewers.

lheckemann · 2017-06-30T07:06:32Z

cc @aszlig

0xABAB · 2017-06-30T23:28:43Z

nixos/modules/services/x11/xserver.nix

+      missing=()
+      for layout in "''${layouts[@]}" ; do
+        if ! sed -n -e ':i /^! \(layout\|variant\) *$/ {
+            :l; n; /^!/bi; s/^ *\([^ ]\+\).*/\1/p; tl


Holy Batman for your sed invocation. I doubt many people would be able to read that without consulting a manual.

Not mine, @aszlig's. It seems to work so I just left it as is.

@0xABAB: Although I think the expression is quite simple (if you know sed), I'm going to add a comment with a description anyway.

0xABAB · 2017-06-30T23:29:47Z

nixos/modules/services/x11/xserver.nix

+      for layout in "''${layouts[@]}" ; do
+        if ! sed -n -e ':i /^! \(layout\|variant\) *$/ {
+            :l; n; /^!/bi; s/^ *\([^ ]\+\).*/\1/p; tl
+           }' "$xkbDir/rules/base.lst" | grep -qxF "$layout"


It would be safer to add -- to stop processing options.

That would result in it searching the file named by "$layout". But in a similar vein, I should add -e.

0xABAB · 2017-06-30T23:32:43Z

nixos/modules/services/x11/xserver.nix

      cat >&2 <<-EOF

-      The selected keyboard layout definition does not exist:
+      The selected keyboard layout definitions do not exist:


Good that you are trying to help the user, but you could also just output all the valid values (a constructive reply from the machine) with the file name, instead of letting the machine say that the user messed up without any help.

There are too many valid values for printing all of them to be useful.

Hm, I think we can do that if we pipe them into column.

0xABAB · 2017-06-30T23:33:29Z

nixos/modules/services/x11/xserver.nix


      EOF
-      exit 1


What happened to this exit 1?

Isn't necessary, the build will fail because it didn't produce $out. I suppose it's better to have it explicit rather than implicit though, so I'll put it back.

0xABAB

I feel many of my concerns need to be addressed.

Fixes NixOS#26961

aszlig · 2017-07-03T05:07:45Z

nixos/modules/services/x11/xserver.nix

-         }' "$xkbDir/rules/base.lst" | grep -qxF "$layout"
-      then
-        touch "$out"
+      IFS=, read -a layouts <<<'${cfg.layout}'


This is not well escaped, because if the layout option contains a single quote you can inject arbitrary shell commands, like let's say layout = "' || exit 0. In this case it's better to use $layout here, because we already have that in the builder environment.

aszlig · 2017-07-03T05:09:20Z

nixos/modules/services/x11/xserver.nix

+      missing=()
+      for layout in "''${layouts[@]}" ; do
+        if ! sed -n -e ':i /^! \(layout\|variant\) *$/ {
+            :l; n; /^!/bi; s/^ *\([^ ]\+\).*/\1/p; tl


@0xABAB: Although I think the expression is quite simple (if you know sed), I'm going to add a comment with a description anyway.

aszlig · 2017-07-03T05:10:16Z

nixos/modules/services/x11/xserver.nix

      cat >&2 <<-EOF

-      The selected keyboard layout definition does not exist:
+      The selected keyboard layout definitions do not exist:


Hm, I think we can do that if we pipe them into column.

aszlig · 2017-07-03T05:12:28Z

nixos/modules/services/x11/xserver.nix

-      layout (check $xkbDir/rules/base.lst for options).
+      Set \`services.xserver.layout' to include only names of
+      existing keyboard layouts (check $xkbDir/rules/base.lst
+      for options).


We probably should just use echo here, so the lines wrap around the actual line width of the users terminal, because $xkbDir clearly stands out in length here and might be confusing if you just glimpse over it.

aszlig · 2017-07-03T05:28:17Z

I'm going to commit a different fix of #26961 which is similar to your PR, but with the comments addressed. Basically I was the one who broke it, so I'll fix it :-D

lheckemann · 2017-07-03T06:17:08Z

Alright.

@0xABAB

This was brought up by @0xABAB in #26984 by the following comment: #26984 (comment) Signed-off-by: aszlig <aszlig@redmoonstudios.org>

@lheckemann

Regression introduced by 44c64fe. The services.xserver.layout option allows to specify more than one layout separated by comma, which the commit above didn't take into account. This is very similar to @lheckemann's pull request (#26984) but differs in the following ways: * Print out the full list available layouts (as suggested by @0xABAB in [1]). * Loop over $layout using the default IFS (and thus no need for escaping ${cfg.layout}), because the layouts won't contain white spaces. * Re-do the error message, which now uses multiple echos instead of a heredoc, so the line is wrapped according to the viewers terminal width. I've tested this with several good and bad layouts and also against the keymap NixOS VM subtests. [1]: #26984 (comment) Signed-off-by: aszlig <aszlig@redmoonstudios.org> Fixes: #26961 Closes: #26984

@0xABAB

This was brought up by @0xABAB in NixOS#26984 by the following comment: NixOS#26984 (comment) Signed-off-by: aszlig <aszlig@redmoonstudios.org> (cherry picked from commit d97cdfc)

@lheckemann

Regression introduced by 44c64fe. The services.xserver.layout option allows to specify more than one layout separated by comma, which the commit above didn't take into account. This is very similar to @lheckemann's pull request (NixOS#26984) but differs in the following ways: * Print out the full list available layouts (as suggested by @0xABAB in [1]). * Loop over $layout using the default IFS (and thus no need for escaping ${cfg.layout}), because the layouts won't contain white spaces. * Re-do the error message, which now uses multiple echos instead of a heredoc, so the line is wrapped according to the viewers terminal width. I've tested this with several good and bad layouts and also against the keymap NixOS VM subtests. [1]: NixOS#26984 (comment) Signed-off-by: aszlig <aszlig@redmoonstudios.org> Fixes: NixOS#26961 Closes: NixOS#26984 (cherry picked from commit 69da180)

0xABAB reviewed Jun 30, 2017

View reviewed changes

0xABAB suggested changes Jun 30, 2017

View reviewed changes

xserver: check case of multiple keyboard layouts

00d328b

Fixes NixOS#26961

lheckemann force-pushed the xkb-multiple branch from a12e165 to 00d328b Compare July 1, 2017 09:52

aszlig requested changes Jul 3, 2017

View reviewed changes

lheckemann closed this Jul 3, 2017

aszlig added a commit that referenced this pull request Jul 3, 2017

nixos/xserver: Document the layouts sed expression

d97cdfc

This was brought up by @0xABAB in #26984 by the following comment: #26984 (comment) Signed-off-by: aszlig <aszlig@redmoonstudios.org>

lheckemann deleted the xkb-multiple branch January 18, 2019 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xserver: check case of multiple keyboard layouts #26984

xserver: check case of multiple keyboard layouts #26984

lheckemann commented Jun 30, 2017

mention-bot commented Jun 30, 2017

lheckemann commented Jun 30, 2017

0xABAB Jun 30, 2017

lheckemann Jul 1, 2017

aszlig Jul 3, 2017

0xABAB Jun 30, 2017

lheckemann Jul 1, 2017

0xABAB Jun 30, 2017

lheckemann Jul 1, 2017

aszlig Jul 3, 2017

0xABAB Jun 30, 2017

lheckemann Jul 1, 2017

0xABAB left a comment

aszlig Jul 3, 2017

aszlig Jul 3, 2017

aszlig Jul 3, 2017

aszlig Jul 3, 2017

aszlig commented Jul 3, 2017

lheckemann commented Jul 3, 2017

xserver: check case of multiple keyboard layouts #26984

xserver: check case of multiple keyboard layouts #26984

Conversation

lheckemann commented Jun 30, 2017

Motivation for this change

Things done

mention-bot commented Jun 30, 2017

lheckemann commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xABAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aszlig commented Jul 3, 2017

lheckemann commented Jul 3, 2017