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