`modify-services` no longer affects multiple instances of the same service

Hi Guix!

Recently there was a change to the behavior of `modify-services` that adds logic to check for any unused clauses so that an exception can be raised to alert the user of this case.

It seems that the new logic has a bug that prevents a used clause from being executed on more than one instance of a compatible service in a single execution of `modify-services`. Here's a new test case for `gnu/tests/services.scm` that exhibits the issue:

```

(test-equal "modify-services: delete multiple services of the same type"

'(1 3)

(let* ((t1 (service-type (name 't1)

(extensions '())

(description "")))

(t2 (service-type (name 't2)

(extensions '())

(description "")))

(t3 (service-type (name 't3)

(extensions '())

(description "")))

(services (list (service t1 1) (service t2 2)

(service t2 2) (service t3 3))))

(map service-value

(modify-services services

(delete t2)))))

```

Here's the output of the test:

```

test-name: modify-services: delete multiple services of the same type

location: /home/daviwil/Projects/Code/guix/tests/services.scm:325

source:

+ (test-equal

+ "modify-services: delete multiple services of the same type"

+ '(1 3)

+ (let* ((t1 (service-type

+ (name 't1)

+ (extensions '())

+ (description "")))

+ (t2 (service-type

+ (name 't2)

+ (extensions '())

+ (description "")))

+ (t3 (service-type

+ (name 't3)

+ (extensions '())

+ (description "")))

+ (services

+ (list (service t1 1)

+ (service t2 2)

+ (service t2 2)

+ (service t3 3))))

+ (map service-value

+ (modify-services services (delete t2)))))

expected-value: (1 3)

actual-value: (1 2 3)

result: FAIL

```

The problem occurs because of this `fold2` logic in `apply-clauses` of gnu/services.scm`:

```

(fold2 (lambda (clause service remainder)

(if service

(match clause

((kind proc properties)

(if (eq? kind (service-kind service))

(values (proc service) remainder)

(values service

(cons clause remainder)))))

(values #f (cons clause remainder))))

head

'()

clauses)))

```

In the #t case of checking the service kind, `(values (proc service remainder)` is returned, meaning the successful clause is not being added back to the list of clauses as `fold2` continues. Any subsequent items of the service list will no longer be tested against the removed clause.

I believe this function's logic needs to be updated to keep a list of successful clauses to be diffed against the full clause list at the end of `apply-clauses` so that the unapplied clause list can be determined without having to remove successful clauses in-flight.

If anyone has any pointers on the best way to approach this, I'll be happy to submit a patch!

David

merge 64106 63921

thankyou

Hi David,

"David Wilson" <david@daviwil.com> writes:

This was intended, but was probably not a good idea. I'll look into how

we could revert this while keeping the main idea of the change.

Best,

--

Josselin Poiret

severity 64106 important

quit

Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

It wasn’t really intended, more a side effect of the implementation, and

I agree it should be fixed.

Ludo’.

Ludovic Courtès <ludo@gnu.org> writes:

There have been a number of complaints about the behavior change, and I

think the patch should probably be reverted. My only intention was to

raise an error for the cases where a service was used in

‘modify-services’ that wasn't defined, as that's something people would

probably want to know about and fix, but the sequelae to that change

have changed the primary behavior of ‘modify-services’ in a disruptive

way (and without a corresponding news item).

I've taken a number of stabs at tweaking the current code to satisfy

both my desire to raise an error for mis-configurations as well as match

every instance of a service, but I can't find a way to do it that also

preserves service ordering.

However, if ‘modify-services’ can be changed to do two passes, the first

as a sanity check which verifies service references and raises errors,

and the second to do the actual modification, that should work well. I'm

not concerned with efficiency particularly. Unless there are many

thousands of services I sincerely doubt anyone would notice the extra

pass, even on a Raspberry Pi.

WDYT?

-bjc

With this commit, modify-services will delete all instances of the services

that match the indicated service-type. At the same time, it will also raise

errors for misconfigurations.

The patch was motivated by Brian Cully's statements about the difficult

tradeoff here. [1]

Using the changes respectfully submitted there, modify-services will extract

the "delete" clauses from the clause-alist via code inspired by SRFI-1's

lset-difference. It applies those deletions first, before then passing the

remaining clauses (and the remaining services) on to apply-clauses.

It is possible that apply-clauses can also be simplified since the "delete"

case is now gone, but I remain unsure about how to do that.

This commit was lightly tested on a production machine.

The author owes a deep debt of gratitude to Zipheir from the IRC channel

Libera:#scheme for their expert help in understanding the problem and in

coming up with an elegant solution.

Similarly, Bruno Victal (aka mirai) provided major encouragment with his

participation in the same conversation.

Thank you to both of you!

[1] https://issues.guix.gnu.org/64106#4

stanza. (Closes: #63921, #64106)

---

gnu/services.scm | 38 +++++++++++++++++++++++++++++++++++---

1 file changed, 35 insertions(+), 3 deletions(-)

Hi,

Brian Cully <bjc@spork.org> skribis:

Yeah I think we’ll have to do two passes, or to revert the whole thing.

The former sounds preferable for me.

Could you work on a patch?

Thanks in advance!

Ludo’.

This patch reverts the behavior introduced in

181951207339508789b28ba7cb914f983319920f which caused ‘modify-services’

clauses to only match a single instance of a service.

We will now match all service instances when doing a deletion or update, while

still raising an exception when trying to match against a service that does

not exist in the services list, or which was deleted explicitly by a ‘delete’

clause (or an update clause that returns ‘#f’ for the service).

Fixes: #64106

(modify-services): Use it.

(apply-clauses): Add DELETED-SERVICES argument, change to modify one service

at a time.

("modify-services: delete then modify"),

("modify-services: modify then delete"),

("modify-services: delete multiple services of the same type"),

("modify-services: modify multiple services of the same type"): New tests.

---

gnu/services.scm | 95 +++++++++++++++++++++++++++-------------------

tests/services.scm | 68 +++++++++++++++++++++++++++++++++

2 files changed, 124 insertions(+), 39 deletions(-)

I sent a patch to this ticket yesterday, before remembering this morning

that y'all probably weren't auto-Cc'd on it by debbugs.

Please have a look over it, especially the tests, in case I missed some

functionality or misinterpreted some requirements.

This will probably be deserving of a news item, since it will cause

multiple deletes on the same service type to fail, and that's what at

least some people are doing due to the previous patch.

One option would be to convert the ‘raise’ incantations to warnings, at

least for a while to give people a chance to update without their

configs breaking, but I don't know a good way to do that.

-bjc

severity 65184 important

merge 64106 65184

thanks

sigh

Hi Brian!

Brian Cully <bjc@spork.org> writes:

[...]

I've applied the following cosmetic changes:

and installed it. Thanks for contributing to Guix!

--

Thanks,

Maxim

Hi Maxim,

On Thu, Aug 31, 2023 at 8:49?PM Maxim Cournoyer

<maxim.cournoyer@gmail.com> wrote:

Thanks for taking action. Can Bug#63921 also be closed?

Kind regards

Felix