| Message ID | CACwB4R5Nype6=UppoXOZZO234AM5CjA=Opad4hyJNM0WChqKcQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#57590] Adding libldm: Manager for Windows dynamic disks including software RAID. It creates device mapper entries for dynamic disks allowing them to be mounted. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
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’.
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 >
Hi, Lukasz Olszewski <dev@lukaszolszewski.info> skribis: >> > +++ 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. I did see it (thanks!). It would be great though if you could send a single “v2” patch that includes everything. >> > + #:parallel-build? #t >> >> This is unnecessary. >> > > Are parallel builds enabled by default? Yes, that’s why. >> > + (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. Yes please. Thanks, Ludo’.
diff --git a/gnu/packages/libldm.scm b/gnu/packages/libldm.scm new file mode 100644 index 0000000000..38fb5e218e --- /dev/null +++ b/gnu/packages/libldm.scm @@ -0,0 +1,70 @@ +(define-module (gnu packages libldm) + #:use-module (gnu packages) + #:use-module (guix packages) + #:use-module ((guix licenses) + #:prefix license:) + #:use-module (guix build-system gnu) + #:use-module (gnu packages base) + #:use-module (gnu packages autotools) + #:use-module (gnu packages m4) + #:use-module (gnu packages pkg-config) + #:use-module (gnu packages gnome) + #:use-module (gnu packages glib) + #:use-module (gnu packages compression) + #:use-module (gnu packages readline) + #:use-module (gnu packages linux) + #:use-module (gnu packages xml) + #:use-module (gnu packages docbook) + #:use-module (gnu packages gtk) + #:use-module (guix git-download)) + +(define-public libldm + (package + (name "libldm") + (version "0.2.5") + (source (origin + (method git-fetch) + (uri (git-reference + (url "https://github.com/mdbooth/libldm.git") + (commit (string-append "libldm-" version)))) + (file-name (git-file-name name version)) + (sha256 + (base32 + "08iz3kq4ci79abpyxwwqmzi3bayyk4s29n8h1jqgdgk5yskwgnrn")))) + (build-system gnu-build-system) + (inputs (list json-glib + glib + zlib + readline + lvm2 + libgudev)) + (native-inputs (list which + m4 + libtool + autoconf-wrapper + automake + pkg-config + `(,glib "bin") + gtk-doc + libxml2 + libxslt + docbook-xsl)) + (arguments + '(#:tests? #f + #:parallel-build? #t + #:phases (modify-phases %standard-phases + (add-before 'configure 'set-env + (lambda _ + (setenv "CONFIG_SHELL" + (which "")) #t)) + (add-before 'bootstrap 'run-gtkdocize + (lambda _ + (invoke "gtkdocize"))) + (replace 'bootstrap + (lambda _ + (invoke "autoreconf" "-fiv")))))) + (home-page "https://github.com/mdbooth/libldm") + (synopsis "Manager for Microsoft Windows dynamic disks") + (description + "Libldm is a library for managing Microsoft Windows dynamic disks, which use Microsoft's LDM metadata. It can inspect them, and also create and remove device-mapper block devices which can be mounted.") + (license license:gpl3))) diff --git a/gnu/services/libldm.scm b/gnu/services/libldm.scm new file mode 100644 index 0000000000..00e19540f0 --- /dev/null +++ b/gnu/services/libldm.scm @@ -0,0 +1,47 @@ +(define-module (gnu services libldm) + #:use-module (guix records) + #:use-module (guix gexp) + #:use-module (guix diagnostics) + #:use-module (guix i18n) + #:use-module (srfi srfi-1) + #:use-module (srfi srfi-26) + #:use-module (ice-9 match) + #:use-module (guix gexp) + #:use-module (gnu services) + #:use-module (guix modules) + #:use-module (gnu services base) + #:use-module (gnu services shepherd) + #:use-module (gnu packages libldm) + #:export (libldm-configuration libldm-configuration? libldm-service-type)) + +(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")))) + +(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.") + (provision '(libldmd)) + (one-shot? #t) + (start #~(make-forkexec-constructor (append (list (string-append #$libldm + "/bin/ldmtool")) + '(#$@action)))) + (stop #~(make-kill-destructor)))))) + +(define libldm-service-type + (service-type (name 'libldm) + (extensions (list (service-extension + shepherd-root-service-type