[bug#57590] [PATCH] Adding libldm: Manager for Windows dynamic disks including software RAID. It creates device mapper entries for dynamic disks allowing them to be mounted.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Lukasz,

Apologies for the delay!

I think the patch series is close to being ready; we’ll need a few
changes before we’re done.

Lukasz Olszewski <dev@lukaszolszewski.info> skribis:

> ---
>  gnu/packages/libldm.scm | 70 +++++++++++++++++++++++++++++++++++++++++
>  gnu/services/libldm.scm | 47 +++++++++++++++++++++++++++

Please make one patch adding the package, and another one adding the
service.

In each patch, please make sure to add the new file to gnu/local.mk (you
can check the Git history for examples.)

> +++ b/gnu/packages/libldm.scm
> @@ -0,0 +1,70 @@
> +(define-module (gnu packages libldm)

We’ll need the license/copyright header as you noted.

> +    (arguments
> +     '(#:tests? #f

Please add a comment explaining why tests are skipped.  That should be a
last resort.

> +       #:parallel-build? #t

This is unnecessary.

> +       #:phases (modify-phases %standard-phases
> +                  (add-before 'configure 'set-env
> +                    (lambda _
> +                      (setenv "CONFIG_SHELL"
> +                              (which "")) #t))

I don’t think this can work because (which "") returns #f but ‘setenv’
expects a string.

> +                  (replace 'bootstrap
> +                    (lambda _
> +                      (invoke "autoreconf" "-fiv"))))))

Is it necessary?  The default ‘bootstrap’ phase does something similar.

> +    (license license:gpl3)))

This should be ‘license:gpl3+’ because source file headers carry the “or
any later version” wording.

> +(define-record-type* <libldm-configuration>
> +                     libldm-configuration
> +                     make-libldm-configuration
> +                     libldm-configuration?
> +                     (package
> +                       libldm-configuration-package
> +                       (default libldm))
> +                     (action libldm-configuration-action
> +                             (default '("create" "all"))))

Indentation is off here (I noticed that ‘guix style’ got it wrong so I’m
fixing it now…).

> +(define (libldm-shepherd-service config)
> +  "Return a <shepherd-service> for libldm with CONFIG"
> +  (let* ((libldm (libldm-configuration-package config))
> +         (action (libldm-configuration-action config)))
> +    (list (shepherd-service (documentation
> +                             "Run ldmtool to create Windows dynamic
> disc device nodes at startup.")

Maybe s/disc/disk/ throughout for consistency?

> +(define libldm-service-type
> +  (service-type (name 'libldm)
> +                (extensions (list (service-extension
> +                                   shepherd-root-service-type
> +                                   libldm-shepherd-service)))
> +                (default-value (libldm-configuration))
> +                (description
> +                 "Run ldmtool to create device nodes for Windows
> dynamic discs so they can be mounted")))

Please add a period at the end, and write @command{ldmtool}.

One last thing: could you add documentation for the service in
doc/guix.texi, maybe under “Virtualization” or in some new section?
Please include a paragraph giving some context and an example.

Could you send updated patches?

Thanks in advance!

Ludo’.