bug#72547: [PATCH 0/9] Make test suite pass on macOS.

bug#72547: [PATCH 0/9] Make test suite pass on macOS.

[Tomas Volf]

In my new work I sadly got a MacBook Pro.  I obviously want to use Guile
on it, however while trying to package it, I discovered that the test
suite is not passing.  This series remedies that.  Most of the fixes are
just in tests, but two are actual bug fixes.

With this series applied, 3.0.10 test suite does fully pass (well, minus
the skips ^_^) on 14.6 macOS.

I am not sure if the project cares about non-free system, but since I
had to write the patches anyway, I might as well share them.

Tomas Volf (9):
  tests: Check TCP_NODELAY for non-zero instead of 1.
  tests: Skip tests of abstract Unix sockets on Darwin.
  tests: Skip hole-related port tests on Darwin.
  filesys.c: Fix readlink for ports on Darwin.
  tests: Skip mkdtemp test for invalid template on Darwin.
  tests: Fix spawn with #:environment on MacOS.
  tests: Fix spawn if file not found with Gnulib.
  Do not depend on tmpnam in posix.test.
  posix.c: Set errno when pipe2 is not available and flags provided.

 configure.ac                    |  2 +-
 libguile/filesys.c              | 13 ++++++++++--
 libguile/posix.c                |  2 +-
 test-suite/tests/00-socket.test | 10 ++++++++-
 test-suite/tests/filesys.test   |  5 +++++
 test-suite/tests/ports.test     |  9 ++++++++
 test-suite/tests/posix.test     | 37 ++++++++++++++++++++++++++-------
 7 files changed, 65 insertions(+), 13 deletions(-)

--
2.45.2

bug#72547: [PATCH 1/9] tests: Check TCP_NODELAY for non-zero instead of 1.

[Tomas Volf]

POSIX does not explicitly say that stored value using setsockopt will be
returned by getsockopt.  At least for TCP_NODELAY on Darwin they do
differ.  Darwin returns internal define TF_NODELAY (4) instead of 1 the
test expected.  Since for boolean flags "non-zero is true", rewrite the
test to check just that.

* test-suite/tests/00-socket.test ("setsockopt AF_INET")
["IPPROTO_TCP TCP_NODELAY"]: Check for non-zero value from getsockopt.
---
 test-suite/tests/00-socket.test | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test-suite/tests/00-socket.test b/test-suite/tests/00-socket.test
index b3692f61a..17e686225 100644
--- a/test-suite/tests/00-socket.test
+++ b/test-suite/tests/00-socket.test
@@ -147,7 +147,7 @@
       (pass-if "IPPROTO_TCP TCP_NODELAY"
 	(let ((sock (socket AF_INET SOCK_STREAM 0)))
           (setsockopt sock IPPROTO_TCP TCP_NODELAY 1)
-          (eqv? 1 (getsockopt sock IPPROTO_TCP TCP_NODELAY))))))
+          (not (eqv? 0 (getsockopt sock IPPROTO_TCP TCP_NODELAY)))))))
 
 \f
 ;;;
-- 
2.45.2

bug#72547: [PATCH 2/9] tests: Skip tests of abstract Unix sockets on Darwin.

[Tomas Volf]

Darwin does not support abstract Unix sockets, so mark the tests as
skipped.

* test-suite/tests/00-socket.test (skip-on-darwin): New procedure.
("bind abstract", "listen abstract", "connect abstract")
("accept abstract"): Skip on Darwin.
---
 test-suite/tests/00-socket.test | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test-suite/tests/00-socket.test b/test-suite/tests/00-socket.test
index 17e686225..ba9bb279b 100644
--- a/test-suite/tests/00-socket.test
+++ b/test-suite/tests/00-socket.test
@@ -25,6 +25,10 @@
   #:use-module (srfi srfi-26)
   #:use-module (test-suite lib))
 
