GNU bug report logs - #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.

Previous Next

Full log

Message #11 received at 57590 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Lukasz Olszewski <dev <at> lukaszolszewski.info>
Cc: 57590 <at> debbugs.gnu.org
Subject: Re: 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.
Date: Mon, 17 Oct 2022 10:03:49 +0200

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 <at> 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’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.