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

Message #14 received at 74801@debbugs.gnu.org (full text, mbox, reply):

Received: (at 74801) by debbugs.gnu.org; 3 Apr 2025 16:34:10 +0000
From debbugs-submit-bounces@debbugs.gnu.org Thu Apr 03 12:34:10 2025
Received: from localhost ([127.0.0.1]:35404 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
	id 1u0NW5-0008Om-Re
	for submit@debbugs.gnu.org; Thu, 03 Apr 2025 12:34:10 -0400
Received: from wolfsden.cz ([37.205.8.62]:47386)
 by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)
 (Exim 4.84_2) (envelope-from <~@wolfsden.cz>) id 1u0NW2-0008Oa-E0
 for 74801@debbugs.gnu.org; Thu, 03 Apr 2025 12:34:07 -0400
Received: by wolfsden.cz (Postfix, from userid 104)
 id 26E9D29904F; Thu,  3 Apr 2025 16:34:03 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=wolfsden.cz; s=mail;
 t=1743698043; bh=j5Axaf+MgQTGmXBV0DRZbnZGB5+KD7z82hWydBf8HIU=;
 h=From:To:Cc:Subject:In-Reply-To:References:Date;
 b=v1fT4kDdv+McX6hgNMMR3bJzCohQLJckb9zJ8rX4azLf3OnuKcQSXtr7ltwEcpIaD
 vtwMRMM9/UUbkiJvxrSNh6AAPKfzZG3V4o0OLcsHBjcX21SFycpZ6AtAtFweJftD7F
 2BloGIzUp/AwIA1YU4iMY/uM9XCeR6xCYAwgwpZSTzXf/kE1F459iQ12g4zgL6RcWi
 ZchPaAMjkJY/NYGLiV+1yrszDzyXotH7sjXo3mjM4bX42Kz+IiIRXkksN7a/ahuDwK
 DSDtcInnKNqDZsMg0x3EndURF44yOT5N3FEBCFPs4KfHbETx7M5mqMOFz5DQbw8Nub
 QroM6Dt5Ta3KQZRzW6fDF7fBc6e2F36/y/n7e/Zx8z4J+qec4pMRl4Qtzim/s8npOp
 HCUurdpyM87BnbD1btLeDF8fvE+/Nv1y8YGbyys0vU7zMfaHZQnC072xNbocVZ+W+c
 ha0PU02vUvWzydvpslFNzs/gt4VbJQ1B222IYhCuh4KG/Bhj45aLwY8fzkoa3H/Bku
 XNZxBhkQ1nlXxW8kJXDMsmxUgbeqGlLENypz4C9rjyGDqImTWqv8tjjJhq0UjZSWQG
 fdW/qOAGyXGrRywfyus7NIGrx9mskZRktReKAjfidJVMPhEcw/icCuRPj4gO455yX/
 KHvZOQZYkoFQoERzhe/Ejmvk=
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on wolfsden
X-Spam-Level: 
X-Spam-Status: No, score=-3.1 required=5.0 tests=ALL_TRUSTED,BAYES_00,
 DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,URIBL_BLOCKED
 autolearn=ham autolearn_force=no version=3.4.6
Received: from localhost (unknown [128.0.188.242])
 by wolfsden.cz (Postfix) with ESMTPSA id 31692298BB7;
 Thu,  3 Apr 2025 16:34:02 +0000 (UTC)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=wolfsden.cz; s=mail;
 t=1743698042; bh=j5Axaf+MgQTGmXBV0DRZbnZGB5+KD7z82hWydBf8HIU=;
 h=From:To:Cc:Subject:In-Reply-To:References:Date;
 b=xW9gmXjKjhuX76thHKU56uApF+Y+0rNFM61MCrjog2t15yLAlq9N5JjnaE1lmI7a+
 tStW9DPZjAHaTVrg/TRCzHQpIPlwyy5c4Z2P3rxnyc+dwryxbwzOUb1QqF1304yvNX
 p8S9yl43lJdIHhQcMAVkQ6p+osB3h5mNcl76IGaXH2mLaKt1d5NP58ZTFOB9sTn4pV
 gyiEct4KGhHcsyhLMQUFPCekiuVTzRuJDUMQMEcun45NQHk012Ip3xe8lrO7VsN+Lx
 YiQy1BYLgc7Xk8OABWnJSleRT3gRNxaK7V2Q6A2F4Tu8z0Eu2EfHEaRy80TTLhgvlI
 qUV02d6R3T0d46MFqd3mHE01dF1TIxnrTbQxH/8WMVwZJKknw/bS5pEcohDdivEfAF
 DDb+txZ41rLIhIE0Tme7dmBKvjKbHgNa5Wzl1+869xT5yeEPHKFD3Ria3GsAYe98C0
 gxTxjgoOOxAGhoYpoDGpZXV749YAHnGj8r9SWLojczppvRDYOfWiLFBpXhZGVUtKTI
 QdL2Sw8OZtfKqx6CKWZqTRtg7z7cl/rZfuojDtmtwozZ/jSFlnDJGFHPIDvEqF1ZYX
 Xk967GV5n+8QzW3/exLpBFbe+HBT4YsGprZ1sLdSz10MzISOPpM/f+sKo6lvQN4Pd+
 xihKGbPeu6lVbxKLzYCvs+tA=
From: Tomas Volf <~@wolfsden.cz>
To: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Subject: Re: [bug#74801] [PATCH v2] gnu: home: services: Add
 home-mpv-service-type.
In-Reply-To: <87y0whxtpx.fsf@gmail.com> (Maxim Cournoyer's message of "Thu, 03
 Apr 2025 22:25:30 +0900")
References: <29d3cc661a49b7bef951041f8ec3c7600c9777a7.1733953822.git.~@wolfsden.cz>
 <50f9a741440bc9b89ecdabcd7afb3a324173f7f9.1743602733.git.~@wolfsden.cz>
 <87y0whxtpx.fsf@gmail.com>
Date: Thu, 03 Apr 2025 18:34:01 +0200
Message-ID: <87mscxw6fa.fsf@wolfsden.cz>
User-Agent: Gnus/5.13 (Gnus v5.13)
MIME-Version: 1.0
Content-Type: text/plain
X-Spam-Score: 0.0 (/)
X-Debbugs-Envelope-To: 74801
Cc: 74801@debbugs.gnu.org
X-BeenThere: debbugs-submit@debbugs.gnu.org
X-Mailman-Version: 2.1.18
Precedence: list
List-Id: <debbugs-submit.debbugs.gnu.org>
List-Unsubscribe: <https://debbugs.gnu.org/cgi-bin/mailman/options/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=unsubscribe>
List-Archive: <https://debbugs.gnu.org/cgi-bin/mailman/private/debbugs-submit/>
List-Post: <mailto:debbugs-submit@debbugs.gnu.org>
List-Help: <mailto:debbugs-submit-request@debbugs.gnu.org?subject=help>
List-Subscribe: <https://debbugs.gnu.org/cgi-bin/mailman/listinfo/debbugs-submit>, 
 <mailto:debbugs-submit-request@debbugs.gnu.org?subject=subscribe>
Errors-To: debbugs-submit-bounces@debbugs.gnu.org
Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org>
X-Spam-Score: -1.0 (-)

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.

Send a report that this bug log contains spam.