+(define (skip-on-darwin)
+  (when (string-ci=? "darwin" (vector-ref (uname) 0))
+    (throw 'untested)))
+
 \f
 
 ;;;
@@ -336,6 +340,7 @@
         (set! path (string-append "\x00" path))
 
 	(pass-if "bind abstract"
+          (skip-on-darwin)
 	  (catch 'system-error
 	    (lambda ()
 	      (bind server-socket AF_UNIX path)
@@ -348,6 +353,7 @@
 		    (apply throw args))))))
 
 	(pass-if "listen abstract"
+          (skip-on-darwin)
 	  (if (not server-bound?)
 	      (throw 'unresolved)
 	      (begin
@@ -375,6 +381,7 @@
 	       #t))))
 
 	(pass-if "connect abstract"
+          (skip-on-darwin)
 	  (if (not server-pid)
 	      (throw 'unresolved)
 	      (let ((s (socket AF_UNIX SOCK_STREAM 0)))
@@ -383,6 +390,7 @@
 		#t)))
 
 	(pass-if "accept abstract"
+          (skip-on-darwin)
 	  (if (not server-pid)
 	      (throw 'unresolved)
               (begin
-- 
2.45.2

bug#72547: [PATCH 3/9] tests: Skip hole-related port tests on Darwin.

[Tomas Volf]

Hole are itself a file-system specific feature and they are not
mandated.  While APFS does support sparse files, they do not behave like
on Linux.  I did not discover exact rules, but the file needs to be
large (100s of kB at least) and the holes are not aligned as the test
code expects.  So just disable them.

* test-suite/tests/ports.test (skip-on-darwin): New procedure.
("size of sparse file", "SEEK_DATA while on data")
("SEEK_DATA while in hole", "SEEK_HOLE while in hole"): Skip on Darwin.
---
 test-suite/tests/ports.test | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test-suite/tests/ports.test b/test-suite/tests/ports.test
index 7b5b535e7..3fde402b4 100644
--- a/test-suite/tests/ports.test
+++ b/test-suite/tests/ports.test
@@ -39,6 +39,10 @@
 (define (test-file)
   (data-file-name "ports-test.tmp"))
 
+(define (skip-on-darwin)
+  (when (string-ci=? "darwin" (vector-ref (uname) 0))
+    (throw 'untested)))
+
 \f
 ;;;; Some general utilities for testing ports.
 
@@ -185,6 +189,7 @@
     (close-port iport))
   (delete-file filename))
 
+;;; Note: Holes are weird on Darwin.
 (let* ((file (test-file))
        (port (open-output-file file)))
   (seek port 4096 SEEK_SET)
@@ -193,10 +198,12 @@
 
   (pass-if-equal "size of sparse file"
       4100
+    (skip-on-darwin)
     (stat:size (stat file)))
 
   (pass-if-equal "SEEK_DATA while on data"
       4096
+    (skip-on-darwin)
     (if (defined? 'SEEK_DATA)
         (call-with-input-file file
           (lambda (port)
@@ -209,6 +216,7 @@
 
   (pass-if-equal "SEEK_DATA while in hole"
       4096
+    (skip-on-darwin)
     (if (defined? 'SEEK_DATA)
         (call-with-input-file file
           (lambda (port)
@@ -221,6 +229,7 @@
 
   (pass-if-equal "SEEK_HOLE while in hole"
       10
+    (skip-on-darwin)
     (if (defined? 'SEEK_HOLE)
         (call-with-input-file file
           (lambda (port)
-- 
2.45.2

bug#72547: [PATCH 4/9] filesys.c: Fix readlink for ports on Darwin.

[Tomas Volf]

When passed a port, `readlink' relies on the Linux specific behavior of
empty c_path meaning "the fd itself".  That does not work on Darwin.
Since there is no branch that would yield both fd and c_path, fallback
to freadlink when __APPLE__ is defined.

* libguile/filesys.c (do_readlink): Call freadlink for !__APPLE__.
* configure.ac (AC_CHECK_FUNCS): Add freadlink.
---
 configure.ac       |  2 +-
 libguile/filesys.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 262b171f1..e6ed14220 100644
--- a/configure.ac
+++ b/configure.ac
@@ -532,7 +532,7 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid         \
   fstatat futimens openat						\
   sched_getaffinity sched_setaffinity sendfile pipe2			\
   posix_spawn_file_actions_addclosefrom_np                              \
-  clearenv])
+  clearenv freadlink])
 
 # The newlib C library uses _NL_ prefixed locale langinfo constants.
 AC_CHECK_DECLS([_NL_NUMERIC_GROUPING], [], [], [[#include <langinfo.h>]])
diff --git a/libguile/filesys.c b/libguile/filesys.c
index 8657fedc3..8585005a9 100644
--- a/libguile/filesys.c
+++ b/libguile/filesys.c
@@ -1192,14 +1192,23 @@ SCM_DEFINE (scm_symlinkat, "symlinkat", 3, 0, 0,
 #undef FUNC_NAME
 #endif /* HAVE_SYMLINKAT */
 
-/* Static helper function for choosing between readlink
+/* Static helper function for choosing between readlink, freadlink
    and readlinkat. */
 static int
 do_readlink (int fd, const char *c_path, char *buf, size_t size)
 {
-#ifdef HAVE_READLINKAT
+/* Darwin does not accept empty c_path. */
+#if HAVE_READLINKAT && !__APPLE__
   if (fd != -1)
     return readlinkat (fd, c_path, buf, size);
+#elif HAVE_FREADLINK
+  /* There is no branch in s_scm_readlink that would lead to having both fd and
+   * non-empty c_path.  Therefore if fd is set, we (on Darwin only) use
+   * freadlink and ignore the c_path.  On linux this case is already handled by
+   * readlinkat, but Darwin does not understand empty c_path to mean "the fd
+   * itself" the way Linux does. */
+  if (fd != -1)
+    return freadlink (fd, buf, size);
 #else
   (void) fd;
 #endif
-- 
2.45.2

bug#72547: [PATCH 5/9] tests: Skip mkdtemp test for invalid template on Darwin.

[Tomas Volf]

Darwin accepts any template, as demonstrated here:

    #include <stdio.h>
    #include <unistd.h>

    int
    main(void)
    {
    	char template[] = {'T', '-', 'A', 'A', 'A', 'A', 'A', 'A', '\0'};
    	char *res = mkdtemp(template);
    	puts(res ? res : "(null)");
    	perror("mkdtemp");
    }

Outputs:

    T-AAAAAA
    mkdtemp: Undefined error: 0

This does not match prescribed POSIX behavior, but it is what it is.

* test-suite/tests/filesys.test (skip-on-darwin): New procedure.
("mkdtemp")["invalid template"]: Skip on Darwin.
---
 test-suite/tests/filesys.test | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test-suite/tests/filesys.test b/test-suite/tests/filesys.test
index fa8d6f797..72d778145 100644
--- a/test-suite/tests/filesys.test
+++ b/test-suite/tests/filesys.test
@@ -34,6 +34,9 @@
 (define (test-directory2)
   (data-file-name "filesys-test-dir2.tmp"))
 
+(define (skip-on-darwin)
+  (when (string-ci=? "darwin" (vector-ref (uname) 0))
+    (throw 'untested)))
 
 ;;;
 ;;; copy-file
@@ -345,6 +348,8 @@
            (= (string-length name) 8)))))
 
   (pass-if-exception "invalid template" exception:system-error
+    ;; MacOS accepts any template and just does no replacements.
+    (skip-on-darwin)
     (if (not (defined? 'mkdtemp))
         (throw 'unresolved)
         (mkdtemp "T-AAAAAA")))
-- 
2.45.2

bug#72547: [PATCH 6/9] tests: Fix spawn with #:environment on MacOS.

[Tomas Volf]

MacOS adds __CF_USER_TEXT_ENCODING to every program, in similar way GNU
Hurd prepends LD_ORIGIN_PATH (based on the comment).  So extend the
logic to do similar stripping on MacOS.

* test-suite/tests/posix.test ("spawn")
["env with #:environment and #:output"]: Strip trailing
__CF_USER_TEXT_ENCODING environment variable when on Darwin.
---
 test-suite/tests/posix.test | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/test-suite/tests/posix.test b/test-suite/tests/posix.test
index 18dad8902..ac5a3b289 100644
--- a/test-suite/tests/posix.test
+++ b/test-suite/tests/posix.test
@@ -442,12 +442,23 @@
         (close-port (car input+output))
         (waitpid pid)
 
-        ;; On GNU/Hurd, the exec server prepends 'LD_ORIGIN_PATH' for
-        ;; every program: <https://bugs.gnu.org/62501>.  Strip it.
-        (if (and (string=? "GNU" (utsname:sysname (uname)))
-                 (string-prefix? "LD_ORIGIN_PATH=" str))
-            (string-drop str (+ 1 (string-index str #\newline)))
-            str))))
+        (let ((sysname (utsname:sysname (uname))))
+          (cond
+           ((string=? "GNU" sysname)
+            ;; On GNU/Hurd, the exec server prepends 'LD_ORIGIN_PATH' for
+            ;; every program: <https://bugs.gnu.org/62501>.  Strip it.
+            (if (string-prefix? "LD_ORIGIN_PATH=" str)
+                (string-drop str (+ 1 (string-index str #\newline)))
+                str))
+           ((string-ci=? "darwin" sysname)
+            ;; MacOS appends '__CF_USER_TEXT_ENCODING' for every program.  Strip
+            ;; it.
+            (let ((pos (string-contains str "__CF_USER_TEXT_ENCODING=")))
+              (if pos
+                  (string-drop-right str (- (string-length str) pos))
+                  str)))
+           (else
+            str))))))
 
   (pass-if-equal "ls /proc/self/fd"
       "0\n1\n2\n3\n"                     ;fourth FD is for /proc/self/fd
-- 
2.45.2

bug#72547: [PATCH 7/9] tests: Fix spawn if file not found with Gnulib.

[Tomas Volf]

On Darwin posix_spawnp is not considered secured and therefore we
fallback to Gnulib's version.  That one however does not return ENOENT
when the file does not exist, but PID of the child process.  This seems
to be allowed by the standard.

* test-suite/tests/posix.test (skip-on-darwin): New procedure.
("spawn")["file not file"]: Skip on Darwin.
---
 test-suite/tests/posix.test | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test-suite/tests/posix.test b/test-suite/tests/posix.test
index ac5a3b289..95692eeb1 100644
--- a/test-suite/tests/posix.test
+++ b/test-suite/tests/posix.test
@@ -22,6 +22,9 @@
   #:use-module (test-suite lib)
   #:use-module ((rnrs io ports) #:select (get-string-all)))
 
+(define (skip-on-darwin)
+  (when (string-ci=? "darwin" (vector-ref (uname) 0))
+    (throw 'untested)))
 
 ;; FIXME: The following exec tests are disabled since on an i386 debian with
 ;; glibc 2.3.2 they seem to interact badly with threads.test, the latter
@@ -475,6 +478,11 @@
 
   (pass-if-equal "file not found"
       ENOENT
+    ;; Actually "skip-if-posix_spawn-replaced" would be preferred, since the
+    ;; behavior below is not implemented in gnulib, but I do not know how to
+    ;; write such test.  However due to posix_spawnp being buggy on MacOS it is
+    ;; always replaced, so whatever.
+    (skip-on-darwin)
     (catch 'system-error
       (lambda ()
         (spawn "this-does-not-exist" '("nope")
-- 
2.45.2

bug#72547: [PATCH 8/9] Do not depend on tmpnam in posix.test.

[Tomas Volf]

`tmpnam' is a deprecated procedure that can be excluded during a
configure (`--disable-tmpnam').  There currently was a single test
relying on it, and therefore failing is such configuration.  This commit
switches to mkstemp instead.

* test-suite/tests/posix.test ("system*"): Use mkstemp instead of
tmpnam.
---
 test-suite/tests/posix.test | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/test-suite/tests/posix.test b/test-suite/tests/posix.test
index 95692eeb1..c4360b79e 100644
--- a/test-suite/tests/posix.test
+++ b/test-suite/tests/posix.test
@@ -365,7 +365,8 @@
 
   (pass-if-equal "https://bugs.gnu.org/52835"
       "bong\n"
-    (let ((file (tmpnam)))
+    (let* ((port (mkstemp "T-XXXXXX"))
+           (file (port-filename port)))
       ;; Redirect stdout and stderr to FILE.
       (define status
         (call-with-output-file file
@@ -376,8 +377,9 @@
                   (lambda ()
                     (system* "sh" "-c" "echo bong >&2"))))))))
 
+      (delete-file file)
       (and (zero? (status:exit-val status))
-           (call-with-input-file file get-string-all))))
+           (get-string-all port))))
 
   (pass-if-equal "https://bugs.gnu.org/63024"
       0
-- 
2.45.2

bug#72547: [PATCH 9/9] posix.c: Set errno when pipe2 is not available and flags provided.

[Tomas Volf]

If pipe2 is not available (e.g. on MacOS) and flags are set,
SCM_SYSERROR was correctly signaled, however errno was not set, so it
reported as:

    Undefined error: 0

That sucks both in tests (the test is not skipped) and in actual
usage (user has no idea what went wrong).

So set errno to ENOSYS as well.

* libguile/posix.c (scm_pipe2) [!HAVE_PIPE2] <c_flags>: Set errno to
ENOSYS.
---
 libguile/posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 9a873b5a1..986dcc7d0 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -282,7 +282,7 @@ SCM_DEFINE (scm_pipe2, "pipe", 0, 1, 0,
     /* 'pipe2' cannot be emulated on systems that lack it: calling
        'fnctl' afterwards to set the relevant flags is not equivalent
        because it's not atomic.  */
-    rv = ENOSYS;
+    rv = errno = ENOSYS;
 #endif
 
   if (rv)
-- 
2.45.2

bug#72547: [PATCH 0/9] Make test suite pass on macOS.

[Ludovic Courtès]

Hi Tomas,

Tomas Volf <~@wolfsden.cz> skribis:

> In my new work I sadly got a MacBook Pro.  I obviously want to use Guile
> on it, however while trying to package it, I discovered that the test
> suite is not passing.  This series remedies that.  Most of the fixes are
> just in tests, but two are actual bug fixes.
>
> With this series applied, 3.0.10 test suite does fully pass (well, minus
> the skips ^_^) on 14.6 macOS.
>
> I am not sure if the project cares about non-free system, but since I
> had to write the patches anyway, I might as well share them.

It does: we’ve always tried to ensure Guile is portable (developers
usually run GNU/Linux but we’re happy to take patches like these that
fix portability to other systems).

> Tomas Volf (9):
>   tests: Check TCP_NODELAY for non-zero instead of 1.
>   tests: Skip tests of abstract Unix sockets on Darwin.
>   tests: Skip hole-related port tests on Darwin.
>   filesys.c: Fix readlink for ports on Darwin.
>   tests: Skip mkdtemp test for invalid template on Darwin.
>   tests: Fix spawn with #:environment on MacOS.
>   tests: Fix spawn if file not found with Gnulib.
>   Do not depend on tmpnam in posix.test.
>   posix.c: Set errno when pipe2 is not available and flags provided.

Applied with minor edits (see other messages).

Thanks!

Ludo’.

bug#72547: [PATCH 3/9] tests: Skip hole-related port tests on Darwin.

[Ludovic Courtès]

Tomas Volf <~@wolfsden.cz> skribis:

> +(define (skip-on-darwin)
> +  (when (string-ci=? "darwin" (vector-ref (uname) 0))
> +    (throw 'untested)))

I changed (vector-ref (uname) 0) to (utsname:sysname (uname)) in this
and other patches.

(We might want to factorize this procedure.  :-))

Ludo’.

bug#72547: [PATCH 9/9] posix.c: Set errno when pipe2 is not available and flags provided.

[Ludovic Courtès]

Tomas Volf <~@wolfsden.cz> skribis:

> --- a/libguile/posix.c
> +++ b/libguile/posix.c
> @@ -282,7 +282,7 @@ SCM_DEFINE (scm_pipe2, "pipe", 0, 1, 0,
>      /* 'pipe2' cannot be emulated on systems that lack it: calling
>         'fnctl' afterwards to set the relevant flags is not equivalent
>         because it's not atomic.  */
> -    rv = ENOSYS;
> +    rv = errno = ENOSYS;

It doesn’t make much of a difference but I changed it to:

  rv = -1, errno = ENOSYS;

bug#72547: [PATCH 4/9] filesys.c: Fix readlink for ports on Darwin.

[Ludovic Courtès]

Tomas Volf <~@wolfsden.cz> skribis:

> +#elif HAVE_FREADLINK
> +  /* There is no branch in s_scm_readlink that would lead to having both fd and
> +   * non-empty c_path.  Therefore if fd is set, we (on Darwin only) use
> +   * freadlink and ignore the c_path.  On linux this case is already handled by
> +   * readlinkat, but Darwin does not understand empty c_path to mean "the fd
> +   * itself" the way Linux does. */

Removed the leading stars on continuation lines, for styling
consistency.

bug#72547: [PATCH 3/9] tests: Skip hole-related port tests on Darwin.

[Tomas Volf]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

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

> Tomas Volf <~@wolfsden.cz> skribis:
>
>> +(define (skip-on-darwin)
>> +  (when (string-ci=? "darwin" (vector-ref (uname) 0))
>> +    (throw 'untested)))
>
> I changed (vector-ref (uname) 0) to (utsname:sysname (uname)) in this
> and other patches.

I am of course fine with the change, but I am pretty sure I just
copy-pasted it from either test-suite/standalone/test-out-of-memory or
test-suite/standalone/test-stack-overflow.

>
> (We might want to factorize this procedure.  :-))

Will put that on the todo list.

Tomas

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 853 bytes --]