Re: Digimend kernel drivers for Guix

[Tobias Geerinckx-Rice]

Marlin,

Marlin wrote:
> I have just managed to get the digimend kernel drivers packaged 
> for guix:

Cool!  I encourage you to read the ‘Contributing’ section of the 
manual, set up your own git repository, and submit a patch adding 
this package to linux.scm.

I don't mind merging this version eventually, but you'll have to 
do so sooner or later if you want to keep contributing (and I hope 
you will).  Might as well jump in.

> (define-public digimend-kernel-drivers

I realise this is the upstream name.  I'm not sure what to do 
here.

We currently have 2 separately packaged Linux kernel modules: 
vhba-module (which was first) and acpi-call-linux-module (the 
latter added by me).  I'd really like to see some semblance of 
consistency in naming these things and don't think ‘-module[s]’ 
(for Guile?), ‘-driver[s]’ (not all modules are drivers, but I 
agree it's subtle), or ‘-kernel-’ (which kernel?) cut it.

>   (package
>    (name "digimend-kernel-drivers")
>    (version "9")
>    (source (origin
>             (method url-fetch)
>             (uri (string-append "[…long URL is long…]"

Please keep lines shorter than 80 characters.  Often indenting can 
make the difference (SOURCE and ORIGIN here could be on two 
separate lines, for example) but that won't help you here.

You're already calling STRING-APPEND, so you can simply split up 
the URL:

 (uri (string-append
       "https://github.com/DIGImend/digimend-kernel-drivers/";
       "releases/download/v9/digimend-kernel-drivers-"
       …

…or however you prefer.  Also note the hard-coded ‘9’ that needs 
replacing by VERSION.

                                                                                               
>          (build-system linux-module-build-system)

All lines from here on are misleadingly indented: this opening 
bracket should be even with that of SOURCE above.

If you use emacs, fixing this is as easy as running C-M-q at the 
beginning of the package definition.  If you don't (nobody's 
perfect), we provide a stand-alone (emacs) script to do this:

 ./etc/indent-code.el gnu/packages/FILE.scm PACKAGE

…although I've never used it.  See the ‘Formatting Code’ section 
of the manual.

>          (arguments '(#:phases (modify-phases %standard-phases 
>          (delete 'check))))

This works (and once re-indented it won't even exceed 80 
characters), but consider the more conventional and easy-to-skim

 (arguments
  '(#:phases
    (modify-phases %standard-phases
      (delete 'foo))))            ; no foo gizmo

…including the helpful comment.

However, in this case, please use the even simpler:
             
 (arguments
  '(#:tests?))                    ; no test suite

>          (synopsis "Kernel Drivers for non-wacom Digital 
>          Tablets")

‘Drivers’ & ‘Digital Tablets’ shouldn't be capitalised; ‘Wacom’ 
should.

However, I think the project already offers a good synopsis on 
their home page: how about "Linux drivers for generic graphics 
tablets"?

>          (description "Provides Drivers for Tablets from \
>                           companies like huion and ugee.")

Same here: nouns in English don't need to be capitalised, proper 
nouns such as ‘Huion’ do (even if they don't do so themselves!).

Why single out these two manufacturers?  The home page lists a lot 
more.

This is also far to short to be a description: ideally, they 
should be around ~10 lines.  Here too, the home page has some good 
stuff ready to use.

 The DIGImend project aims at improving Linux support for generic
 graphics tablets.  Essentially, this means any USB-connected 
 drawing
 tablet not produced by Wacom (and often more affordable).

 This package supports tablets designed and sold by Huion, KYE,
 UC-Logic and Waltop, rebranded and sold by Aiptek, Genius, 
 Monoprice,
 Princeton, Trust, and many others.

This way, people searching for ‘Aiptek’ will still find this 
package, and I took the liberty of ‘keyword-stuffing’ the word 
‘drawing’ in there for good measure ;-)

This is still pretty short.  If there's more to be said, please 
add it!

>  (license gpl2)))

This seems to be accurate.  You'll need to add a ‘license:’ when 
moving this to linux.scm.

Thank you for packaging this, and I hope you'll submit a 
git-formatted patch to guix-patches@ soon.  If you get stuck, 
don't hesitate to ask questions on guix-devel@ or #guix.

Kind regards,

T G-R (nckx)

signature.asc
Description: PGP signature