[bug#66525,1/7] gnu: mutter: Remove dependency on (guix build syscalls).

* gnu/packages/gnome.scm (mutter)[arguments]: Remove #:imported-modules.
Remove (guix build syscalls) from #:modules.
Rewrite ‘check’ phase to reap processes from the build process.
[native-inputs]: Remove TINI.
---
 gnu/packages/gnome.scm | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Am Freitag, dem 13.10.2023 um 17:47 +0200 schrieb Ludovic Courtès:
> * gnu/packages/gnome.scm (mutter)[arguments]: Remove #:imported-
> modules.
> Remove (guix build syscalls) from #:modules.
> Rewrite ‘check’ phase to reap processes from the build process.
> [native-inputs]: Remove TINI.
> ---
LGTM, but where would this go?

Hi Ludovic,

On 2023-10-13 16:47, Ludovic Courtès wrote:
> [native-inputs]: Remove TINI.

[…]

>                  (match (primitive-fork)
>                    (0                    ;child process
> -                   (set-child-subreaper!)
>                     ;; Use tini so that signals are properly handled and
>                     ;; doubly-forked processes get reaped; otherwise,
>                     ;; python-dbusmock would waste time polling for the dbus
>                     ;; processes it spawns to be reaped, in vain.
> -                   (apply execlp "tini" "--"
> -                          "dbus-run-session" "--"
> +                   (apply execlp "dbus-run-session" "dbus-run-session"
>                            "xvfb-run" "-a" "-s" (getenv "XVFB_SERVER_ARGS")

Looks like this comment could be removed as well?

Hi Ludo,

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

> * gnu/packages/gnome.scm (mutter)[arguments]: Remove #:imported-modules.
> Remove (guix build syscalls) from #:modules.
> Rewrite ‘check’ phase to reap processes from the build process.
> [native-inputs]: Remove TINI.
> ---
>  gnu/packages/gnome.scm | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/gnu/packages/gnome.scm b/gnu/packages/gnome.scm
> index 908b5782b5..a4993b7aa9 100644
> --- a/gnu/packages/gnome.scm
> +++ b/gnu/packages/gnome.scm
> @@ -7831,10 +7831,7 @@ (define-public mutter
>      (build-system meson-build-system)
>      (arguments
>       (list
> -      #:imported-modules `(,@%meson-build-system-modules
> -                           (guix build syscalls))
>        #:modules '((guix build meson-build-system)
> -                  (guix build syscalls)
>                    (guix build utils)
>                    (ice-9 match))
>        #:glib-or-gtk? #t
> @@ -7928,22 +7925,23 @@ (define-public mutter
>                              "1"))
>                  (match (primitive-fork)
>                    (0                    ;child process
> -                   (set-child-subreaper!)
>                     ;; Use tini so that signals are properly handled and
>                     ;; doubly-forked processes get reaped; otherwise,
>                     ;; python-dbusmock would waste time polling for the dbus
>                     ;; processes it spawns to be reaped, in vain.

As Bruno mentioned, the comment above has gone stale.

> -                   (apply execlp "tini" "--"
> -                          "dbus-run-session" "--"
> +                   (apply execlp "dbus-run-session" "dbus-run-session"
>                            "xvfb-run" "-a" "-s" (getenv "XVFB_SERVER_ARGS")
>                            "meson" "test" "-t" "0" "--print-errorlogs"
>                            test-options))
> -                  (pid
> -                   (match (waitpid pid)
> -                     ((_ . status)
> -                      (unless (zero? status)
> -                        (error "`meson test' exited with status"
> -                               status))))))))))))
> +                  (dbus-pid
> +                   (let loop ()
> +                     (match (waitpid WAIT_ANY)
> +                       ((pid . status)
> +                        (if (= pid dbus-pid)
> +                            (unless (zero? status)
> +                              (error "`meson test' exited with status"
> +                                     status))
> +                            (loop)))))))))))))

Interesting simplification!  I obviously wasn't aware this could work
instead of the more intricate set-child-subreaper! + fake init (tini).
I guess it solves a very narrow subset signal handling behavior compared
to a real init, but that it is sufficient here.

LGTM with comments from Bruno taken into account.

