Package: guix-patches;
Reported by: Alexey Abramov <levenson <at> mmer.org>
Date: Sun, 23 Mar 2025 10:27:04 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #50 received at 77204 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Alexey Abramov <levenson <at> mmer.org> Cc: Ludovic Courtès <ludo <at> gnu.org>, 77204 <at> debbugs.gnu.org Subject: Re: [bug#77204] [PATCH v3 1/3] services: dnsmasq: Add shepherd-provision and shepherd-requirement fields. Date: Tue, 22 Apr 2025 14:30:18 +0900
Hi Alexey,
Alexey Abramov <levenson <at> mmer.org> writes:
> * gnu/services/dns.scm (<dnsmasq-configuration>)[provision]: Mark as
> deprecated with a warning. Set default to #f.
> * gnu/services/dns.scm (<dnsmasq-configuration>)[shepherd-provision]:
> Add new field for consistency with other services.
> * gnu/services/dns.scm (<dnsmasq-configuration>)[shepherd-requirement]:
> Add new field.
> * gnu/services/dns.scm (dnsmasq-shepherd-service): Use the new fields.
> * doc/guix.texi: Document these changes.
> * doc/guix-cookbook.texi (Custom NAT-based network for libvirt):
> Update example to use shepherd-provision instead of provision.
Same file names in changelog should only appear once. See (info
"(standards) Style of Change Logs") (that's a manual part of our
'gnu-standards' package) for more guidance, or 'git log' for examples.
> ---
> doc/guix-cookbook.texi | 4 ++--
> doc/guix.texi | 11 ++++++++---
> gnu/services/dns.scm | 24 ++++++++++++++++++++----
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/doc/guix-cookbook.texi b/doc/guix-cookbook.texi
> index fe4cac79c3a..d4832b9bb40 100644
> --- a/doc/guix-cookbook.texi
> +++ b/doc/guix-cookbook.texi
> @@ -4031,8 +4031,8 @@ Custom NAT-based network for libvirt
> (service dnsmasq-service-type
> (dnsmasq-configuration
> ;; You can have multiple instances of `dnsmasq-service-type` as long
> - ;; as each one has a different provision.
> - (provision '(dnsmasq-virbr0))
> + ;; as each one has a different shepherd-provision.
> + (shepherd-provision '(dnsmasq-virbr0))
> (extra-options (list
> ;; Only bind to the virtual bridge. This
> ;; avoids conflicts with other running
Looks reasonable; it does seem like 'shepherd-provision' and
'shepherd-requirement' are the prevalent and preferable names to
distinguish them from other configuration options.
> diff --git a/doc/guix.texi b/doc/guix.texi
> index bcb1f9d9cf8..90fa6779657 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -35067,9 +35067,14 @@ DNS Services
> @item @code{package} (default: @var{dnsmasq})
> Package object of the dnsmasq server.
>
> -@item @code{provision} (default: @code{'(dnsmasq)})
> -A list of symbols for the Shepherd service corresponding to this dnsmasq
> -configuration.
> +@item @code{shepherd-provision} (default: @code{'(dnsmasq)})
> +@itemx @code{shepherd-requirement} (default: @code{'(user-processes networking)})
> +This option can be used to provide a list of Shepherd service names
> +(symbols) provided by this service. You might want to change the default
^
Use double space to separate sentences in the Guix sources (whether in
doc or comments, etc); see (info "(standards) Comments").
> +value if you intend to run several @command{dnsmasq} instances.
> +
> +Likewise, @code{shepherd-requirement} is a list of Shepherd service names
> +(symbols) that this service will depend on.
>
> @item @code{no-hosts?} (default: @code{#f})
> When true, don't read the hostnames in /etc/hosts.
> diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
> index 05291eb65d9..f617f26891d 100644
> --- a/gnu/services/dns.scm
> +++ b/gnu/services/dns.scm
> @@ -27,6 +27,7 @@ (define-module (gnu services dns)
> #:use-module (gnu system shadow)
> #:use-module (gnu packages admin)
> #:use-module (gnu packages dns)
> + #:use-module (guix deprecation)
> #:use-module (guix packages)
> #:use-module (guix records)
> #:use-module (guix gexp)
> @@ -742,8 +743,13 @@ (define-record-type* <dnsmasq-configuration>
> dnsmasq-configuration?
> (package dnsmasq-configuration-package
> (default dnsmasq)) ;file-like
> - (provision dnsmasq-provision
> - (default '(dnsmasq)))
> + (provision dnsmasq-configuration-provision ; deprecated
> + (default #f)
> + (sanitize warn-deprecated-dnsmasq-configuration-provision))
> + (shepherd-provision dnsmasq-configuration-shepherd-provision
> + (default '(dnsmasq)))
> + (shepherd-requirement dnsmasq-configuration-shepherd-requirement
> + (default '(user-processes networking)))
Since we're busting our 80 columns max width coding style here, I'd
refrain from indenting the right hand side as a block. You may need to
reformat lines (break them) so that it fits under 80 chars too.
> (no-hosts? dnsmasq-configuration-no-hosts?
> (default #f)) ;boolean
> (port dnsmasq-configuration-port
> @@ -799,9 +805,19 @@ (define-record-type* <dnsmasq-configuration>
> (tftp-unique-root dnsmasq-tftp-unique-root
> (default #f))) ;"" or "ip" or "mac"
>
> +(define (warn-deprecated-dnsmasq-configuration-provision value)
> + (when (pair? value)
> + (warn-about-deprecation
> + 'provision #f
> + #:replacement 'shepherd-provision))
> + value)
> +
Yeah, I think that's the best we can do currently with deprecation for
guix record fields. It'd be nice to add deprecation support builtin and
have source info, maybe.
> (define (dnsmasq-shepherd-service config)
> (match-record config <dnsmasq-configuration>
> (package
> + provision
> + shepherd-provision
> + shepherd-requirement
> no-hosts?
> port local-service? listen-addresses
> resolv-file no-resolv?
> @@ -815,8 +831,8 @@ (define (dnsmasq-shepherd-service config)
> tftp-lowercase? tftp-port-range
> tftp-root tftp-unique-root extra-options)
> (shepherd-service
> - (provision (dnsmasq-provision config))
> - (requirement '(user-processes networking))
> + (provision (or provision shepherd-provision))
> + (requirement shepherd-requirement)
> (documentation "Run the dnsmasq DNS server.")
> (start #~(make-forkexec-constructor
> (list
Other than these nitpicks, it LGTM. I'm not done reviewing 2 and 3, but
after I do so, could you please send a v2 with the above adjusted?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.