[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.

[Lukasz Olszewski <dev@lukaszolszewski.info>]

[-- Attachment #1: Type: text/plain, Size: 4588 bytes --]

Hi,

No problem at all regarding the delay. Unfortunately I've been busier than
usual in last few weeks (this is likely to continue for few more weeks).

Regarding the comments please see inline below.

On Mon, 17 Oct 2022, 10:03 Ludovic Courtès, <ludo@gnu.org> wrote:

> 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.)
>

Ok, will do.


> > +++ b/gnu/packages/libldm.scm
> > @@ -0,0 +1,70 @@
> > +(define-module (gnu packages libldm)
>
> We’ll need the license/copyright header as you noted.
>

I've posted a later patch that included those, but it was posted as a patch
on top of a patch so perhaps it wasn't well visible. I'll integrate it in
the next version.


> > +    (arguments
> > +     '(#:tests? #f
>
> Please add a comment explaining why tests are skipped.  That should be a
> last resort.
>
> > +       #:parallel-build? #t
>
> This is unnecessary.
>

Are parallel builds enabled by default? Or is there a convention not to
enable then unless some requirements are met?


> > +       #: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.
>

I'll have to test it without. If setenv indeed fails now then it should
continue to work without it.


> > +                  (replace 'bootstrap
> > +                    (lambda _
> > +                      (invoke "autoreconf" "-fiv"))))))
>
> Is it necessary?  The default ‘bootstrap’ phase does something similar.
>

I've copied this phase from another package. If I remember correctly the
configure phase failed without. I'll have to test again to check.


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

Ok, will do.


> > +(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…).
>

OK, I'll keep the above.


> > +(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?
>

Ok


> > +(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.
>

OK, will do.


> Could you send updated patches?
>

If I don't manage to do it this week, then on the weekend.


>
> Thanks in advance!
>
> Ludo’.
>

Regards,
Lukasz

>

[-- Attachment #2: Type: text/html, Size: 8629 bytes --]