[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#57590] [PATCH] Adding libldm: Manager for Windows dynamic disks inc
From: |
Ludovic Courtès |
Subject: |
[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 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.1 (gnu/linux) |
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’.
- [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 <=