I think it could go to core-updates since we're already prepping the
branch.  It may give some respite to the berlin aarch64 workers, which
have been working non-stop for days or weeks.

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

> Am Freitag, dem 13.10.2023 um 17:47 +0200 schrieb Ludovic Courtès:
>> * gnu/packages/gnome.scm (mutter)[arguments]: Remove #:imported-
>> modules.
>> Remove (guix build syscalls) from #:modules.
>> Rewrite ‘check’ phase to reap processes from the build process.
>> [native-inputs]: Remove TINI.
>> ---
> LGTM, but where would this go?

I think it could go to core-updates.

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
>
>> Am Freitag, dem 13.10.2023 um 17:47 +0200 schrieb Ludovic Courtès:
>>> * gnu/packages/gnome.scm (mutter)[arguments]: Remove #:imported-
>>> modules.
>>> Remove (guix build syscalls) from #:modules.
>>> Rewrite ‘check’ phase to reap processes from the build process.
>>> [native-inputs]: Remove TINI.
>>> ---
>> LGTM, but where would this go?
>
> I think it could go to core-updates.

I proposed a dedicated branch, in line with the branching policy that
was discussed back in February during and after the Guix Days:

  https://issues.guix.gnu.org/66525#0

Ludo’.

Bruno Victal <mirai@makinata.eu> skribis:

