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

Previous Next

Full log

Message #29 received at 74801 <at> debbugs.gnu.org (full text, mbox):

From: Tomas Volf <~@wolfsden.cz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Tanguy Le Carrour <tanguy <at> bioneland.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,
 Gabriel Wicki <gabriel <at> erlikon.ch>, Andrew Tropin <andrew <at> trop.in>,
 Hilton Chain <hako <at> ultrarare.space>, 74801 <at> debbugs.gnu.org,
 Janneke Nieuwenhuizen <janneke <at> gnu.org>
Subject: Re: [bug#74801] [PATCH v2] gnu: home: services: Add
 home-mpv-service-type.
Date: Wed, 07 May 2025 00:03:34 +0200

Ludovic Courtès <ludo <at> gnu.org> 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.
>
> Is #74748 still a significant issue after
> 5b158ddca9425d79ea4ceb374003fe0f7e6bd336?
>
> (This v2 already takes quite a few seconds to build, so it might be a
> good starting point for profiling…)

I believe the workaround is still justified due to REPL part.  In #74748
you write:

> At the REPL you could type:
>
>   ,o optimization-level 1
>
> You could use ‘repl-option-set!’ from ~/.guile somehow I guess.

But that means that every user experimenting with the configuration in
their REPL needs to do the same.  What is more, they first need to find
out that they should do that.  That seems not great.

>> 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.
>
> Sounds reasonable.
>
>> Option to configure inputs (for mpv) will come later in a separate patch.
>
> OK.
>
> (I think this text is not meant for the commit log itself; you can
> instead write it after the --- line.)

Will remove.  I sometimes struggle with this a bit (since commit message
I can write right away, the text for "after ---" I need to
keep... somewhere, until the patch is ready).

>
>> +@deffn {Procedure} make-home-mpv-configuration
>> +Return a new instance of @code{home-mpv-configuration}.  Available
>> +keyword arguments are:
>> +
>> +@table @asis
>> +@item @code{inherit} (default: @code{#t})
>> +Inherit fields from an another instance.
>> +
>> +@item @code{global} (default: @code{(make-mpv-profile-configuration)})
>> +The global configuration preceding all profiles.
>> +
>> +@item @code{profiles} (default: @code{()})
>> +An alist containing configuration for any additional profiles to include
>> +in the configuration file.
>> +
>> +@lisp
>> +(make-home-mpv-configuration
>> + #:profiles `((fullscreen . ,(make-mpv-profile-configuration
>> +                              #:fullscreen #t))))
>> +@end lisp
>> +
>> +@item @code{extra-config} (default: @code{#f})
>> +Additional content to include in the configuration file.  It is placed
>> +at the end of the file.
>
> Two minor stylistic issues:
>
>   1. We almost always drop the ‘make-’ bit from constructors, so I
>      wonder whether we should do the same here (OTOH these procedures
>      are different from the usual record constructors, so…).

Yeah, I did it like this on purpose to have a backwards compatible way
to switch to normal records (named without the make-) later.

>
>   2. Please add question marks for boolean flags: #:inherit?,
>      #:fullscreen?, etc.

Ah, good catch, will special-case 'boolean type in the generator (though
#:inherit in particular is not a boolean flag).

>
>> +;;; Generated code - START.
>
> Could you include the code you used to generate the options?

I definitely *could* do that.  While the code is maybe a bit ugly, it is
not intended to be kept secret.  Couple of questions though.

1. Where should I put it?  Somewhere in etc directory?  Or next to the
   service, so e.g. gnu/home/services/mpv-refresh.sh?

2. It has external dependencies, some of which are packaged in Guix
   (emacs, emacs-paredit) and some of them are not (guile-wolfsden).  Is
   that a problem?  Do I need to rewrite it to use just packaged
   libraries and programs?

>
> Apart from that it LGTM.

Thanks for the review. :)  Once we clarify the fate of the code
generator, I will send v3.

Tomas

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.