Subject: Re: [bug#62969] [PATCH] home: Add msmtp service.

From: Bruno Victal <[email-masked]>
Date: Thu, 20 Apr 2023 17:36:59 +0100

Hi Tanguy,

On 2023-04-20 15:42, Tanguy Le Carrour wrote:
> - one that I don't know how to solve
> ```
> +  ;; FIXME `In procedure every: Wrong type argument: #<syntax-transformer msmtp-account?>`
> +  ;(every msmtp-account? lst))
> ```

Place this after '(define-configuration mstmp-account ...)'. You might want to place this block
before '(define-configuration msmtp-configuration ...)'.

> +@deftp {Data Type} home-msmtp-configuration
> +Available @code{home-msmtp-configuration} fields are:
> +
> +@table @asis
> +@item @code{defaults} (type: msmtp-configuration)
> +The configuration that will be set as default for all accounts.
> +
> +@item @code{accounts} (default: @code{()}) (type: list-of-msmtp-accounts)
> +A list of @code{msmtp-account} records which contain information about
> +all your accounts.
> +
> +@item @code{default-account} (type: maybe-string)
> +Set the default account.
> +
> +@item @code{extra-content} (default: @code{""}) (type: raw-configuration-string)
> +Extra content appended as-is to the configuration file.  Run
> +@command{man msmtp} for more information about the configuration file
> +format.
> +
> +@end table
> +
> +@end deftp

You should preserve the @c lines from configuration->documentation to make it clear that
the block of text was generated.

> +
> +(define-module (gnu home services mail)
> +  #:use-module (guix gexp)
> +  #:use-module (gnu packages)
> +  #:use-module (gnu services)
> +  #:use-module (gnu services configuration)
> +  #:use-module (gnu home services)
> +  #:use-module (gnu home services shepherd)
> +  #:use-module (ice-9 string-fun)
> +  #:use-module (srfi srfi-26)
> +  #:export (home-msmtp-configuration
> +            home-msmtp-configuration?
> +            home-msmtp-service-type
> +            msmtp-account
> +            msmtp-configuration))

You should export the accessors for the fields.
You can use this snippet within 'guix repl' to automate the typing for you:

--8<---------------cut here---------------start------------->8---
(define (helper-configuration-exports fields)
  (map
   (lambda (s)
     (let* ((f (compose object->string configuration-field-getter))
           (g (compose cadr string-tokenize))
           (s* ((compose g f) s)))
       (string->symbol (substring s* 1 (string-contains s* "-procedure")))))
   fields))

(define (helper-formatted-exports fields)
  (format #t "~{~a~%~}" (helper-configuration-exports fields)))
--8<---------------cut here---------------end--------------->8---

Example usage:

--8<---------------cut here---------------start------------->8---
;; paste snippet above into repl

scheme@(guix-user)> ,m (gnu services audio)
scheme@(gnu services audio)> (helper-formatted-exports mpd-configuration-fields)
mpd-configuration-user
mpd-configuration-group
mpd-configuration-shepherd-requirement
mpd-configuration-environment-variables
mpd-configuration-log-file
mpd-configuration-log-level
mpd-configuration-music-directory
mpd-configuration-music-dir
mpd-configuration-playlist-directory
mpd-configuration-playlist-dir
mpd-configuration-db-file
mpd-configuration-state-file
mpd-configuration-sticker-file
mpd-configuration-default-port
mpd-configuration-endpoints
mpd-configuration-address
mpd-configuration-database
mpd-configuration-partitions
mpd-configuration-neighbors
mpd-configuration-inputs
mpd-configuration-archive-plugins
mpd-configuration-input-cache-size
mpd-configuration-decoders
mpd-configuration-resampler
mpd-configuration-filters
mpd-configuration-outputs
mpd-configuration-playlist-plugins
mpd-configuration-extra-options
$1 = #t
--8<---------------cut here---------------end--------------->8---

> +
> +(define raw-configuration-string? string?)

This isn't necessary, continued below. [1]

> +
> +(define (configuration-serialize-maybe-string field-name value)
> +  #~(if #$(maybe-value-set? value)
> +      (string-append #$(uglify-symbol field-name) " " #$value "\n")
> +      ""))> +
> +(define (configuration-serialize-maybe-integer field-name value)
> +  #~(if #$(maybe-value-set? value)
> +      (string-append #$(uglify-symbol field-name) " " (number->string #$value) "\n")
> +      ""))
> +
> +(define (configuration-serialize-maybe-boolean field-name value)
> +  #~(if #$(maybe-value-set? value)
> +      (string-append #$(uglify-symbol field-name) " " (if #$value "on" "off") "\n")
> +      ""))
You don't have to perform the maybe-value-set? checks, it is automatically done for you.
The only cases where this isn't true is if you explicitly override the serializer in
define-configuration. [2]

> +(define (account-serialize-string field-name value)
> +  #~(string-append " " #$(uglify-symbol field-name) " " #$value "\n"))
> +
> +(define (account-serialize-string field-name value)
> +  #~(string-append " " #$(uglify-symbol field-name) " " #$value "\n"))

Duplicated?

> +
> +(define (account-serialize-msmtp-configuration field-name value)
> +  ; FIXME Begin each line inside an account section with a space.
> +  #~(string-append #$(serialize-configuration value msmtp-configuration-fields)))

This doesn't do anything and since it's a cosmetic change I'd just ignore it, since
the file is managed with guix anyways.

> +(define (home-configuration-serialize-default-account field-name value)
> +  #~(if #$(maybe-value-set? value)
> +      (string-append "\naccount default : " #$value "\n")
> +      ""))

See [2] above.

> +;; Source <https://marlam.de/msmtp/msmtp.html#Configuration-files>.
> +(define-configuration msmtp-configuration
> +  (auth?
> +   maybe-boolean
> +   "Enable or disable authentication.")
> +  (tls?
> +   maybe-boolean
> +   "Enable or disable TLS (also known as SSL) for secured connections.")
> +  (tls-starttls
> +   maybe-boolean
> +   "Choose the TLS variant: start TLS from within the session (‘on’, default),
> +or tunnel the session through TLS (‘off’).")
> +  (tls-trust-file
> +   maybe-string
> +   "Activate server certificate verification using a list of
> +trusted Certification Authorities (CAs).")
> +  (logfile
> +   maybe-string
> +   "Enable logging to the specified file. An empty argument disables logging.
> +The file name ‘-’ directs the log information to standard output.")
> +  (host
> +    maybe-string
> +    "The SMTP server to send the mail to.")
> +  (port
> +    maybe-integer
> +    "The port that the SMTP server listens on. The default is 25 (\"smtp\"),
> +unless TLS without STARTTLS is used, in which case it is 465 (\"smtps\").")
> +  (user
> +    maybe-string
> +    "Set the user name for authentication.")
> +  (from
> +    maybe-string
> +    "Set the envelope-from address.")
> +  (passwordeval
> +    maybe-string
> +    "Set the password for authentication to the output (stdout) of the command cmd.")
> +  (extra-content
> +   (raw-configuration-string "")
> +   "Extra content appended as-is to the configuration block.  Run
> +@command{man msmtp} for more information about the configuration file
> +format.")

Instead of defining a raw-configuration-string? predicate, simply use string?
and set the serializer to '(serializer my-custom-string-serializer)' or '(serializer (lambda ...))'. [1]

> +  (prefix configuration-))

These are poor prefix choices for a module named (gnu home services mail).
If it were (gnu home services msmtp) or (gnu home services mail msmtp) it would be acceptable
but a module named (gnu home services mail) is expected to eventually contain multiple services that
have to co-exist. These prefixes are too non-specific and confusion prone for such circumstances.

I'd set this to (prefix msmtp-configuration-). (the same logic applies to the remaining (prefix ...) lines)

> +
> +(define-configuration msmtp-account
> +  (name
> +   (string)
> +   "The unique name of the account."
> +   (serializer account-serialize-name))
> +  (configuration
> +   (msmtp-configuration)
> +   "The configuration for this given account.")
> +  (prefix account-))
> +
> +(define-configuration home-msmtp-configuration
> +  (defaults
> +   (msmtp-configuration (msmtp-configuration))
> +   "The configuration that will be set as default for all accounts.")
> +  (accounts
> +   (list-of-msmtp-accounts '())
> +   "A list of @code{msmtp-account} records which contain
> +information about all your accounts.")
> +  (default-account
> +   maybe-string
> +   "Set the default account."
> +   (serializer home-configuration-serialize-default-account))
> +  (extra-content
> +   (raw-configuration-string "")
> +   "Extra content appended as-is to the configuration file.  Run
> +@command{man msmtp} for more information about the configuration file
> +format.")
> +  (prefix home-configuration-))

You might want to separate each field with a space to make things easier to read but this is optional.

Cheers,
Bruno