Subject: Re: [bug#77396] [PATCH] services: Add ngircd-service-type.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 03 Apr 2025 15:36:40 +0900

Hi!

Thanks for the prompt review!

Ludovic Courtès <[email-masked]> writes:

[...]

>> +@cindex IRC (Internet Relay Chat)
>> +
>> +@url{https://ngircd.barton.de/, ngIRCd}, is a lightweight @acronym{IRCd,
>> +Internet Relay Chat daemon}, which can be used to host your own IRC
>> +server.
>
> Could you add an example configuration, as is usually done for services?
> It’s always nice to have something to copy/paste to get started.

I've added a not-too-serious one:

--8<---------------cut here---------------start------------->8---
modified   doc/guix.texi
@@ -30365,6 +30365,21 @@ Messaging Services
 @defvar ngircd-service-type
 The service type for ngIRCd.  Its value is a @code{ngircd-configuration}
 object, documented below.
+
+A simple example configuration could look like:
+
+@lisp
+(service ngircd-service-type
+         (ngircd-configuration
+          (channels
+           (list (ngircd-channel
+                  (name "#fruits")
+                  (topic "All things fruits -- veggies are off-topic"))))
+          (operators
+           (list (ngircd-operator
+                  (name "mikan")
+                  (password "tomatoes-are-fruits/carrots-are-not"))))))
+@end lisp
 @end defvar
 
 @c To regenerate the rest of this section documentation, use the
--8<---------------cut here---------------end--------------->8---

>> +            <ngircd-configuration>
>> +            ngircd-configuration
>> +            ngircd-configuration?
>> +            <ngircd-global>
>> +            ngircd-global
>> +            ngircd-global?
>> +            <ngircd-limits>
>> +            ngircd-limits
>> +            ngircd-limits?
>> +            <ngircd-options>
>> +            ngircd-options
>> +            ngircd-options?
>> +            <ngircd-ssl>
>> +            ngircd-ssl
>> +            ngircd-ssl?
>> +            <ngircd-operator>
>> +            ngircd-operator
>> +            ngircd-operator?
>> +            <ngircd-server>
>> +            ngircd-server
>> +            ngircd-server?
>> +            <ngircd-channel>
>
> Please don’t export record type descriptors like <ngircd-configuration>
> since that makes it impossible to provide any guarantee (ABI, validity
> of fields, etc.).

Since there would be so many fields to export, I was hoping to punt on
exporting all individual accessors, and at least let users be able to
use 'match-record', which requires the record type.  Isn't match-record
intended to be used by users as well as service authors?

>> +(define (ngircd-shepherd-service config)
>> +  (match-record config <ngircd-configuration>
>> +                (ngircd debug? global)
>> +    (let ((ngircd.conf (serialize-ngircd-configuration config))
>> +          (ngircd (file-append ngircd "/sbin/ngircd"))
>> +          (pid-file (ngircd-global-pid-file global))
>> +          (user group (ngircd-user+group config)))
>> +      (list (shepherd-service
>> +             (provision '(ngircd))
>> +             (requirement '(user-processes networking syslogd))
>
> I would drop ‘networking’: see <https://issues.guix.gnu.org/66306>.

I've read the link above, and I think it's probably safer to keep it,
since the interfaces that should be listened can be configured by the
user.  Also, the 'contrib' systemd service uses 'After=network.target'
[0].

[0]  https://github.com/ngircd/ngircd/blob/512af135d06e7dad93f51eae51b3979e1d4005cc/contrib/ngircd.service#L7

>> +             (actions (list (shepherd-configuration-action ngircd.conf)))
>> +             (start #~(make-forkexec-constructor
>> +                       (append (list #$(ngircd-wrapper config)
>> +                                     "--nodaemon" "--syslog"
>
> I’d use #:log-file and drop ‘--syslog’; I find it more convenient.

Do you find it more convenient because of the new 'herd log service'
functionality when #:log-file is used?  Otherwise I usually prefer
searching messages in a single place as opposed to various log files.

>> +                                     "--config" #$ngircd.conf)
>> +                               (if #$debug?
>> +                                   '("--debug")
>> +                                   '()))
>> +                       #:pid-file #$pid-file))
>
> If ngircd supports socket activation, I’d suggest using
> ‘make-systemd-constructor’ instead of #:pid-file: it equally achieves
> startup synchronization, but it allows for shorter startup times and can
> start the daemon lazily on-demand.

It's a bit weird to me that something as permanent as an IRC server
should be lazily started by socket activation, but it does support that,
and it simplifies things a bit, so I've made the switch, like so:

--8<---------------cut here---------------start------------->8---
1 file changed, 44 insertions(+), 43 deletions(-)
gnu/services/messaging.scm | 87 ++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------

modified   gnu/services/messaging.scm
@@ -1040,9 +1040,9 @@ (define-configuration ngircd-global     ;[Global]
    "Info text of the server. This will be shown by WHOIS and LINKS requests
 for example.")
   (listen
-   (maybe-list-of-strings (list "::" "0.0.0.0"))
+   (list-of-strings (list "::" "0.0.0.0"))
    "A list of IP address on which the server should listen.  By default it
-listens on all interfaces.")
+listens on all configured IP addresses and interfaces.")
   (motd-file
    ;; Provide an empty default file to avoid a warning when running --conftest
    ;; in the activation script.
@@ -1064,10 +1064,11 @@ (define-configuration ngircd-global     ;[Global]
 no password is required.  PAM must be disabled for this option to have an
 effect.")
   (pid-file
-   (string "/run/ngircd/ngircd.pid")
-   "The file name where the PID of ngIRCd is written after it starts.")
+   maybe-string
+   "The file name where the PID of ngIRCd should be written after it starts.
+By default, no PID file is created.")
   (ports
-   (maybe-list-of-ports (list 6667))
+   (list-of-ports (list 6667))
    "Port number(s) on which the server should listen for @emph{unencrypted}
 connections.")
   (server-uid
@@ -1207,7 +1208,7 @@ (define-configuration ngircd-ssl        ;[SSL]
    "File name of the SSL Server Key to be used for SSL connections, which is
 required for SSL/TLS support.")
   (ca-file
-   (string "/etc/ssl/certs/ca-certificates.crt")
+   (maybe-string "/etc/ssl/certs/ca-certificates.crt")
    "A file listing all the certificates of the trusted Certificate
 Authorities.")
   (ports
@@ -1439,7 +1440,6 @@ (define (ngircd-wrapper config)
   (let* ((ngircd.conf (serialize-ngircd-configuration config))
          (user group (ngircd-user+group config))
          (global (ngircd-configuration-global config))
-         (pid-file (ngircd-global-pid-file global))
          (help-file (ngircd-global-help-file global))
          (motd-file (ngircd-global-motd-file global))
          (ssl (ngircd-configuration-ssl config))
@@ -1460,11 +1460,7 @@ (define (ngircd-wrapper config)
              (target source))
             (file-system-mapping
              (source ngircd.conf)
-             (target source))
-            (file-system-mapping
-             (source (string-append (dirname pid-file)))
-             (target source)
-             (writable? #t)))
+             (target source)))
       (if (maybe-value-set? help-file)
           (list (file-system-mapping
                  (source help-file)
@@ -1509,53 +1505,58 @@ (define (ngircd-wrapper config)
      #:user user
      #:group group
      ;; ngircd wants to look up users in /etc/passwd so run in the global user
-     ;; namespace.  Also preserve the PID namespaces otherwise the PID file
-     ;; would contain an unrelated PID number and confuse Shepherd.
-     #:namespaces (fold delq %namespaces '(net pid user)))))
+     ;; namespace.
+     #:namespaces (fold delq %namespaces '(net user)))))
 
 (define (ngircd-shepherd-service config)
   (match-record config <ngircd-configuration>
-                (ngircd debug? global)
-    (let ((ngircd.conf (serialize-ngircd-configuration config))
-          (ngircd (file-append ngircd "/sbin/ngircd"))
-          (pid-file (ngircd-global-pid-file global))
-          (user group (ngircd-user+group config)))
+                (ngircd debug? global ssl)
+    (let* ((ngircd.conf (serialize-ngircd-configuration config))
+           (ngircd (file-append ngircd "/sbin/ngircd"))
+           (addresses (ngircd-global-listen global))
+           (ports* (ngircd-global-ports global))
+           (ports (if (and (maybe-value-set? ssl)
+                           (maybe-value-set? (ngircd-ssl-ports ssl)))
+                      (append ports* (ngircd-ssl-ports ssl))
+                      ports*)))
       (list (shepherd-service
              (provision '(ngircd))
              (requirement '(user-processes networking syslogd))
+             (modules (cons '(srfi srfi-1) %default-modules))
              (actions (list (shepherd-configuration-action ngircd.conf)))
-             (start #~(make-forkexec-constructor
+             (start #~(make-systemd-constructor
                        (append (list #$(ngircd-wrapper config)
                                      "--nodaemon" "--syslog"
                                      "--config" #$ngircd.conf)
                                (if #$debug?
                                    '("--debug")
                                    '()))
-                       #:pid-file #$pid-file))
-
-             (stop  #~(make-kill-destructor)))))))
+                       ;; Compute endpoints for each listen addresses/ports
+                       ;; combinations.
+                       (append-map
+                        (lambda (port)
+                          (map (lambda (addr)
+                                 (endpoint
+                                  (addrinfo:addr
+                                   (car (getaddrinfo
+                                         addr
+                                         (number->string port)
+                                         (logior AI_NUMERICHOST
+                                                 AI_NUMERICSERV))))))
+                               (list #$@addresses)))
+                        (list #$@ports))))
+             (stop  #~(make-systemd-destructor)))))))
 
 (define (ngircd-activation config)
   (let* ((ngircd (file-append (ngircd-configuration-ngircd config)))
-         (pid-file (ngircd-global-pid-file
-                    (ngircd-configuration-global config)))
-         (ngircd.conf (serialize-ngircd-configuration config))
-         (user _ (ngircd-user+group config)))
-    (with-imported-modules (source-module-closure
-                            '((gnu build activation)))
-      #~(begin
-          (use-modules (gnu build activation)
-                       (ice-9 match))
-          (define pw (match #$user
-                       ((? number?) (getpwuid #$user))
-                       ((? string?) (getpwnam #$user))))
-          (mkdir-p/perms #$(dirname pid-file) pw #o755)
-          (system (string-join
-                   (list #$(file-append ngircd "/sbin/ngircd")
-                         "--configtest" "--config" #$ngircd.conf
-                         ;; Ensure stdin is not a TTY to avoid pausing for a key
-                         ;; during boot when a problem is detected.
-                         "<" "/dev/null")))))))
+         (ngircd.conf (serialize-ngircd-configuration config)))
+    #~(begin
+        (system (string-join
+                 (list #$(file-append ngircd "/sbin/ngircd")
+                       "--configtest" "--config" #$ngircd.conf
+                       ;; Ensure stdin is not a TTY to avoid pausing for a key
+                       ;; during boot when a problem is detected.
+                       "<" "/dev/null"))))))
 
 (define ngircd-service-type
   (service-type
--8<---------------cut here---------------end--------------->8---

>> +        (mkdir-p/perms #$(dirname pid-file) pw #o755)
>> +        (system (string-join
>> +                 (list #$(file-append ngircd "/sbin/ngircd")
>> +                       "--configtest" "--config" #$ngircd.conf
>> +                       ;; Ensure stdin is not a TTY to avoid pausing for a key
>> +                       ;; during boot when a problem is detected.
>> +                       "<" "/dev/null"))))))
>
> I think you can do:
>
>   (parameterize ((current-input-port (%make-void-port "r")))
>     (system* #$(file-append …) "--configtest" …))

Neat!  I seemed to remember a buggy Guile interaction between 'system'
and stdin/stdout redirections from Guile (e.g. bug#43364), but it
doesn't seem to be an issue with your suggestion above.

> But! if it’s about checking the configuration, I would do it in a
> derivation (instead of at activation time), similar to how this is done
> for mcron.

Hm, I have looked at the mcron service and others, but haven't found it.
Could you please point me to the exact file/line?

>> +          (test-assert "ngircd listens on TCP port 6667"
>> +            (wait-for-tcp-port 6667 marionette))
>
> Maybe try a /JOIN command or whatever?

Done, using the 'ii' minimalist IRC client, with something like this:

--8<---------------cut here---------------start------------->8---
2 files changed, 81 insertions(+), 28 deletions(-)
gnu/services/messaging.scm | 12 +++++++-----
gnu/tests/messaging.scm    | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------

modified   gnu/services/messaging.scm
@@ -1174,11 +1174,13 @@ (define-configuration ngircd-options    ;[Options]
 requests by non-chanops as if they were coming from the server.  Only enable
 this if you have ircd-irc2 servers in your IRC network.")
   (pam?
-   (maybe-boolean #t)
-   "Set to @code{#f} to disable all calls to the PAM library at runtime; all
-users connecting without password are allowed to connect, all passwords given
-will fail.  Users identified without PAM are registered with a
-tilde (@samp{~}) prepended to their user name.")
+   (boolean #f)
+   "Set to @code{#t} to enable calls to the PAM library at runtime; all users
+connecting without password are allowed to connect, all passwords given will
+fail.  Users identified without PAM are registered with a tilde (@samp{~})
+prepended to their user name.  This defaults to @code{#f} in Guix because the
+service runs as a unpriveleged user and thus cannot authenticate other users
+via the @code{pam_unix} PAM module.")
   (pam-is-optional?
    (maybe-boolean #f)
    "Set to @code{#t} to make PAM authentication optional, causing clients not
modified   gnu/tests/messaging.scm
@@ -24,9 +24,13 @@ (define-module (gnu tests messaging)
   #:use-module (gnu system)
   #:use-module (gnu system vm)
   #:use-module (gnu services)
+  #:use-module (gnu services base)
   #:use-module (gnu services messaging)
   #:use-module (gnu services networking)
+  #:use-module (gnu services ssh)
+  #:use-module (gnu packages irc)
   #:use-module (gnu packages messaging)
+  #:use-module (gnu packages screen)
   #:use-module (guix gexp)
   #:use-module (guix store)
   #:use-module (guix modules)
@@ -225,32 +229,50 @@ (define %test-bitlbee
 ;;;
 
 (define %ngircd-os
-  (marionette-operating-system
-   (simple-operating-system
-    (service dhcp-client-service-type)
-    (service ngircd-service-type
-             (ngircd-configuration
-              (debug? #t)
-              (global
-               (ngircd-global
-                (server-uid 990)
-                (server-gid 990)))
-              ;; There is no need to serialize the following sections, which
-              ;; are all optional, but include them anyway to test the
-              ;; serializers.
-              (limits (ngircd-limits))
-              (options (ngircd-options))
-              (ssl (ngircd-ssl))
-              (operators (list (ngircd-operator
-                                (name "maxim")
-                                (password "1234"))))
-              (channels (list (ngircd-channel
-                               (name "#guix")))))))
-   #:imported-modules (source-module-closure '((gnu services herd)))))
+  (operating-system
+    (inherit %simple-os)
+    (packages (cons* ii screen %base-packages))
+    (services
+     (cons*
+      (service dhcp-client-service-type)
+      ;; For ease of debugging.  Run the vm with:
+      ;; '-nic user,model=virtio-net-pci,hostfwd=tcp::10022-:22'
+      (service openssh-service-type     ;for ease of debugging
+               (openssh-configuration
+                (permit-root-login #t)
+                (allow-empty-passwords? #t)))
+      (service ngircd-service-type
+               (ngircd-configuration
+                (debug? #t)
+                (global
+                 (ngircd-global
+                  (server-uid 990)
+                  (server-gid 990)))
+                ;; There is no need to serialize the following sections, which
+                ;; are all optional, but include them anyway to test the
+                ;; serializers.
+                (limits (ngircd-limits))
+                (options (ngircd-options))
+                (ssl (ngircd-ssl))
+                (operators (list (ngircd-operator
+                                  (name "apteryx")
+                                  (password "1234"))))
+                (channels
+                 (list (ngircd-channel
+                        (name "#guix")
+                        (topic "GNU Guix | https://guix.gnu.org"))))))
+      %base-services))))
 
 (define (run-ngircd-test)
   (define vm
-    (virtual-machine (operating-system %ngircd-os)))
+    (virtual-machine
+     (operating-system
+       (marionette-operating-system
+        %ngircd-os
+        #:imported-modules (source-module-closure
+                            '((gnu build dbus-service)
+                              (guix build utils)
+                              (gnu services herd)))))))
 
   (define test
     (with-imported-modules '((gnu build marionette))
@@ -274,6 +296,35 @@ (define (run-ngircd-test)
           (test-assert "ngircd listens on TCP port 6667"
             (wait-for-tcp-port 6667 marionette))
 
+          (test-assert "basic irc operations function as expected"
+            (marionette-eval
+             '(begin
+                (use-modules ((gnu build dbus-service) #:select (with-retries))
+                             (ice-9 textual-ports))
+
+                (define (write-command command)
+                  (call-with-output-file "in"
+                    (lambda (port)
+                      (display (string-append command "\n") port))))
+
+                (define (grep-output text)
+                  (with-retries 5 1     ;retry for 5 seconds
+                    (string-contains (call-with-input-file "out" get-string-all)
+                                     text)))
+
+                (unless (zero? (system "ii -s localhost -i /tmp &"))
+                  (error "error connecting to irc server"))
+
+                (with-retries 5 1
+                  (chdir "/tmp/localhost")) ;move to FIFO directory
+
+                (write-command "/join #guix")
+                (grep-output "GNU Guix | https://guix.gnu.org")
+
+                (write-command "/oper apteryx 1234")
+                (grep-output "+o"))
+             marionette))
+
           (test-end))))
 
   (gexp->derivation "ngircd-test" test))

--8<---------------cut here---------------end--------------->8---

I'll send a v2 with the above changes.  The remaining points, pending my
above questions, are:

- Use dedicated log file?
- Move configuration check to a derivation

-- 
Thanks,
Maxim