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

[Tomas Volf]

Hi Maxim,

thanks a lot for the review! :)

I have implemented most of your comments, only remaining question is
about the type/... pattern, see below.

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> Tomas Volf <~@wolfsden.cz> 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.

I was also surprised. :)

>
>
> [...]
>
>> +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.
>

I have liked the flexibility (no need to wrap procedure calls in
->bool), but will change this to #t/#f only, it does not matter much.

>
>> +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).

I did not like that someone could set the field to '%unset-marker%, and
it would be treated the same way as %unset-value.  That is why my %unset
value was a list '(*unset*), to ensure uniqueness as far as eq? goes.

However I will give you that this is somewhat unlikely to cause any
issues in practice, so I will yield here and use %unset-value instead.

>
> [...]
>
>> +
>> +;;;
>> +;;; 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.

But it does follow the same pattern.  The type is named type/boolean.
So the pattern of serialize-$TYPE results in serialize-type/boolean.
Without the type/ prefix I would run into collisions, since I need a
predicate for an integer (type/integer?), but integer? is already a
procedure doing something else.  I am not sure I want to shadow it.

Hm, would it make it better for you if I replaced the type/ prefix with
mpv/ prefix?  So mpv/integer, instead of type/integer?  That would
result in serialize-mpv/boolean, which might be better in your eyes?

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

It does not, by design. :)

>
> (type/boolean? "string")
> $25 = #t
>
> (type/boolean? 0)
> $26 = #t
>
> Seems like no.

Well, since any object is acceptable as test in an if, the (const #t)
seemed appropriate.  Now that I have per your suggestion above switched
to #t or #f instead of truthfulness, I will update the check to check
for #t or #f instead.

>
>> +(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))).

Done.

>
>> +
>> +(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.

Done.

>
>> +(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.

However values satisfying integer? should be accepted as well, at least
from the user point of view it should be fine to write just 2, not 2.0,
so inexact? does not seem ideal here.

> 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.

I am not sure this is correct, since floats/doubles can have large range
than integers.  So, if you do not mind too much, I will leave the real?
for now.  In practice I believe it should work well enough.

>
> 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'.

I was not able to wrap my head around define-compile-time-procedure, but
I took some other inspiration in the (gnu services base) module and used
formatted-message.  Seems to be printed well by guix build.

--8<---------------cut here---------------start------------->8---
guix build: error: option fullscreena not found for mpv-profile-configuration
--8<---------------cut here---------------end--------------->8---

and

--8<---------------cut here---------------start------------->8---
guix build: error: invalid mpv configuration for fullscreen: yes
--8<---------------cut here---------------end--------------->8---

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

Once I know what to do with the type/..., will send. :)

Tomas

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.