[bug#34067] Add input-wacom/inputattach

Message ID 87fttvl9b1.fsf@yahoo.de
State Accepted
Headers show
Series [bug#34067] Add input-wacom/inputattach | expand

Checks

Context Check Description
cbaines/applying patch success Successfully applied

Commit Message

Tim Gesthuizen Jan. 14, 2019, 12:20 p.m. UTC
Hi,
I added the following changes in order to get the touchscreen on my
thinkpad x200t to work.

Quick summary:

- Add functions to locate modules for linux-libre.
- Change linux-libre to not include non deterministic symlinks to dead
  items and package the source in separate output.
- Add input-wacom driver and inputattach.
- Add an inputattach service type that starts inputattach as a daemon.
- Document the new service type

There are some things worth discussing about these patches:

- firmware.scm for input-wacom is probably not quite the optimal file.
- linux-libre gains quite some size from packaging its build tree
- Lots of small things
- Maybe other things

It would be nice if someone could review the patches.

Tim.

Comments

Ludovic Courtès Jan. 20, 2019, 6:05 p.m. UTC | #1
Hi Tim,

Thanks for the patches!  Here’s a quick review:

>>From 9c5fc83d9d5a162fb3d4662c3e66cd77918159da Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Wed, 9 Jan 2019 18:40:42 +0100
> Subject: [PATCH 1/7] gnu: Add linux-libre-fill-version
>
> linux-libre-fill-version adds a patch version to the version if it is
> missing.
>
> * gnu/packages/linux.scm (linux-libre-fill-version): New function

Could the ‘version-major+minor’ procedure play a similar role for what
you want?

>>From 24f65c10bcfc8349778d024f039528997c9e7da9 Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Wed, 9 Jan 2019 17:56:21 +0100
> Subject: [PATCH 2/7] gnu: Add linux-libre-module-path
>
> Add a function that evaluates to the module path where the modules for
> linux-libre in a specific version are stored.
>
> * gnu/packages/linux.scm (linux-libre-module-path): New function

s/path/file-name/  :-)

>>From e771153f957e1bd41dbef32bf6f7e997f9a732f5 Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Fri, 4 Jan 2019 12:22:34 +0100
> Subject: [PATCH 3/7] gnu: linux-libre: Copy source to the store
>
> The source code is needed by some kernel modules to compile. The item in the
> store only symlinks the build directory in /tmp which is not reachable later
> on and is a source of non determinism for the store item.
> This patch deletes the symlinks and copies the source to a separate output.
>
> * gnu/packages/linux.scm (linux-libre):
> [outputs]: Add output source
> [arguments]: Add phase to copy source to the store item.

I’d rather avoid that and this would fill up stores.  What about simply
writing:

  (inputs
    `(("linux-libre-source" ,(package-source linux-libre))
      …))

in the ‘input-wacom’ package?  The downside is that the package
definition of ‘input-wacom’ would have to unpack it, etc., but it seems
worth it.

>>From 4cc4535566f0496e24fcf567c73494c18d4b8a08 Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Sat, 5 Jan 2019 00:32:16 +0100
> Subject: [PATCH 4/7] gnu: Add input-wacom
>
> * gnu/packages/firmware.scm (input-wacom): New variable

[...]

> +    (synopsis "Linux kernel driver for various wacom touchscreens")

So this should go to linux.scm rather than firmware.scm.  :-)

Could you please make sure the code doesn’t contain binary blobs—e.g.,
in the form of large ‘char’ arrays?  You can maybe just check whether
Debian “main” contains this driver.

Then there’s the question of which kernel version to target.  I guess
it’s reasonable to just choose whatever ‘linux-libre’ points to.
Thoughts?

> +    (description "A set of kernel drivers that add support for various wacom
> +touchscreens. In combination with xf86-input-wacom and libwacom it forms a set
> +of modules to support wacom touchscreens with the X server.")

Please write full sentences and address the two-space-after-period issue
that ‘guix lint’ tells you about.  :-)

>>From cb02272b9759426427ba1accc60915b455dfb357 Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Sat, 5 Jan 2019 20:55:14 +0100
> Subject: [PATCH 5/7] gnu: Add inputattach
>
> * gnu/packages/firmware.scm (inputattach): New variable

[...]

> +         (replace 'install
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let ((target-dir (string-append
> +                                (assoc-ref outputs "out")
> +                                "/bin/")))
> +               (mkdir-p target-dir)
> +               (copy-file "inputattach/inputattach"
> +                          (string-append target-dir
> +                                         "inputattach"))))))))

Please return #t.

>>From 8a1bb6706be11cd9c1e683e6d242accad0346d6b Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Sat, 5 Jan 2019 23:28:18 +0100
> Subject: [PATCH 6/7] gnu: Add inputattach service
>
> Add a service that runs inputattach as a daemon to translate events from
> serial ports.
>
> * gnu/local.mk: Add gnu/services/hardware.scm
> * gnu/services/hardware.scm (<inputattach-configuration>): New record type
> * gnu/services/hardware.scm (inputattach-service-type): New service type
> * gnu/services/hardware.scm (inputattach-service): New function
>
> squash

Leftover.  :-)  Also, no need to repeat the file name when it’s the same.

> +++ b/gnu/services/hardware.scm

Do you think desktop.scm would make sense?

> +(define-record-type* <inputattach-configuration>
> +  inputattach-configuration
> +  make-inputattach-configuration
> +  inputattach-configuration?
> +  (devicetype inputattach-configuration-devicetype
> +              (default "wacom"))

‘device-type’

> +(define inputattach-service-type
> +  (service-type
> +   (name 'inputattach)
> +   (extensions
> +    (list (service-extension shepherd-root-service-type
> +                             inputattach-shepherd-service)))
> +   (default-value (inputattach-configuration))))

Please add a ‘description’ field so people can find out about it with
‘guix system search’.

> +(define* (inputattach-service #:key (type "wacom") (device "/dev/ttyS0") (log-file #f))

This can be removed (we no longer provide such procedures.)

>>From a9cd86ad2244fff023f0c1bf4038748872aeab13 Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
> Date: Sun, 6 Jan 2019 11:56:57 +0100
> Subject: [PATCH 7/7] doc: Add documentation for inputattach-service
>
> * doc/guix.texi (Miscellaneous Services): Add inputattach Service
>   subsubheading

Can be squashed with the previous patch.

> +The @code{(gnu services hardware)} module provides the following service.
> +
> +@deffn {Scheme Procedure} inputattach-service [#:type "wacom"] @
> +       [#:device "/dev/ttyS0"] [#:log-file #f]
> +Return a service that runs inputattach on @var{device} to decode events from
> +@var{type}.
> +@end deffn

Here could you document ‘inputattach-service-type’ and
‘inputattach-configuration’?

Thanks!

Ludo’.

Patch

From a9cd86ad2244fff023f0c1bf4038748872aeab13 Mon Sep 17 00:00:00 2001
From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
Date: Sun, 6 Jan 2019 11:56:57 +0100
Subject: [PATCH 7/7] doc: Add documentation for inputattach-service

* doc/guix.texi (Miscellaneous Services): Add inputattach Service
  subsubheading
---
 doc/guix.texi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index c0cc8d416..e0921d34d 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -22020,6 +22020,17 @@  that enables sharing the clipboard with a vm and setting the guest display
 resolution when the graphical console window resizes.
 @end deffn
 
+@cindex inputattach
+@subsubheading inputattach Service
+
+The @code{(gnu services hardware)} module provides the following service.
+
+@deffn {Scheme Procedure} inputattach-service [#:type "wacom"] @
+       [#:device "/dev/ttyS0"] [#:log-file #f]
+Return a service that runs inputattach on @var{device} to decode events from
+@var{type}.
+@end deffn
+
 @subsubsection Dictionary Services
 @cindex dictionary
 The @code{(gnu services dict)} module provides the following service:
-- 
2.20.1