Backward incompatible changes in mpd-service-type

Hi!

I wanted to note some findings regarding the improved

mpd-configuration/mdp-service-type (thanks!)

(5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1, "services: mpd: Refactor MPD

service.") after having reconfigured my machine yesterday. I've noted

these two backward incompatible changes:

1. the mixer type must now be a string instead of a symbol, and takes

"none" instead of 'null for the null mixer:

It'd be nice if there was some deprecation warning and automatic healing

code for now. The doc should also be updated, because it still mention

the @code{null} value:

2. The MPD user appears to be created instead of using an existing

one. I was using my own account, like this:

and 'guix system reconfigure' is now warning that a user is defined

twice, and my user description following reconfiguration is: "Music

Player Daemon (MPD) user" :-).

Perhaps it should check if a user already exists and not add it if it

does? Else an error rather than a warning when multiple same-name users

are defined would be more appropriate, I think.

--

Thanks,

Maxim

Hi Maxim,

On 2023-02-17 12:53, Maxim Cournoyer wrote:

Indeed, this was an unintentional API breakage. IMO taking symbols here

always stood out since most of the fields were happy taking strings as values.

A deprecated warning and a symbol->string can be added here for compatibility.

There's no mistake here, 'null' mixer is distinct from 'none' mixer.

This is an unfortunate situation arising from a bug before the service was refactored.

Before d7fd9ec209f72e9cfff04a48bf16e092f258d8ff (actually 5c5f0fc1135ff15f9c4adfc5f27eadd9a592b5d1)

mpd-service-type contained a service-extension for %mpd-accounts where the values for both group and user were hardcoded to "mpd"

but this was actually never used since shepherd would launch the service using root and mpd would downgrade its permissions and switch

to the user specified in the mpd-configuration record since this field is serialized to the configuration file.

No "warning for user defined twice" would show precisely because the service-extension %mpd-accounts for account-service-type was

hardcoded, unless your "user-account" happened to be named "mpd".

%mpd-accounts being hardcoded also meant that the mpd-service-activation procedure made little sense. (since it was

chown'ing and chmod'ing directories from the "mpd" user to the value in the 'user' field from mpd-configuration record type)

AFAIK, the 'users' (resp. 'groups') fields from the 'operating-system' record type do not have precedence over account-service-type service extensions

(though this should be the case) so it's "whoever remains" after duplicates are filtered out, which in this case mpd-service-type happened to "win the

race".

I'd expect this behavior to happen with any service whose configuration record-type contains a user (resp. group) field that is relayed to a

account-service-type service-extension. (examples would be some of the git services if I'm not mistaken)

Another way to trigger this is with 'radicale', by manually creating the 'radicale' user in 'operating-system' (say that you want to change the

HOME directory since its where the data is saved to) and adding 'radicale-service-type' to 'services'.

I don't think service definitions have access to the operating-system field,

this was one of the hurdles when #60735 was being tackled.

IMO, this "issue" is avoided by not using the mpd-service-type as a "home service", which is what you're trying to do

when you pass it your own "interactive" user account.

Forcing mpd-service-type as a home service is fraught with subtle peculiarities, one of them was noted in commit message

637a9c3b264fe8eb76b6ed9f104b635d95632be6 and was discussed in #59866.

This service should be used "system-wide", which could be streaming oriented setups (via HTTP, icecast, PulseAudio + rtp, ...) or

system-wide PulseAudio. (not recommended)

"Home service" use cases should get its own home service definition, either by reusing many of the record-types and procedures here

or simply by inheriting and selectively deleting some of the service extensions. (such as account-service-type)

Cheers,

Bruno

Hi Bruno and Maxim,

Am Freitag, dem 17.02.2023 um 15:33 +0000 schrieb Bruno Victal:

It would be quite weird if someone had already pointed out how to

properly handle the accounts and groups only for that to be ignored

later in the review.

Am Samstag, dem 24.12.2022 um 18:20 +0100 schrieb eine leichtsinnige

Person, die ihre eigenen Anmerkungen vergisst:

Never mind then.

Am Freitag, dem 17.02.2023 um 07:53 -0500 schrieb Maxim Cournoyer:

Guess what, it used to be a formatted message (i.e. an actual error).

However, that broke some configs as reported in [1], so I demoted it to

a warning.

Cheers

[1] http://git.savannah.gnu.org/cgit/guix.git/commit/?id=8488f45b6e05d646224cc2b410497ddf9864c612

(mpd-serialize-user-account, mpd-serialize-user-group): New variables.

(mpd-configuration)[user]: Change type to user-account.

Change default accordingly.

[group]: Change type to user-group. Change default accordingly.

(mpd-shepherd-service, mpd-accounts): Adjust accordingly.

---

This patch fixes the issue of not being able to insert actual users and

groups into MPD service. Sadly, as define-configuration lacks proper

support for sanitizers, it's a backwards-incompatible change.

Perhaps it makes sense to extend define-configuration to support this

case?

Cheers

doc/guix.texi | 4 ++--

gnu/services/audio.scm | 41 +++++++++++++++++++++++++++--------------

2 files changed, 29 insertions(+), 16 deletions(-)

Hi Maxim, Liliana

On 2023-02-18 17:42, Liliana Marie Prikler wrote:> This patch fixes the issue of not being able to insert actual users and

I'd like to ask to hold merging this patch yet. I've got a few additional patches that addresses

some usability issues and IMO we can get a nicer patch by fixing define-configuration directly (whilst preserving API compatibility).

My intention is to patch define-configuration to accept a custom-sanitizer if specified.

Cheers,

Bruno

Hi,

Bruno Victal <mirai@makinata.eu> writes:

Sounds good! Thanks for working toward that.

--

Thanks,

Maxim

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:

[...]

I think Bruno has been reworking that, I think they must be about ready.

Interesting. I didn't know we were usefully (?) abusing duplicate users

and group. Perhaps we should try to isolate the most common offenders

(services?), fix them up, and then re-introduce the check, perhaps

gradually (e.g. "in 6 months time, duplicated users or groups will

become a configuration error").

--

Thanks,

Maxim

Am Montag, dem 06.03.2023 um 20:13 -0500 schrieb Maxim Cournoyer:

As far as I'm aware, we aren't. Even if such uses exist, they raise

said warning and probably cause more issues down the line, like with

your bug report.

The only instance known to me (cups creating a duplicate lp group) was

fixed back in 2021.

Cheers

Done with 7fdadeac11a997583305cb867b4a8828808ae953.