Subject: [bug#74801] [PATCH v2] gnu: home: services: Add home-mpv-service-type.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 03 Apr 2025 22:25:30 +0900

Hi,

Tomas Volf <[email-masked]> writes:

> This commit adds a new service type to generate configuration file for the mpv
> media player.
>
> Originally I attempted to use Guix Records (via define-configuration) for
> this, but ran into the bug #74748, so I had to switch to procedures instead.
> The usage is (hopefully) sufficiently described in the documentation.  When
> the bug is resolved, I will update it to use define-configuration instead.
>
> The full list of supported options is documented, however I decided to *not*
> document types and purpose for each individual fields.  While I had mostly
> working prototype to extract the documentation from mpv, once I realized it
> would be few 10k of lines added, I decided it is not worth it.  It would bloat
> the .texi file (by more than 50%), be hard to maintain and, in my opinion,
> would not provide enough value to justify that.  The current version seems
> like sane middle ground.

Wow! I didn't know mpv add so many configurable switches.

[...]

> +Due to the bug #74748, it does not use Guix Records to represent the
> +configuration, but uses keyword arguments to achieve similar result.

> +Example follows:
> +
> +@lisp
> +(service home-mpv-service-type
> +         (make-home-mpv-configuration
> +          #:global (make-mpv-profile-configuration
> +                    #:fullscreen #t
> +                    #:alang '("jpn" "eng"))))
> +@end lisp

This looks reasonable if #74748 can't be resolved (I suspect it may
cause an itch to Ludovic, but their plate is pretty full already I
assume!).

[...]

> +@table @asis
> +@item Flags
> +The value is evaluated for truthfulness.  Typically you would use
> +@code{#f} or @code{#t}.

nitpick: I'd leave out the implementation details (the phrase about
truthfulness) and simply mention these expect a boolean value, #t or #f.

[...]

> +Only set fields are outputted to the configuration file.  Accessors are
> +provided for every field, returning either their value or a sentinel
> +object @code{%unset}.

This should be %unset-value, which is already defined in (gnu services
configuration).

[...]

> +
> +;;;
> +;;; Basic types.
> +;;;
> +(define (serialize-type/boolean field-name value)

Nitpick: it's more common in the code base to name serializers as
'serialize-boolean'; it'd be nicer to stick to that naming style, for
consistency.

> +  #~(string-append #$(symbol->string field-name)
> +                   "="
> +                   #$(if value "yes" "no")
> +                   "\n"))
> +(define type/boolean?
> +  (const #t))

Does the above really check anything?

(type/boolean? "string")
$25 = #t

(type/boolean? 0)
$26 = #t

Seems like no.

> +(define (serialize-type/integer field-name value)
> +  #~(string-append #$(symbol->string field-name)
> +                   "="
> +                   #$(number->string value)
> +                   "\n"))
> +(define (type/integer? n)
> +  ;; We assume integer is a signed 32bit number.
> +  (and-let* (((integer? n))
> +             ((>= n -2147483648))
> +             ((<= n  2147483647)))))

I'd use the maths to compute the values, for clarity and ensuring
correctness, e.g. (* -1 (expt 2 (1- 32))) and (1- (expt 2 (1- 32))).

> +
> +(define (serialize-type/integer64 field-name value)
> +  #~(string-append #$(symbol->string field-name)
> +                   "="
> +                   #$(number->string value)
> +                   "\n"))
> +(define (type/integer64? n)
> +  ;; We assume integer is a signed 64bit number.
> +  (and-let* (((integer? n))
> +             ((>= n -9223372036854775808))
> +             ((<= n  9223372036854775807)))))

Likewise.

> +(define (serialize-type/string field-name value)
> +  #~(string-append #$(symbol->string field-name)
> +                   "="
> +                   #$value
> +                   "\n"))
> +(define type/string?
> +  string?)
> +
> +(define (serialize-type/float field-name value)
> +  #~(string-append #$(symbol->string field-name)
> +                   "="
> +                   #$(number->string (exact->inexact value))
> +                   "\n"))
> +(define type/float?
> +  ;; I am not sure how to validate floats.
> +  real?)

Maybe inexact? would be closer.  For floats you could check that

(type/integer? (inexact->exact (truncate value))) is true.

For doubles you could check that

(type/integer64? (inexact->exact (truncate value))) is true.

I think.

For the rest, it looks good to me, except that you throw old fashioned
exception values.  Please take a look in the code base to see how
raising error message exceptions that get shown nicely by the Guix command
line.

Maybe define-compile-time-procedure from (guix combinators) can be used,
see it used for example in (gnu services base) for `assert-valid-name'.

Could you send a v2 with the above taken into consideration?

-- 
Thanks,
Maxim