[bug#34067] Add input-wacom/inputattach

[Ludovic Courtès]

Hi Tim,

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

>>From 9c5fc83d9d5a162fb3d4662c3e66cd77918159da Mon Sep 17 00:00:00 2001
> From: Tim Gesthuizen <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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.
> +
> address@hidden {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
> address@hidden
> address@hidden deffn

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

Thanks!

Ludo’.