| Message ID | 20220614033223.377744-1-bauermann@kolabnow.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#55954] gnu: public-inbox: Fixes to allow the testsuite to run | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi, Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > This patch makes the public-inbox testsuite pass. Some tests are skipped, > so the test coverage could likely be increased with more massaging. > > Perhaps the most significant change is using tini to run the testsuite so > that the testsuite's sub-processes are reaped. The ‘check’ phase is based on > the one from the mutter package. Thanks to Maxim Cournoyer for pointing out > this solution. > > * gnu/packages/patches/public-inbox-fix-spawn-test.patch: New file. > * gnu/local.mk (dist_patch_DATA): Add new patch. > * gnu/packages/mail.scm (public-inbox)[source]: Add new patch. > [arguments]<#:tests?>: Remove argument. > <#:imported-modules>: Add argument. > <#:modules>: Likewise. > <#:phases>{qualify-paths}: Substitute path for ‘/bin/cp’. > {pre-check}: Don't skip httpd-unix.t test. Remove unnecessary path > substitutions for “env” and “/bin/sh”. > {check}: Replace with custom version that launches the tests under tini. > [native-inputs]: Add tini. Applied, thanks! [...] > + (replace 'check > + (lambda* (#:key target > + (tests? (not target)) (test-flags '()) > + #:allow-other-keys) > + (if tests? > + (match (primitive-fork) > + (0 ;child process > + ;; lei tests build UNIX domain sockets in the temporary > + ;; directory, but the path of those sockets can be at most > + ;; 108 chars and Guix' default value for the variables > + ;; below already use 47 chars. Use the shortest temporary > + ;; path possible to avoid hitting the limit. > + (setenv "TEMP" "/tmp") > + (setenv "TEMPDIR" "/tmp") > + (setenv "TMP" "/tmp") > + (setenv "TMPDIR" "/tmp") > + > + ;; Use tini so that signals are properly handled and > + ;; doubly-forked processes get reaped; otherwise, > + ;; lei-daemon is kept as a zombie and the testsuite > + ;; fails thinking that it didn't quit as it should. > + (set-child-subreaper!) > + (apply execlp "tini" "--" > + "make" "check" test-flags)) > + (pid > + (match (waitpid pid) > + ((_ . status) > + (unless (zero? status) > + (error "`make check' exited with status" status)))))) It’s a bummer that we have to do all this. Would this work: (replace 'check (lambda _ (sigaction SIGINT SIG_DFL) (sigaction SIGTERM SIG_DFL) (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) ;; Ugly hack to make sure signal handler asyncs get a chance ;; to run while we’re in ‘waitpid’ during the “make check” ;; invocation. (sigaction SIGALRM (lambda _ (alarm 1))) (alarm 1))) ? If not, the solution you propose LGTM. Thanks, Ludo’.
Hello Ludo, Thank you for your review! Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > >> This patch makes the public-inbox testsuite pass. Some tests are skipped, >> so the test coverage could likely be increased with more massaging. >> >> Perhaps the most significant change is using tini to run the testsuite so >> that the testsuite's sub-processes are reaped. The ‘check’ phase is based on >> the one from the mutter package. Thanks to Maxim Cournoyer for pointing out >> this solution. >> >> * gnu/packages/patches/public-inbox-fix-spawn-test.patch: New file. >> * gnu/local.mk (dist_patch_DATA): Add new patch. >> * gnu/packages/mail.scm (public-inbox)[source]: Add new patch. >> [arguments]<#:tests?>: Remove argument. >> <#:imported-modules>: Add argument. >> <#:modules>: Likewise. >> <#:phases>{qualify-paths}: Substitute path for ‘/bin/cp’. >> {pre-check}: Don't skip httpd-unix.t test. Remove unnecessary path >> substitutions for “env” and “/bin/sh”. >> {check}: Replace with custom version that launches the tests under tini. >> [native-inputs]: Add tini. > > Applied, thanks! I guess you had second thoughts. :-) >> + (replace 'check >> + (lambda* (#:key target >> + (tests? (not target)) (test-flags '()) >> + #:allow-other-keys) >> + (if tests? >> + (match (primitive-fork) >> + (0 ;child process >> + ;; lei tests build UNIX domain sockets in the temporary >> + ;; directory, but the path of those sockets can be at most >> + ;; 108 chars and Guix' default value for the variables >> + ;; below already use 47 chars. Use the shortest temporary >> + ;; path possible to avoid hitting the limit. >> + (setenv "TEMP" "/tmp") >> + (setenv "TEMPDIR" "/tmp") >> + (setenv "TMP" "/tmp") >> + (setenv "TMPDIR" "/tmp") >> + >> + ;; Use tini so that signals are properly handled and >> + ;; doubly-forked processes get reaped; otherwise, >> + ;; lei-daemon is kept as a zombie and the testsuite >> + ;; fails thinking that it didn't quit as it should. >> + (set-child-subreaper!) >> + (apply execlp "tini" "--" >> + "make" "check" test-flags)) >> + (pid >> + (match (waitpid pid) >> + ((_ . status) >> + (unless (zero? status) >> + (error "`make check' exited with status" status)))))) > > It’s a bummer that we have to do all this. Would this work: > > (replace 'check > (lambda _ > (sigaction SIGINT SIG_DFL) > (sigaction SIGTERM SIG_DFL) > (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) > > ;; Ugly hack to make sure signal handler asyncs get a chance > ;; to run while we’re in ‘waitpid’ during the “make check” > ;; invocation. > (sigaction SIGALRM (lambda _ (alarm 1))) > (alarm 1))) > > ? I assume there should be a "make check" in there somewhere. :-) Unfortunately, it didn't work. This is the check phase I tried: --8<---------------cut here---------------start------------->8--- (replace 'check (lambda _ (setenv "TEMP" "/tmp") (setenv "TEMPDIR" "/tmp") (setenv "TMP" "/tmp") (setenv "TMPDIR" "/tmp") (sigaction SIGINT SIG_DFL) (sigaction SIGTERM SIG_DFL) (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) ;; Ugly hack to make sure signal handler asyncs get a chance ;; to run while we’re in ‘waitpid’ during the “make check” ;; invocation. (sigaction SIGALRM (lambda _ (alarm 1))) (alarm 1) (unless (zero? (status:exit-val (system* "make" "check"))) (error "`make check' exited with error")))) --8<---------------cut here---------------end--------------->8--- And this was the end of the output of the phase: --8<---------------cut here---------------start------------->8--- ⋮ Bailout called. Further testing stopped: daemon still running (PID:5282) FAILED--Further testing stopped: daemon still running (PID:5282) make: *** [Makefile:4141: check-each] Error 255 error: in phase 'check': uncaught exception: misc-error #f "~A" ("`make check' exited with error") #f phase `check' failed after 27.7 seconds --8<---------------cut here---------------end--------------->8--- “daemon still running” means that lei's daemon process wasn't reaped. > If not, the solution you propose LGTM. Thanks. Hopefully this can be fixed in Guix's build machinery itself in the core-updates branch and all the packages that are using this hack can drop it then.
Hello, Thiago Jung Bauermann <bauermann@kolabnow.com> skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >> Applied, thanks! > > I guess you had second thoughts. :-) Yes, sorry; I wasn’t sure whether bothering you was a good idea, but I was also dissatisfied with that whole situation (which is not public-inbox’s fault!). >> It’s a bummer that we have to do all this. Would this work: >> >> (replace 'check >> (lambda _ >> (sigaction SIGINT SIG_DFL) >> (sigaction SIGTERM SIG_DFL) >> (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) >> >> ;; Ugly hack to make sure signal handler asyncs get a chance >> ;; to run while we’re in ‘waitpid’ during the “make check” >> ;; invocation. >> (sigaction SIGALRM (lambda _ (alarm 1))) >> (alarm 1))) >> >> ? > > I assume there should be a "make check" in there somewhere. :-) > > Unfortunately, it didn't work. This is the check phase I tried: > > (replace 'check > (lambda _ > (setenv "TEMP" "/tmp") > (setenv "TEMPDIR" "/tmp") > (setenv "TMP" "/tmp") > (setenv "TMPDIR" "/tmp") > > (sigaction SIGINT SIG_DFL) > (sigaction SIGTERM SIG_DFL) > (sigaction SIGCHLD (lambda _ (waitpid WAIT_ANY WNOHANG))) > > ;; Ugly hack to make sure signal handler asyncs get a chance > ;; to run while we’re in ‘waitpid’ during the “make check” > ;; invocation. > (sigaction SIGALRM (lambda _ (alarm 1))) > (alarm 1) > (unless (zero? (status:exit-val (system* "make" "check"))) > (error "`make check' exited with error")))) > > > And this was the end of the output of the phase: > > ⋮ > Bailout called. Further testing stopped: daemon still running (PID:5282) > FAILED--Further testing stopped: daemon still running (PID:5282) > make: *** [Makefile:4141: check-each] Error 255 > error: in phase 'check': uncaught exception: > misc-error #f "~A" ("`make check' exited with error") #f > phase `check' failed after 27.7 seconds > > “daemon still running” means that lei's daemon process wasn't reaped. Damnit, I wonder how that can happen. >> If not, the solution you propose LGTM. > > Thanks. Hopefully this can be fixed in Guix's build machinery itself in > the core-updates branch and all the packages that are using this hack > can drop it then. Yeah. Anyway, applied for good this time, thanks for taking extra time on this! Ludo’.
diff --git a/gnu/local.mk b/gnu/local.mk index 03e180cc8508..0653cbb3bc16 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -1650,6 +1650,7 @@ dist_patch_DATA = \ %D%/packages/patches/psm-disable-memory-stats.patch \ %D%/packages/patches/psm-ldflags.patch \ %D%/packages/patches/psm-repro.patch \ + %D%/packages/patches/public-inbox-fix-spawn-test.patch \ %D%/packages/patches/pulseaudio-fix-mult-test.patch \ %D%/packages/patches/pulseaudio-longer-test-timeout.patch \ %D%/packages/patches/pulseview-qt515-compat.patch \ diff --git a/gnu/packages/mail.scm b/gnu/packages/mail.scm index 8596d0808beb..6f44c6c0f56a 100644 --- a/gnu/packages/mail.scm +++ b/gnu/packages/mail.scm @@ -87,6 +87,7 @@ (define-module (gnu packages mail) #:use-module (gnu packages django) #:use-module (gnu packages dns) #:use-module (gnu packages docbook) + #:use-module (gnu packages docker) #:use-module (gnu packages documentation) #:use-module (gnu packages emacs) #:use-module (gnu packages enchant) @@ -4084,10 +4085,16 @@ (define-public public-inbox (sha256 (base32 "0xni1l54v1z3p0zb52807maay0yqabp8jgf5iras5zmhgjyk3swz")) - (file-name (git-file-name name version)))) + (file-name (git-file-name name version)) + (patches (search-patches "public-inbox-fix-spawn-test.patch")))) (build-system perl-build-system) (arguments - '(#:tests? #f + `(#:imported-modules (,@%perl-build-system-modules + (guix build syscalls)) + #:modules ((guix build perl-build-system) + (guix build syscalls) + (guix build utils) + (ice-9 match)) #:phases (modify-phases %standard-phases (add-before 'configure 'qualify-paths @@ -4096,18 +4103,45 @@ (define-public public-inbox (substitute* "lib/PublicInbox/Xapcmd.pm" (("'xapian-compact'") (format #f "'~a'" (search-input-file inputs - "/bin/xapian-compact")))))) + "/bin/xapian-compact")))) + (substitute* "lib/PublicInbox/TestCommon.pm" + ;; This is only used for tests, but get it from ‘inputs’ so + ;; that cross builds won't hold a reference to a package built + ;; for another architecture. + (("/bin/cp") (search-input-file inputs "/bin/cp"))))) (add-before 'check 'pre-check (lambda _ - (substitute* "t/spawn.t" - (("\\['env'\\]") (string-append "['" (which "env") "']"))) - (substitute* "t/ds-leak.t" - (("/bin/sh") (which "sh"))) - (invoke "./certs/create-certs.perl") - ;; XXX: This test fails due to zombie process is not reaped by - ;; the builder. - (substitute* "t/httpd-unix.t" - (("^SKIP: \\{") "SKIP: { skip('Guix');")))) + (invoke "./certs/create-certs.perl"))) + (replace 'check + (lambda* (#:key target + (tests? (not target)) (test-flags '()) + #:allow-other-keys) + (if tests? + (match (primitive-fork) + (0 ;child process + ;; lei tests build UNIX domain sockets in the temporary + ;; directory, but the path of those sockets can be at most + ;; 108 chars and Guix' default value for the variables + ;; below already use 47 chars. Use the shortest temporary + ;; path possible to avoid hitting the limit. + (setenv "TEMP" "/tmp") + (setenv "TEMPDIR" "/tmp") + (setenv "TMP" "/tmp") + (setenv "TMPDIR" "/tmp") + + ;; Use tini so that signals are properly handled and + ;; doubly-forked processes get reaped; otherwise, + ;; lei-daemon is kept as a zombie and the testsuite + ;; fails thinking that it didn't quit as it should. + (set-child-subreaper!) + (apply execlp "tini" "--" + "make" "check" test-flags)) + (pid + (match (waitpid pid) + ((_ . status) + (unless (zero? status) + (error "`make check' exited with status" status)))))) + (format #t "test suite not run~%")))) (add-after 'install 'wrap-programs (lambda* (#:key inputs outputs #:allow-other-keys) (let ((out (assoc-ref outputs "out"))) @@ -4126,7 +4160,7 @@ (define-public public-inbox (find-files (string-append out "/bin"))))))))) (native-inputs (list ;; For testing. - lsof openssl)) + lsof openssl tini)) (inputs (list bash-minimal curl diff --git a/gnu/packages/patches/public-inbox-fix-spawn-test.patch b/gnu/packages/patches/public-inbox-fix-spawn-test.patch new file mode 100644 index 000000000000..2739b1974de8 --- /dev/null +++ b/gnu/packages/patches/public-inbox-fix-spawn-test.patch @@ -0,0 +1,43 @@ +From 5593489d9c3ce22b1942f35c7ebb0e06fcf2bfa8 Mon Sep 17 00:00:00 2001 +From: Thiago Jung Bauermann <bauermann@kolabnow.com> +Date: Fri, 10 Jun 2022 12:39:18 -0300 +Subject: [PATCH] t/spawn: Find invalid PID to try to join its process group + +In the container used to build packages of the GNU Guix distribution, PID 1 +runs as the same user as the test so this spawn that should fail actually +succeeds. + +Fix the problem by going through different PIDs and picking one that +either doesn't exist or we aren't allowed to signal. +--- + +This patch is taken from the public-inbox repository and will appear in the +release after v1.8. + + t/spawn.t | 13 ++++++++++++- + 1 file changed, 12 insertions(+), 1 deletion(-) + +diff --git a/t/spawn.t b/t/spawn.t +index 6168c1f6171c..5fc99a2a101c 100644 +--- a/t/spawn.t ++++ b/t/spawn.t +@@ -24,7 +24,18 @@ SKIP: { + is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process'); + is($?, 0, 'true exited successfully'); + pipe(my ($r, $w)) or BAIL_OUT; +- $pid = eval { spawn(['true'], undef, { pgid => 1, 2 => $w }) }; ++ ++ # Find invalid PID to try to join its process group. ++ my $wrong_pgid = 1; ++ for (my $i=0x7fffffff; $i >= 2; $i--) { ++ if (kill(0, $i) == 0) { ++ $wrong_pgid = $i; ++ last; ++ } ++ } ++ ++ # Test spawn behavior when it can't join the requested process group. ++ $pid = eval { spawn(['true'], undef, { pgid => $wrong_pgid, 2 => $w }) }; + close $w; + my $err = do { local $/; <$r> }; + # diag "$err ($@)";
This patch makes the public-inbox testsuite pass. Some tests are skipped, so the test coverage could likely be increased with more massaging. Perhaps the most significant change is using tini to run the testsuite so that the testsuite's sub-processes are reaped. The ‘check’ phase is based on the one from the mutter package. Thanks to Maxim Cournoyer for pointing out this solution. * gnu/packages/patches/public-inbox-fix-spawn-test.patch: New file. * gnu/local.mk (dist_patch_DATA): Add new patch. * gnu/packages/mail.scm (public-inbox)[source]: Add new patch. [arguments]<#:tests?>: Remove argument. <#:imported-modules>: Add argument. <#:modules>: Likewise. <#:phases>{qualify-paths}: Substitute path for ‘/bin/cp’. {pre-check}: Don't skip httpd-unix.t test. Remove unnecessary path substitutions for “env” and “/bin/sh”. {check}: Replace with custom version that launches the tests under tini. [native-inputs]: Add tini. --- gnu/local.mk | 1 + gnu/packages/mail.scm | 60 +++++++++++++++---- .../patches/public-inbox-fix-spawn-test.patch | 43 +++++++++++++ 3 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 gnu/packages/patches/public-inbox-fix-spawn-test.patch base-commit: 965d54e344dcd19adf3c32f8b9d2dcab62b91e6a