>>                  (match (primitive-fork)
>>                    (0                    ;child process
>> -                   (set-child-subreaper!)
>>                     ;; Use tini so that signals are properly handled and
>>                     ;; doubly-forked processes get reaped; otherwise,
>>                     ;; python-dbusmock would waste time polling for the dbus
>>                     ;; processes it spawns to be reaped, in vain.
>> -                   (apply execlp "tini" "--"
>> -                          "dbus-run-session" "--"
>> +                   (apply execlp "dbus-run-session" "dbus-run-session"
>>                            "xvfb-run" "-a" "-s" (getenv "XVFB_SERVER_ARGS")
>
> Looks like this comment could be removed as well?

Oops, noted (waiting for other comments before sending a new version).

Ludo’.

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> I think it could go to core-updates since we're already prepping the
> branch.  It may give some respite to the berlin aarch64 workers, which
> have been working non-stop for days or weeks.

Yeah, it’s a tempting option in terms of resource usage, but less in
terms of legibility of the whole process.  Dunno.

Ludo’.

Hi,

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

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> I think it could go to core-updates since we're already prepping the
>> branch.  It may give some respite to the berlin aarch64 workers, which
>> have been working non-stop for days or weeks.
>
> Yeah, it’s a tempting option in terms of resource usage, but less in
> terms of legibility of the whole process.  Dunno.

Yeah, for resource usage, bundling this to core-updates makes sense,
especially since it should only affect running the test suite of the
packages touched, not their output.

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> I think it could go to core-updates since we're already prepping the
>>> branch.  It may give some respite to the berlin aarch64 workers, which
>>> have been working non-stop for days or weeks.
>>
>> Yeah, it’s a tempting option in terms of resource usage, but less in
>> terms of legibility of the whole process.  Dunno.
>
> Yeah, for resource usage, bundling this to core-updates makes sense,
> especially since it should only affect running the test suite of the
> packages touched, not their output.

OTOH, my initial motivation was to apply patches to syscalls.scm that
have been queued for quite a while already:

  https://issues.guix.gnu.org/66055
  https://issues.guix.gnu.org/66054
  https://issues.guix.gnu.org/65546

I fear that bundling it with ‘core-updates’ would delay it by several
more months.

Resource usage is a concern due to the low AArch64 build power, but it’s
not too bad lately, even with ‘rust-team’ and ‘gnome-team’ updates:

  https://ci.guix.gnu.org/metrics

So overall, I think I have a preference for making a dedicated branch
and queueing a branch merge request.  (I think it’s also good to use
that process more widely.)

WDYT?

Ludo’.

Hi Ludo,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>>
>>>> I think it could go to core-updates since we're already prepping the
>>>> branch.  It may give some respite to the berlin aarch64 workers, which
>>>> have been working non-stop for days or weeks.
>>>
>>> Yeah, it’s a tempting option in terms of resource usage, but less in
>>> terms of legibility of the whole process.  Dunno.
>>
>> Yeah, for resource usage, bundling this to core-updates makes sense,
>> especially since it should only affect running the test suite of the
>> packages touched, not their output.
>
> OTOH, my initial motivation was to apply patches to syscalls.scm that
> have been queued for quite a while already:
>
>   https://issues.guix.gnu.org/66055
>   https://issues.guix.gnu.org/66054
>   https://issues.guix.gnu.org/65546
>
> I fear that bundling it with ‘core-updates’ would delay it by several
> more months.
>
> Resource usage is a concern due to the low AArch64 build power, but it’s
> not too bad lately, even with ‘rust-team’ and ‘gnome-team’ updates:
>
>   https://ci.guix.gnu.org/metrics
>
> So overall, I think I have a preference for making a dedicated branch
> and queueing a branch merge request.  (I think it’s also good to use
> that process more widely.)
>
> WDYT?

Go for it!  I'm confident core-updates won't take several months, but
who knows :-)

Hi Ludovic,

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

> Hello!
>
> Here’s an updated version of this patch set; changes compared to v1:
>
>   • Update outdated comment in one of the packages;
>
>   • Remove dependency on (guix build syscalls) of the following build
>     systems: ant, dub, android-ndk.
>
> I’ve pushed this as ‘wip-syscall-update’ and got ci.guix to build it:
>
>   https://ci.guix.gnu.org/jobset/syscall-update
>
> As discussed earlier, I’ll send a merge request.

OK.  It seems the branch was already mostly built (72% vs 73% for
master), so it should be good.

I'd still like to have something like bug#65595 implemented in Cuirass
so that it'd be easy to list all regressions that have to do with a
topic branch.  Currently failed builds are all grouped in the outputs,
whether it's for already or newly failing packages, which is less useful
in the context of feature branches.

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> I’ve pushed this as ‘wip-syscall-update’ and got ci.guix to build it:
>>
>>   https://ci.guix.gnu.org/jobset/syscall-update
>>
>> As discussed earlier, I’ll send a merge request.
>
> OK.  It seems the branch was already mostly built (72% vs 73% for
> master), so it should be good.

Yes.  (For the record, that’s almost 14K builds in 18 hours; the
x86_64/i686 builds were most likely completed in half of that time.)

Now merged as 48c1a74b2461d42dc0df202d8353640b3b64ac62!

> I'd still like to have something like bug#65595 implemented in Cuirass
> so that it'd be easy to list all regressions that have to do with a
> topic branch.  Currently failed builds are all grouped in the outputs,
> whether it's for already or newly failing packages, which is less useful
> in the context of feature branches.

It’s something where the Data Service really shines because it knows the
derivations of each package of each revision, and it can compare
substitute availability.

Perhaps what we need is closer integration and/or user interface
improvement.  For example, it’s not trivial from
<https://data.qa.guix.gnu.org/repository/1/branch/issue-66525> to get to
a comparison against the ‘master’ branch.

Ludo’.

Hey Ludo,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>>> I’ve pushed this as ‘wip-syscall-update’ and got ci.guix to build it:
>>>
>>>   https://ci.guix.gnu.org/jobset/syscall-update
>>>
>>> As discussed earlier, I’ll send a merge request.
>>
>> OK.  It seems the branch was already mostly built (72% vs 73% for
>> master), so it should be good.
>
> Yes.  (For the record, that’s almost 14K builds in 18 hours; the
> x86_64/i686 builds were most likely completed in half of that time.)
>
> Now merged as 48c1a74b2461d42dc0df202d8353640b3b64ac62!

I'm surprised even the ARM machines managed to build so many packages
that fast!  Great :-).

>> I'd still like to have something like bug#65595 implemented in Cuirass
>> so that it'd be easy to list all regressions that have to do with a
>> topic branch.  Currently failed builds are all grouped in the outputs,
>> whether it's for already or newly failing packages, which is less useful
>> in the context of feature branches.
>
> It’s something where the Data Service really shines because it knows the
> derivations of each package of each revision, and it can compare
> substitute availability.

There doesn't seem to be a technical limitation in allowing to filter on
'newly broken' packages; we can already see in the shared bag of 'failed
builds' that some are new failures (they have a down arrow icon), so
Cuirass already has that data; it's just not possibly to query it precisely,
unless I'm missing something.