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

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

Received: (at 74801) by debbugs.gnu.org; 3 Apr 2025 13:26:01 +0000
From debbugs-submit-bounces@debbugs.gnu.org Thu Apr 03 09:26:00 2025
Received: from localhost ([127.0.0.1]:33347 helo=debbugs.gnu.org)
	by debbugs.gnu.org with esmtp (Exim 4.84_2)
	(envelope-from <debbugs-submit-bounces@debbugs.gnu.org>)
	id 1u0KZz-0001jB-Qc
	for submit@debbugs.gnu.org; Thu, 03 Apr 2025 09:26:00 -0400
Received: from mail-pl1-x62f.google.com ([2607:f8b0:4864:20::62f]:46356)
 by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)
 (Exim 4.84_2) (envelope-from <maxim.cournoyer@gmail.com>)
 id 1u0KZv-0001iu-5W
 for 74801@debbugs.gnu.org; Thu, 03 Apr 2025 09:25:56 -0400
Received: by mail-pl1-x62f.google.com with SMTP id
 d9443c01a7336-227b828de00so8859505ad.1
 for <74801@debbugs.gnu.org>; Thu, 03 Apr 2025 06:25:55 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=gmail.com; s=20230601; t=1743686749; x=1744291549; darn=debbugs.gnu.org;
 h=mime-version:user-agent:message-id:date:references:in-reply-to
 :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to;
 bh=IC4gaCTmME5ThocGxpUrf5QPymDJ8ejRarSAMm9l878=;
 b=DiQsdfGdDw2E+srDa0X7TzTzdppMOEXgjQ6v4wlBuZDTwHulKrGAZYNHs4o0kdial7
 bcXoYpH+wJb8lzlEnJViHE75kSoZutnmeivVz/Lmx5NgR5u/rIhZvcz7jctC+XWa1yii
 kkhdQrplK8rf/GTI/2bfC0CmeR/VHdZTxrLnJsr+WbezYx+Q2t2nhWBk2UJLxxTVD2Vr
 Rqx1Q5l70OeGUvzp4y+m84uCLO5kJWF4NSun3+Vh964LWZ86gQT/91Fp9Vka274IVD3e
 OXFlJeTz7+rdXDNdif7gdxA3c7WMg+4JnAhsZN678Z0zrv6eTtizXAYvkzPHJ8x6S0KQ
 gnVg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1743686749; x=1744291549;
 h=mime-version:user-agent:message-id:date:references:in-reply-to
 :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date
 :message-id:reply-to;
 bh=IC4gaCTmME5ThocGxpUrf5QPymDJ8ejRarSAMm9l878=;
 b=xNdMuiM+ae5Bp9EO85ve5p3qcgtVA0oV/S5JgFZ0dAOqzoKLye2ZQ1l9DbvOJonsKz
 fkVP5hzStbF7lh669JNkH46R69ewqm3fA9i9QXQx9dhjH/hJkkovBm6SAppozJu1Bse8
 96d08OY96001F79EhnML+0tjEVuS+fPwqVQfdgX7aCES+OIFrokJkgBJ43VGzdvYLA1g
 Pc+nFe0G7EJsk6c1HpoxsqLD5N3BJ377Wv3PnrJCiAU81/PZFGi/DXjuwWrfKQMFmj2m
 Q9mv7Z1R7cm0x+ajoqldSfzv5BM9lAFaTgW7aKPBojXTuF4IOdMi5bVqoi7EbCG0jyLV
 CrCw==
X-Gm-Message-State: AOJu0YzK2w6UaMNREreklgqxi9/V+FsxN9h/5VqrHyQUJuOgfbmsb/8X
 G6nSxQNo8iIIxaEDYwv7A3stexIvUwdjOlBBgL4GFufMP6pu08JZAD1Iw6/n
X-Gm-Gg: ASbGncsrLItsEJLOAO0I/wri10nBe4YE7G/U/OvFkOeUhaLm8RdgfZ44fl81sn458hi
 x5ioKk/FAE6wFJlhWICim4rZwG2ANX6C0azNt1hsLuR4EjYElL2b3/0km0FTf40lxl4M/zveqpW
 Z2kRrU2Er8jTppb5xCpGd1ReJEQQbmMesRwJJ0Xr8IdLo4e7208H3cGoX+iv6H0rSylt52di9yM
 T/VaqZF479A+Wlfvrkfq40HSayUdqoPlojafp0yhaFNlxISRcAgAp1CYD0lXCBzLD0j5vP9VHSk
 k7cDkMpudkhvuCOmU9lBStgOFVxl7jxjEvgBn2s/cq8=
X-Google-Smtp-Source: AGHT+IEluEyM7anY3GVyApw4TWthK0GZdTwjg5mxvQei9SATQIo1LeqzkHcWGzbWl2lsq3lOuoZu2Q==
X-Received: by 2002:a17:903:1c4:b0:220:c066:94eb with SMTP id
 d9443c01a7336-2296c688bdcmr74208475ad.25.1743686748550; 
 Thu, 03 Apr 2025 06:25:48 -0700 (PDT)
Received: from terra ([2405:6586:be0:0:83c8:d31d:2cec:f542])
 by smtp.gmail.com with ESMTPSA id
 d9443c01a7336-2297865dfefsm13832295ad.151.2025.04.03.06.25.46
 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
 Thu, 03 Apr 2025 06:25:47 -0700 (PDT)
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: Tomas Volf <~@wolfsden.cz>
Subject: Re: [bug#74801] [PATCH v2] gnu: home: services: Add
 home-mpv-service-type.
In-Reply-To: <50f9a741440bc9b89ecdabcd7afb3a324173f7f9.1743602733.git.~@wolfsden.cz>
 (Tomas Volf's message of "Wed, 2 Apr 2025 16:05:33 +0200")
References: <29d3cc661a49b7bef951041f8ec3c7600c9777a7.1733953822.git.~@wolfsden.cz>
 <50f9a741440bc9b89ecdabcd7afb3a324173f7f9.1743602733.git.~@wolfsden.cz>
Date: Thu, 03 Apr 2025 22:25:30 +0900
Message-ID: <87y0whxtpx.fsf@gmail.com>
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: Ludovic Courtès <ludo@gnu.org>,
 Janneke Nieuwenhuizen <janneke@gnu.org>,
 Tanguy Le Carrour <tanguy@bioneland.org>, 74801@debbugs.gnu.org,
 Andrew Tropin <andrew@trop.in>
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,

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.


[...]

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

Send a report that this bug log contains spam.