Re: test-framework-sh: Don't leave temporary directories on NetBSD.

[Collin Funk]

Hi Paul,

On 6/11/24 3:15 PM, Paul Eggert wrote:
> Yes, and Jim's point seems good so I tried to resurrect that by
> installing the attached patch to use GNU-style mktemp -t without
> creating junk directories on NetBSD. Please give it a try.

Works on my NetBSD 10.0 VM, thanks.

Also, I was able to confirm Bruno's analysis. My FreeBSD 14.0 VM had
many -p. directories under /tmp so the issue exists there to. Your
patch fixes the issue there too.

Collin

[PATCH] mktempd: Invoke mktemp portably.

[Collin Funk]

* build-aux/mktempd (mktempd): Don't use -t when invoking mktemp since
some implementations expect an argument while others do not.
---
 ChangeLog | 6 ++
 build-aux/mktempd | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index a8d28c4765..da884143dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-06-11  Collin Funk  
+
+   mktempd: Invoke mktemp portably.
+   * build-aux/mktempd (mktempd): Don't use -t when invoking mktemp since
+   some implementations expect an argument while others do not.
+
 2024-06-11  Bruno Haible  
 
test-framework-sh: Fix 'returns_' to not turn off tracing permanently.
diff --git a/build-aux/mktempd b/build-aux/mktempd
index 7f69a2d66b..542a4016fe 100755
--- a/build-aux/mktempd
+++ b/build-aux/mktempd
@@ -91,7 +91,7 @@ mktempd()
   fail=0
 
   # First, try to use mktemp.
-  d=`env -u TMPDIR mktemp -d -t -p "$destdir" "$template" 2>/dev/null` \
+  d=`env -u TMPDIR mktemp -d -p "$destdir" "$template" 2>/dev/null` \
 || fail=1
 
   # The resulting name must be in the specified directory.
-- 
2.45.2

Re: test-framework-sh: Don't leave temporary directories on NetBSD.

[Collin Funk]

On 6/8/24 4:44 AM, Bruno Haible wrote:
> The cause appears to be that for GNU mktemp, the -t option does not take an
> argument [1]; likewise for OpenBSD mktemp [2] and Solaris mktemp [3].
> 
> Whereas for FreeBSD and NetBSD mktemp, the -t option takes an argument [4][5].
> 
> Since POSIX does not specify mktemp(1), neither of the two is "right" or
> "wrong". We just have to live with both.

Makes sense.

The build-aux/mktempd script uses -t in this way too. Should it be
fixed there?

   build-aux/mktempd:94:  d=`env -u TMPDIR mktemp -d -t -p "$destdir" 
"$template" 2>/dev/null` \

Collin

Re: questions following upcoming POSIX issue 8 release

[Collin Funk]

Eric Blake  writes:

> wants to change requirements for isatty() and friends to be required
> to set errno on failure (right now, there is no obvious way to tell,
> using only the standard, if a failure even happened); but the Austin
> Group would like a wider sampling of what existing libc
> implementations do.  If you could run this program on various systems
> and report, that would be helpful:

Running through the different systems on the compile farm:

AIX 7.1.1 (cfarm111):
$ ./a.out 
25 Not a typewriter
25 Not a typewriter
22 Invalid argument

Oracle Solaris 11.3 (cfarm211):
$ ./a.out 
25 Inappropriate ioctl for device
25 Inappropriate ioctl for device
25 Inappropriate ioctl for device

OpenBSD 7.5 (cfarm220):
$ ./a.out  
22 Invalid argument
22 Invalid argument
22 Invalid argument

FreeBSD 14.0 (cfarm240):
$ ./a.out 
22 Invalid argument
22 Invalid argument
22 Invalid argument

Alpine 3.19 w/ Musl (cfarm27):
$ ./a.out 
25 Not a tty
25 Not a tty
0 No error information

MacOS 12.6 (cfarm104):
$ ./a.out 
25 Inappropriate ioctl for device
25 Inappropriate ioctl for device
25 Inappropriate ioctl for device

A NetBSD 10.0 virtual machine:
$ ./a.out 
25 Inappropriate ioctl for device
0 Undefined error: 0
25 Inappropriate ioctl for device

Feel free to send to the POSIX bug tracker. I don't have an account.

Collin

Re: findutils 4.9.0: localeconv test failed on RISCV

[Collin Funk]

Hi Yuqi,

Yuqi Guo  writes:

> Hello friends, do you have any idea about this?

Sorry, I forgot to respond. I'm moving this to bug-gnulib@gnu.org. I
don't know enough about docker but perhaps someone else can help.

The original report [1]:

> File:
>
>gnulib-tests/test-localeconv.c
>
> Code:
>
> ```
>
> struct lconv *l = localeconv ();
>
> ASSERT (l->frac_digits == CHAR_MAX);
> ASSERT (l->p_cs_precedes == CHAR_MAX);
> ASSERT (l->p_sign_posn == CHAR_MAX);
> ASSERT (l->p_sep_by_space == CHAR_MAX);
> ASSERT (l->n_cs_precedes == CHAR_MAX);
> ASSERT (l->n_sign_posn == CHAR_MAX);
> ASSERT (l->n_sep_by_space == CHAR_MAX);
>
> ```
>
> Problem:
>
> On RISCV, the `char` is unsigned. Thus, the `CHAR_MAX` is 255 instead of
> 127 on x86. However, the localeconv values remain the same on RISCV as
> x86 (the values are 127). This makes the> assertions fail on RISCV.

I asked what system and Yuqi says [2]:

> I use a RISCV QEMU emulator to compile and run the findutils package.
> The emulator was packed and published on Docker Hub by someone several
> months ago. You can get it by `docker pull
> xfan1024/openeuler:23.09-riscv64` and then start a docker container to
> easily use it. Note, you may need to have `qemu-riscv64-static`
> installed on your host system as this emulator is driven by it.
>
> Inside the emulator, I find that:
> (1) The system is openEuler-23.09
>
> (2) The libc is GNU libc (version 2.38), and the compiler is gcc (version 
> 12.3.1)

The correct behavior is these values should be CHAR_MAX as described by
POSIX [3]:

The members with type char are non-negative numbers, any of which can
be {CHAR_MAX} to indicate that the value is not available in the
current locale.

Collin

[1] https://lists.gnu.org/archive/html/bug-findutils/2024-06/msg00036.html
[2] https://lists.gnu.org/archive/html/bug-findutils/2024-06/msg00039.html
[3] https://pubs.opengroup.org/onlinepubs/009695399/functions/localeconv.html

gnulib-tool.py: Handle absolute path checks consistently.

[Collin Funk]

A few functions in constants.py check for drive prefixes where other
code does not. Lets just use os.path.isabs() and let it deal with
platform related stuff. I suppose path.startswith('/') would probably
work too but it is less clear and would make Windows (without Cygwin)
support harder in the future.

* pygnulib/GLImport.py (GLImport.relative_to_destdir)
(GLImport.relative_to_currdir): Use os.path.isabs() instead of checking
for a slash.
* pygnulib/constants.py (symlink_relative, as_link_value_at_dest)
(hardlink): Use os.path.isabs() instead of checking for a slash or drive
prefix.
---
 ChangeLog | 10 ++
 pygnulib/GLImport.py  |  8 
 pygnulib/constants.py | 11 +--
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8e67f0a816..3d34109399 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-06-09  Collin Funk  
+
+   gnulib-tool.py: Handle absolute path checks consistently.
+   * pygnulib/GLImport.py (GLImport.relative_to_destdir)
+   (GLImport.relative_to_currdir): Use os.path.isabs() instead of checking
+   for a slash.
+   * pygnulib/constants.py (symlink_relative, as_link_value_at_dest)
+   (hardlink): Use os.path.isabs() instead of checking for a slash or drive
+   prefix.
+
 2024-06-09  Bruno Haible  
 
c32width tests: Avoid a test failure on Solaris 11 OpenIndiana, OmniOS.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index df541a8b77..5f090d4ea1 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -418,10 +418,10 @@ def relative_to_destdir(self, dir: str) -> str:
 directory, to a filename relative to destdir.
 GLConfig: destdir.'''
 destdir = self.config['destdir']
-if dir.startswith('/'):
+if os.path.isabs(dir):
 return dir
 else:
-if destdir.startswith('/'):
+if os.path.isabs(destdir):
 # XXX This doesn't look right.
 return dir
 else:
@@ -433,10 +433,10 @@ def relative_to_currdir(self, dir: str) -> str:
 to a filename relative to the current directory.
 GLConfig: destdir.'''
 destdir = self.config['destdir']
-if dir.startswith('/'):
+if os.path.isabs(dir):
 return dir
 else:
-if destdir.startswith('/'):
+if os.path.isabs(destdir):
 # XXX This doesn't look right.
 return joinpath(destdir, dir)
 else:
diff --git a/pygnulib/constants.py b/pygnulib/constants.py
index 4890e2129f..71811576da 100644
--- a/pygnulib/constants.py
+++ b/pygnulib/constants.py
@@ -362,7 +362,7 @@ def symlink_relative(src: str, dest: str) -> None:
 os.symlink(src, dest)
 except PermissionError:
 sys.stderr.write('%s: ln -s failed; falling back on cp -p\n' % 
APP['name'])
-if src.startswith('/') or (len(src) >= 2 and src[1] == ':'):
+if os.path.isabs(src):
 # src is absolute.
 cp_src = src
 else:
@@ -384,12 +384,11 @@ def as_link_value_at_dest(src: str, dest: str) -> str:
 raise TypeError('src must be a string, not %s' % (type(src).__name__))
 if type(dest) is not str:
 raise TypeError('dest must be a string, not %s' % 
(type(dest).__name__))
-if src.startswith('/') or (len(src) >= 2 and src[1] == ':'):
+if os.path.isabs(src):
 return src
 else:  # if src is not absolute
-if dest.startswith('/') or (len(dest) >= 2 and dest[1] == ':'):
-cwd = os.getcwd()
-return joinpath(cwd, src)
+if os.path.isabs(dest):
+return joinpath(os.getcwd(), src)
 else:  # if dest is not absolute
 destdir = os.path.dirname(dest)
 if not destdir:
@@ -427,7 +426,7 @@ def hardlink(src: str, dest: str) -> None:
 os.link(src, dest)
 except PermissionError:
 sys.stderr.write('%s: ln failed; falling back on cp -p\n' % 
APP['name'])
-if src.startswith('/') or (len(src) >= 2 and src[1] == ':'):
+if os.path.isabs(src):
 # src is absolute.
 cp_src = src
 else:
-- 
2.45.2

Re: c32ispunct tests: Avoid a test failure on Solaris 11 OmniOS

[Collin Funk]

Bruno Haible  writes:

>> Would it make sense to use __illumos__ here?
>
> No, it is too new. [1]

Makes sense. Feel free to change the ones I added previously to __sun if
you would like.

Collin

Re: c32ispunct tests: Avoid a test failure on Solaris 11 OmniOS

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> -#if !(defined __FreeBSD__ || defined __DragonFly__)
> +#if !(defined __FreeBSD__ || defined __DragonFly__ || defined __sun)

Would it make sense to use __illumos__ here?
Not sure if you tested this on regular Oracle Solaris.

Collin

test-framework-sh: Don't leave temporary directories on NetBSD.

[Collin Funk]

Hi Paul, Jim,

A few years ago Paul removed '-t' from mktemp to accommodate NetBSD [1].
The change was then reverted since Jim preferred the directory name
created with -t. The issue arrives again because although it "works" it
silently litters /tmp with empty directories.

Here is an example on NetBSD 10.0:

 $ gnulib-tool --create-testdir --dir testdir1 localeconv
 $ cd testdir1
 $ ./configure
 $ make check
 $ ls /tmp
 gnulib-python-cache-collin  ssh-6fF5RmMxl5Rr  ssh-y2OL0VTqIZcc
 $ ls /tmp/
 -p.Aa9x3xH3  -p.QDECT3St  gnulib-python-cache-collin  ssh-6fF5RmMxl5Rr  
ssh-y2OL0VTqIZcc

Originally I thought that this was due to GNU getopt rearranging options
or something, since I don't think NetBSD does that. However, it looks
like NetBSD creates two directories and the first one is ignored by the
test framework. Here is before and after removing '-t' and using
"set -x" at the top of init.sh (NetBSD):

# Before:
unset TMPDIR
using redirections: 2>/dev/null do {
d='/tmp/-p.COeKGNtW
gt-test-init.sh.74sa'

# After:
unset TMPDIR
using redirections: 2>/dev/null do {
d=/home/collin/.local/src/gnulib/testdir1/gltests/gt-test-init.sh.5ii7

So the removal of '-t' fixes things on NetBSD. Here is the names created
before and after removing '-t' with Coreutils 9.5:

# Before:
d=/home/collin/.local/src/gnulib/testdir1/gltests/gt-test-init.sh.cx71

# After:
d=/home/collin/.local/src/gnulib/testdir1/gltests/gt-test-init.sh.ma3i

I don't see a difference, but I am not very familiar with mktemp so
perhaps I am missing something. I've gone ahead and committed this to
solve the issue but feel free to change it with this added information.

Collin

[1] https://lists.gnu.org/archive/html/bug-gnulib/2016-03/msg00071.html

>From c07e16b04eca6e678977bc4f7a5358bf98805431 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 8 Jun 2024 02:56:48 -0700
Subject: [PATCH] test-framework-sh: Don't leave temporary directories on
 NetBSD.

Reported by Taylor R Campbell  in
<https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58319>

* tests/init.sh (mktempd_): Don't use mktemp with the -t option as it
leads to uncleaned temporary directories on NetBSD.
---
 ChangeLog | 8 
 tests/init.sh | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index b9cda80d82..58fe6afdba 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-06-08  Collin Funk  
+
+	test-framework-sh: Don't leave temporary directories on NetBSD.
+	Reported by Taylor R Campbell  in
+	<https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58319>
+	* tests/init.sh (mktempd_): Don't use mktemp with the -t option as it
+	leads to uncleaned temporary directories on NetBSD.
+
 2024-06-07  Bruno Haible  
 
 	pthread_sigmask tests: Avoid failure due to known NetBSD 10.0 bug.
diff --git a/tests/init.sh b/tests/init.sh
index c5ec5cfd58..4689b6b758 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -344,7 +344,7 @@ mktempd_ ()
   esac
 
   # First, try to use mktemp.
-  d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` &&
+  d=`unset TMPDIR; { mktemp -d -p "$destdir_" "$template_"; } 2>/dev/null` &&
 
   # The resulting name must be in the specified directory.
   case $d in "$destdir_slash_"*) :;; *) false;; esac &&
-- 
2.45.2

Re: continuous integration for many platforms

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> Yes and no. This mailing list has quite a wide audience. I don't want to
> have lots of chatter about CI details on this list. Therefore, what belongs
> on this list is:
>   - Discussion whether a certain failure is a platform bug, a Gnulib code
> bug, a Gnulib test bug, or a CI bug — unless it's obvious.
>   - Major decisions, such as about adding another platform to the CI.
> But for the rest, such as messages like "I'm starting to investigate
> the NetBSD failure in the last run" or "do you know the package name
> of the locales on Alpine Linux", we should better use private email.
> If private email at some point does not work well, then we should better
> create a separate mailing list.

Sounds good to me, thanks. Now that I think about it 99% of changes will
probably be made while investigating some platform specific bug.
Therefore your explanation makes more sense.

Collin

Re: continuous integration for many platforms

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> The continuous integration of Gnulib for many platforms is now operational.
> 
>
> It tests a testdir of nearly all modules of Gnulib on the following platforms:

Thanks! It already seems helpful from the tests I have worked on so far.
Now we should be able to catch errors quickly as things update, etc.

> Platforms that are not covered and that therefore continue to need occasional
> manual testing:
>   - GNU/Hurd,
>   - AIX,
>   - Android,
>   - other architectures (from Linux/alpha to Solaris/SPARC).
>
> Regarding AIX, I've been told that it's unlikely that there will ever be a
> GitHub runner.

I assume the architecture problem is an issue for most free software
projects. The compile farm should help with that and AIX, as long as
they are available at least.

> If you would like to get involved
>   a) to be able to trigger a run,
>   b) by receiving the failure reports and triaging failures,
>   c) by maintaining the CI when things change on the GitHub site,
> please tell me and I can assign you the permissions.

It would be nice to be able to trigger runs and help fix any failures.
My GitHub is name is "collinfunk" [1].

I'm not sure if you have any "rules" for the repository, but I think it
is best to follow the standard Gnulib procedure. Send any meaningful
patches on the list so others can comment. Simple stuff like updating an
'apt' invocation to install necessary packages, for example, can be done
silently.

Collin

[1] https://github.com/collinfunk

Re: platforms without

[Collin Funk]

Hi Eli,

Eli Zaretskii  writes:

> Since this was brought up (and comes up from time to time), and since
> Collin here is listening as well, let me explain the issues we have
> with Gnulib in Emacs on MS-Windows.
>
> The main point to keep in mind is that Emacs is still striving to
> support very old Windows systems, including Windows 9X.  I know that
> Gnulib decided to drop support for those some time ago, and will
> probably drop XP as well in some not-too-distant future, but Emacs
> didn't and probably won't.

Thanks for the detailed explanations and responses to Bruno's questions.
Perhaps I'll find some time to setup a virtual machine or something with
an old Windows install. Being able to check my changes to any module
Emacs imports would be nice.

> I hope I made the situation and our logic behind it clear, but feel
> free to ask any followup questions, and thanks for listening.

Not at the moment, I think you and Bruno covered most of the important
stuff. I'm subscribed to emacs-devel so if anything ever comes to mind I
can ask.

Collin

gnulib-tool.py: Don't perform unnecessary configure.ac scanning.

[Collin Funk]

At the end of imports gnulib-tool searches for AC_PROG_CC_STDC and
AC_PROG_CC_C99 in configure.ac. If one is found it recommends replacing
them with AC_PROG_CC which is the modern way of doing things.

We only care about the macro occuring in the file so re.search() makes
more sense then re.findall(). If it is found at the start of the file
the rest of the search is just wasted time. Probably noticeable unless
one has a very long configure.ac though.

Collin

>From 80949983d7ebf1f26c9ac96badddb9db276847bb Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Wed, 5 Jun 2024 20:21:51 -0700
Subject: [PATCH] gnulib-tool.py: Don't perform unnecessary configure.ac
 scanning.

* pygnulib/GLImport.py (GLImport.execute): Use re.search() instead of
re.findall() since we only care about finding one match. Remove
unnecessary bool() calls.
---
 ChangeLog| 7 +++
 pygnulib/GLImport.py | 6 ++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index eec970671b..3c932f4f73 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-06-05  Collin Funk  
+
+	gnulib-tool.py: Don't perform unnecessary configure.ac scanning.
+	* pygnulib/GLImport.py (GLImport.execute): Use re.search() instead of
+	re.findall() since we only care about finding one match. Remove
+	unnecessary bool() calls.
+
 2024-06-05  Bruno Haible  
 
 	setenv: On native Windows, don't modify _environ directly.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index e67cbbe5a2..df541a8b77 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -1392,10 +1392,8 @@ def execute(self, filetable: GLFileTable, transformers: dict[str, tuple[re.Patte
 # Detect position_early_after.
 with open(configure_ac, mode='r', newline='\n', encoding='utf-8') as file:
 data = file.read()
-match_result1 = \
-bool(re.compile(r'^ *AC_PROG_CC_STDC', re.M).findall(data))
-match_result2 = \
-bool(re.compile(r'^ *AC_PROG_CC_C99', re.M).findall(data))
+match_result1 = re.compile(r'^ *AC_PROG_CC_STDC', re.MULTILINE).search(data)
+match_result2 = re.compile(r'^ *AC_PROG_CC_C99', re.MULTILINE).search(data)
 if match_result1:
 print('  - replace AC_PROG_CC_STDC with AC_PROG_CC in %s,' % (configure_ac))
 position_early_after = 'AC_PROG_CC_STDC'
-- 
2.45.2

Re: platforms without

[Collin Funk]

Hi Paul,

Paul Eggert  writes:

>> Which platform is this?
>
> Don't know offhand. I could ask.

An old 4.4BSD archive that I have has locale.h so I imagine it is pretty
rare.

> Right now Emacs is starting to freeze up for cutting off the next
> branch and I expect people would not be interested in changing this
> crufty old stuff right now. Probably better to ask later.
[...]
> Because that would drag in a bunch more Gnulib modules. Eli would
> rather minimize that, as he reimplements Gnulib both for ancient (so
> old that Microsoft stopped supporting it many years ago) MS-Windows
> and for not-so-ancient MS-Windows. Not my cup of tea, but not my
> decision either.

That and Android seem to require some extra consideration when it comes
to Gnulib.

By the way, my copyright assignment is in progress for Emacs. Since it
seems that you handle most of the Gnulib stuff there, feel free to ping
me if you need help investigating bugs there. Out of all Emacs users I
am probably the one with the least skill in Lisp so I can't help in that
area though. :)

Collin

Re: Gnulib's boot-time.c breaks the build with mingw.org's MinGW

[Collin Funk]

Hi Eli,

Eli Zaretskii  writes:

> However, that header is not really needed here because the
> signature of the GetTickCount64 function, which boot-time-aux.h now
> calls, is completely defined in boot-time-aux.h:
>
>   typedef ULONGLONG (WINAPI * GetTickCount64FuncType) (void);
[...]
> So I think the Gnulib code should be amended not to include
> sysinfoapi.h.  For the time being I made a local change in Emacs, to
> be able to build the master branch with MinGW, but I don't think it's
> an Emacs-specific issue.

Thanks for the report.

This code was a backup method for getting the boot time on Windows that
I added recently. On my system #include  includes 
which then includes  unconditionally. Therefore you are
correct that there is no need to include it. I've applied the attached
patch in Gnulib fixing this.

Were you using an older Windows version perhaps? For older systems where
the function isn't declared in headers (_WIN32_WINNT < _WIN32_WINNT_VISTA)
the dll is loaded and it tries to get the function address from there.
Note that the line of code you sent is a typedef not a declaration. :)

Collin

>From c41b3f92e3b0e9bd42d0ac05d80f3d23b00b28d2 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Tue, 4 Jun 2024 11:21:04 -0700
Subject: [PATCH] boot-time, readutmp: Fix missing MinGW include (regr.
 2024-05-24).

Reported by Eli Zaretskii in:
<https://lists.gnu.org/archive/html/bug-gnulib/2024-06/msg00044.html>.

* lib/boot-time.c [_WIN32 && !__CYGWIN__]: Remove unnecessary
 include. Some systems do not have this header and
 should have the correct declarations.
* lib/readutmp.c [_WIN32 && !__CYGWIN__]: Likewise.
---
 ChangeLog   | 10 ++
 lib/boot-time.c |  1 -
 lib/readutmp.c  |  1 -
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 787edff23d..1200ec0ec8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-06-04  Collin Funk  
+
+	boot-time, readutmp: Fix missing MinGW include (regr. 2024-05-24).
+	Reported by Eli Zaretskii in:
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-06/msg00044.html>.
+	* lib/boot-time.c [_WIN32 && !__CYGWIN__]: Remove unnecessary
+	 include. Some systems do not have this header and
+	 should have the correct declarations.
+	* lib/readutmp.c [_WIN32 && !__CYGWIN__]: Likewise.
+
 2024-06-03  Paul Eggert  
 
 	nstrftime: remove dependency on hard-locale
diff --git a/lib/boot-time.c b/lib/boot-time.c
index 71562dcf75..515fc48069 100644
--- a/lib/boot-time.c
+++ b/lib/boot-time.c
@@ -46,7 +46,6 @@
 #if defined _WIN32 && ! defined __CYGWIN__
 # define WIN32_LEAN_AND_MEAN
 # include 
-# include 
 # include 
 #endif
 
diff --git a/lib/readutmp.c b/lib/readutmp.c
index 82b9d4ca33..10d79d1d81 100644
--- a/lib/readutmp.c
+++ b/lib/readutmp.c
@@ -54,7 +54,6 @@
 #if defined _WIN32 && ! defined __CYGWIN__
 # define WIN32_LEAN_AND_MEAN
 # include 
-# include 
 # include 
 #endif
 
-- 
2.45.1

Re: Warnings with Android NDK r26

[Collin Funk]

Po Lu  writes:
> Let's hear from Paul first.

Sure.

>> +# define htole32 __gl_endian_htole32
[...]
> Isn't it more in line with Gnulib convention to prefix replacement
> functions with `rpl_'?

Good point. Probably yes since they are public functions. '__gl' is used
for private functions (see stdbit.in.h and binary-io.h).

Collin

Re: Warnings with Android NDK r26

[Collin Funk]

Hi Po,

Po Lu  writes:

> but because Gnulib's definitions are `inline', the functions provided in
> the system endian.h acquire external linkage, and are duplicated across
> every unit by which the redefinitions are included:
>
>   CCLD libemacs.so
> /home/.../.../android-ndk-r10b/toolchains/arm-linux-androideabi-4.8/prebuilt/linux-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.8/../../../../arm-linux-androideabi/bin/ld:
>  error: frame.o: multiple definition of 'htobe16'

My fault... I mistakenly assumed all non-glibc systems would be missing
these functions or implement them as macros.

If I am understanding correctly then I think this patch should fix it.

Can you test it for me? If it works then I can commit it and I assume
you can pull it into Emacs.

Collin

diff --git a/lib/endian.in.h b/lib/endian.in.h
index bd65ae8aab..68e79acd8b 100644
--- a/lib/endian.in.h
+++ b/lib/endian.in.h
@@ -69,8 +69,9 @@ _GL_INLINE_HEADER_BEGIN
 # define BYTE_ORDER LITTLE_ENDIAN
 #endif
 
-/* Make sure function-like macros get undefined.  */
 #if @HAVE_ENDIAN_H@
+
+/* Make sure we don't have any system definitions.  */
 # undef be16toh
 # undef be32toh
 # undef be64toh
@@ -83,6 +84,21 @@ _GL_INLINE_HEADER_BEGIN
 # undef htole16
 # undef htole32
 # undef htole64
+
+/* Define our own.  */
+# define be16toh __gl_endian_be16toh
+# define be32toh __gl_endian_be32toh
+# define be64toh __gl_endian_be64toh
+# define htobe16 __gl_endian_htobe16
+# define htobe32 __gl_endian_htobe32
+# define htobe64 __gl_endian_htobe64
+# define le16toh __gl_endian_le16toh
+# define le32toh __gl_endian_le32toh
+# define le64toh __gl_endian_le64toh
+# define htole16 __gl_endian_htole16
+# define htole32 __gl_endian_htole32
+# define htole64 __gl_endian_htole64
+
 #endif
 
 #ifdef __cplusplus

Re: gnulib-tool.py: Refactor duplicated regular expressions.

[Collin Funk]

Bruno Haible  writes:

>> The lists there seem small enough that I doubt it matters too much.
>
> Bad excuse :) You haven't seen this module yet:
> https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=gnulib-local/modules/libxml;h=dc6404c59362e5d17885a211a61a4eed99da6642;hb=HEAD#l130

Hahaha, I concede my point then. :)

This comment is interesting:

  # We need NO_EXPENSIVE_WARN_CFLAGS, because with gcc 13, the compilation of
  # libxml/parser.c requires 1.9 GB of RAM with -fanalyzer, as opposed to
  # 0.09 GB of RAM with -fno-analyzer.

I noticed that -fanalyzer is slow on files with many lines (e.g.
vasnprintf.c, many Emacs sources) but I never looked into memory usage.

>> Perhaps a patch like this is a better fix then just wrapping the call in
>> set() there:
>
> Although it would work to make _extract_lib_SOURCES return a set (since
> for both callers, the order of the result is irrelevant), it is more
> maintainable to let _extract_lib_SOURCES return a list and let each caller
> decide whether they need a set(...) conversion or not. This way, if a
> third caller is added in the future, you don't need to undo the optimization
> in _extract_lib_SOURCES.

Good point. I've pushed this patch adding a set() around that one call.

Collin

>From c1cdc5ef8ef8b22d46bf55d8cb992f0b75532b46 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 3 Jun 2024 04:57:14 -0700
Subject: [PATCH] gnulib-tool.py: Use a set to optimize.

* pygnulib/GLModuleSystem.py
(GLModule.getAutomakeSnippet_Unconditional): Call set() on the result of
_extract_lib_SOURCES() to ensure computing the difference between
another set is O(n).
---
 ChangeLog  | 8 
 pygnulib/GLModuleSystem.py | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 40238433eb..3e1e553dc4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-06-03  Collin Funk  
+
+	gnulib-tool.py: Use a set to optimize.
+	* pygnulib/GLModuleSystem.py
+	(GLModule.getAutomakeSnippet_Unconditional): Call set() on the result of
+	_extract_lib_SOURCES() to ensure computing the difference between
+	another set is O(n).
+
 2024-06-03  Bruno Haible  
 
 	pthread-* tests, regex tests: Prepare for use of 'alarm'.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 5d67c5b6df..02dacfcf9f 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -587,7 +587,7 @@ def getAutomakeSnippet_Unconditional(self) -> str:
 snippet = self.getAutomakeSnippet_Conditional()
 snippet = combine_lines(snippet)
 # Get all the file names from 'lib_SOURCES += ...'.
-mentioned_files = _extract_lib_SOURCES(snippet)
+mentioned_files = set(_extract_lib_SOURCES(snippet))
 all_files = self.getFiles()
 lib_files = filter_filelist('\n', all_files,
 'lib/', '', 'lib/', '')
-- 
2.45.1

Re: gnulib-tool.py: Refactor duplicated regular expressions.

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> The variable mentioned_files in pygnulib/GLModuleSystem.py line 599
> was a set and is now a list. Is the expression
>   lib_files.difference(mentioned_files)
> therefore being evaluated more slowly (as O(n²) where it was O(n) before)?
> That is, does Python iterate through mentioned_files for each element of
> lib_files, or does Python convert the argument mentioned_files internally
> to a set first?

Oops yes. Good point, I must have overlooked that. It appears that it
will be O(n²). In this loop [1] and the else branch of
"PyAnySet_Check(other)" [2].

The lists there seem small enough that I doubt it matters too much.
Perhaps a patch like this is a better fix then just wrapping the call in
set() there:

diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 5d67c5b6df..b91406f564 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -45,14 +45,14 @@
 _LIB_SOURCES_PATTERN = re.compile(r'^lib_SOURCES[\t ]*\+=[\t ]*(.+?)[\t 
]*(?:#|$)', re.MULTILINE)
 
 
-def _extract_lib_SOURCES(snippet: str) -> list[str]:
+def _extract_lib_SOURCES(snippet: str) -> set[str]:
 '''Return the file names specified in the lib_SOURCES variable of
 a Makefile.am snippet.'''
 lines = [ line.group(1)
   for line in re.finditer(_LIB_SOURCES_PATTERN, snippet) ]
-return [ file_name
+return { file_name
  for line in lines
- for file_name in line.split() ]
+ for file_name in line.split() }


I want to move some other file list stuff to use sets
(GLModuleSystem.filelist particularly) since membership checks are
performed on them and there is many unnecessary sorted() calls that can
be cleaned up.

Sets would work better for GLModule objects too but sorting matters for
them because 'dummy' is always placed last. Sort of strange behavior
that is necessary to match gnulib-tool.sh output in Makefile.am. :)

Collin

[1] 
https://github.com/python/cpython/blob/059be67b51717519609b29c53bf742ca4d91b68f/Objects/setobject.c#L1576
[2] 
https://github.com/python/cpython/blob/059be67b51717519609b29c53bf742ca4d91b68f/Objects/setobject.c#L1430

gnulib-tool.py: Refactor duplicated regular expressions.

[Collin Funk]

In GLModuleSystem.py Makefile.am snippets are searched twice for the
file names stored in 'lib_SOURCES += ...' variable. Instead of writing
the regular expression twice (differently) I have moved it to a private
variable & function.

Indirectly this also fixes two issues. The first is the same issue as
yesterdays patch. Variables with a trailing comment, e.g.

lib_SOURCES += file.c file.h # comment

are not matched. As far as I can tell none of the module descriptions
have this comment style but gnulib-tool.sh handles it (line 3453-3466).
So gnulib-tool.py should too.

The second issue I have mentioned previously, I forget where. In some
nested loops we have "break" statements that are ineffective. Unlike
shell, in Python we can't use "break 2" to exit the outer loop.

In this case we had 3 loops nested, something like this:

 for module in modules:
 for value in lib_SOURCES_var:
for file_name in value:
if not file_name.endswith('.h'):
   condition = True
   break

So the break only stops the current loop 'lib_SOURCES' match of a
snippet. But we should stop looping entirely if even one module
description sets condition to True.

Since the use of a private functions removes a level of nesting, I have
added an 'if condition: break'. Not too messy and since the outer loop
goes over all modules (can be 1000+) it seems worth it.

Collin

>From 26d7603143116626cacba37b988fa26cc7dba1b8 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sun, 2 Jun 2024 22:32:39 -0700
Subject: [PATCH] gnulib-tool.py: Refactor duplicated regular expressions.

* pygnulib/GLModuleSystem.py (_LIB_SOURCES_PATTERN): New variable.
(_extract_lib_SOURCES): New function.
(GLModule.getAutomakeSnippet_Unconditional): Use the new function.
(GLModuleTable.add_dummy): Likewise. Add a second break statement to
stop unnecessary looping.
---
 ChangeLog  |  9 +
 pygnulib/GLModuleSystem.py | 40 --
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 269293ca79..e0ed08a9af 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-06-02  Collin Funk  
+
+	gnulib-tool.py: Refactor duplicated regular expressions.
+	* pygnulib/GLModuleSystem.py (_LIB_SOURCES_PATTERN): New variable.
+	(_extract_lib_SOURCES): New function.
+	(GLModule.getAutomakeSnippet_Unconditional): Use the new function.
+	(GLModuleTable.add_dummy): Likewise. Add a second break statement to
+	stop unnecessary looping.
+
 2024-06-02  Bruno Haible  
 
 	c-strtod, c-strtof, c-strtold: Fix link error on AIX.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 5b8d331dd6..5d67c5b6df 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -40,6 +40,21 @@
 from .GLFileSystem import GLFileSystem
 
 
+# Regular expression matching a 'lib_SOURCES += ...' variable with the file
+# names in capture group 1.
+_LIB_SOURCES_PATTERN = re.compile(r'^lib_SOURCES[\t ]*\+=[\t ]*(.+?)[\t ]*(?:#|$)', re.MULTILINE)
+
+
+def _extract_lib_SOURCES(snippet: str) -> list[str]:
+'''Return the file names specified in the lib_SOURCES variable of
+a Makefile.am snippet.'''
+lines = [ line.group(1)
+  for line in re.finditer(_LIB_SOURCES_PATTERN, snippet) ]
+return [ file_name
+ for line in lines
+ for file_name in line.split() ]
+
+
 #===
 # Define GLModuleSystem class
 #===
@@ -571,13 +586,8 @@ def getAutomakeSnippet_Unconditional(self) -> str:
 # Synthesize an EXTRA_DIST augmentation.
 snippet = self.getAutomakeSnippet_Conditional()
 snippet = combine_lines(snippet)
-pattern = re.compile(r'^lib_SOURCES[\t ]*\+=[\t ]*(.*)$', re.MULTILINE)
-mentioned_files = set(pattern.findall(snippet))
-if mentioned_files:
-# Get all the file names from 'lib_SOURCES += ...'.
-mentioned_files = { filename
-for line in mentioned_files
-for filename in line.split() }
+# Get all the file names from 'lib_SOURCES += ...'.
+mentioned_files = _extract_lib_SOURCES(snippet)
 all_files = self.getFiles()
 lib_files = filter_filelist('\n', all_files,
 'lib/', '', 'lib/', '')
@@ -985,14 +995,14 @@ def add_dummy(self, modules: list[GLModule]) -> list[GLModule]:
 # Extract the value of unconditional "lib_SOURCES += ..." augmentations.
 snippet = combine_lines(snippet)
 snippet = self.remove_if_blocks(snippe

Re: c-strtod, c-strtof, c-strtold: Fix link error on AIX

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> On AIX 7.1, in a testdir of all of gnulib, I see these link errors, both
> with gcc and xlc:
>
> gcc  -Wno-error -g -O2  -L/home/haible/prefix32/lib -o test-c-strtod 
> test-c-strtod.o libtests.a ../gllib/libgnu.a libtests.a ../gllib/libgnu.a 
> libtests.a  -lm  -lm -lm -lm -lm -lm -lm -lm -lm -lm
> ld: 0711-224 WARNING: Duplicate symbol: .rpl_duplocale
> ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information.
> ld: 0711-317 ERROR: Undefined symbol: .pthread_mutex_lock

Thanks. I noticed this yesterday but didn't get around to fixing it.

Collin

Re: gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.

[Collin Funk]

Bruno Haible  writes:

>> > Thanks for this recipe. I think this is worth a unit test, since it
>> > is a special case that is otherwise not tested.
>> 
>> Agreed. I can add one later today.
>
> I added it already.

Thanks, I've applied the attached patch fixing that regular expression.

Also, Paul while investigating this I noticed some strange behavior. The
Autoconf documentation mentions the limit to a single line for
ACLOCAL_AMFLAGS. Perhaps it is worth changing this regexp [1] or
mentioning this in the documentation [2].

Here is a test case:

   $ gnulib-tool --create-testdir --dir testdir1 README-release
   $ cd testdir1
   $ sed -i -e 's/ACLOCAL_AMFLAGS = -I glm4/ACLOCAL_AMFLAGS = -I glm4 # 
--help/g' Makefile.am
   $ cat Makefile.am
 [...]
 ACLOCAL_AMFLAGS = -I glm4 # --help
   $ autoreconf
 Usage: aclocal [OPTION]...

 Generate 'aclocal.m4' by scanning 'configure.ac' or 'configure.in'
 [...]

If you remove aclocal.m4 and try to autoreconf the build will fail
because m4 macros cannot be found of course.

I don't know enough about Autotools to tell if anything silly can be
done using this trick. Not that you would run auto* in a project that
you don't trust anyways...

Collin

[1] 
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/autoconf.html#index-AC_005fCONFIG_005fMACRO_005fDIR-1
[2] https://git.savannah.gnu.org/cgit/autoconf.git/tree/bin/autoreconf.in#n532

>From 1372dc1631053fa72bbf5755cbc89f63cbffec33 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sun, 2 Jun 2024 05:29:33 -0700
Subject: [PATCH] gnulib-tool.py: Fix regular expression (regr. today).

* pygnulib/main.py (main) [import]: Match all characters until '#' or
end of line, whichever comes first.
---
 ChangeLog| 4 
 pygnulib/main.py | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 3a982c4988..f076c168b0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2024-06-02  Collin Funk  
 
+	gnulib-tool.py: Fix regular expression (regr. today).
+	* pygnulib/main.py (main) [import]: Match all characters until '#' or
+	end of line, whichever comes first.
+
 	gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.
 	* pygnulib/main.py (main) [import]: Use a regular expression to match
 	the ACLOCAL_AMFLAGS Makefile.am variable. Properly handle the case where
diff --git a/pygnulib/main.py b/pygnulib/main.py
index 6167135858..c1c2cd2c1e 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -987,7 +987,7 @@ def main(temp_directory: str) -> None:
 if os.path.isfile(filepath):
 with open(filepath, mode='r', newline='\n', encoding='utf-8') as file:
 data = file.read()
-pattern = re.compile(r'^ACLOCAL_AMFLAGS[\t ]*=[\t ]*([^#]+?)$', re.MULTILINE)
+pattern = re.compile(r'^ACLOCAL_AMFLAGS[\t ]*=[\t ]*(.+?)[\t ]*(?:#|$)', re.MULTILINE)
 match = re.search(pattern, data)
 if match:
 aclocal_amflags = match.group(1).split()
-- 
2.45.1

Re: gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

> Thanks for this recipe. I think this is worth a unit test, since it
> is a special case that is otherwise not tested.

Agreed. I can add one later today.

> The regular expression doesn't seem right:
[...]
> The point of using a non-greedy operator is to not include newlines
> in the match, right? So,
>   re.compile(r'^ACLOCAL_AMFLAGS[\t ]*=[\t ]*([^#]*)', re.MULTILINE)
> wouldn't work?

Oops, good catch. I was trying to be clever and failed. :)

Here is something like what I was trying to do:

===
import re
string = """
ACLOCAL_AMFLAGS =
ACLOCAL_AMFLAGS = -I m4 -I glm4 # ignore this please
"""
pattern = re.compile(r'^ACLOCAL_AMFLAGS[\t ]*=[\t ]*(.+?)[\t ]*(?:#|$)', 
re.MULTILINE)
print('"' + re.search(pattern, string).group(1) + '"')
===

Not sure if Makefile.am's are written like that in practice but here is
the output:

$ ./main.py 
"-I m4 -I glm4"

Collin

gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.

[Collin Funk]

Not sure how this didn't get found sooner, to be honest. Here is an
example of the bug:

$ git clone https://git.savannah.gnu.org/git/rcs.git
$ cd rcs
$ git checkout next
$ sh -x autogen.sh
[...]
Traceback (most recent call last):
File "/home/collin/.local/src/gnulib/.gnulib-tool.py", line 30, in 
  main.main_with_exception_handling()
File "/home/collin/.local/src/gnulib/pygnulib/main.py", line 1382, in 
main_with_exception_handling
  main(temporary_directory)
File "/home/collin/.local/src/gnulib/pygnulib/main.py", line 990, in main
  data = data.split('ACLOCAL_AMFLAGS')[1]
 ~^^^
IndexError: list index out of range

I don't remember if I wrote this or not. But regular expressions work
better here than str.find() and indexing. That variable is limited to a
single line so no need to worry about backslash continuations [1].

I've applied this patch fixing it and confirmed it passes with
GNULIB_TOOL_IMPL=sh+py.

Collin

[1] 
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/autoconf.html#index-AC_005fCONFIG_005fMACRO_005fDIR-1

>From 80ed61e8cd8a447dbe1f16e63ac94e6762a795bf Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sun, 2 Jun 2024 01:06:32 -0700
Subject: [PATCH] gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.

* pygnulib/main.py (main) [import]: Use a regular expression to match
the ACLOCAL_AMFLAGS Makefile.am variable. Properly handle the case where
none is found.
---
 ChangeLog| 7 +++
 pygnulib/main.py | 9 ++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 67b0ea66ef..3a982c4988 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-06-02  Collin Funk  
+
+	gnulib-tool.py: Fix crash when no ACLOCAL_AMFLAGS is found.
+	* pygnulib/main.py (main) [import]: Use a regular expression to match
+	the ACLOCAL_AMFLAGS Makefile.am variable. Properly handle the case where
+	none is found.
+
 2024-05-31  Bruno Haible  
 
 	windows-once: Improve comments.
diff --git a/pygnulib/main.py b/pygnulib/main.py
index 68be0ba28f..6167135858 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -987,9 +987,12 @@ def main(temp_directory: str) -> None:
 if os.path.isfile(filepath):
 with open(filepath, mode='r', newline='\n', encoding='utf-8') as file:
 data = file.read()
-data = data.split('ACLOCAL_AMFLAGS')[1]
-data = data[data.find('=') + 1 : data.find('\n')]
-aclocal_amflags = data.split()
+pattern = re.compile(r'^ACLOCAL_AMFLAGS[\t ]*=[\t ]*([^#]+?)$', re.MULTILINE)
+match = re.search(pattern, data)
+if match:
+aclocal_amflags = match.group(1).split()
+else:
+aclocal_amflags = []
 for aclocal_amflag in aclocal_amflags:
 if dirisnext:
 # Ignore absolute directory pathnames, like /usr/local/share/aclocal.
-- 
2.45.1

Re: endian: Quote variables that may be undefined (regr. 2024-05-18).

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:

>> I've attached this patch fixing it.
>
> The patch overshoots the goal. Namely, if one systematically uses 
> double-quotes
> for every variable access, it is harder to notice typos and copy 
> mistakes
> (which are not unfrequent in Gnulib, because we have many idioms and copy
> code from one .m4 file to another).

Ah, okay. I was conflicted on this because on one hand quoting variables
is safer (undefined variables, special characters, etc.). However, as
you mention it makes simple mistakes like typos go unnoticed. Thanks for
explaining the conventions.

>> or may be defined by the user
>
> This is not a worthy consideration. Users are not meant to set *_cv_*
> values to "" or "non sense", only to "yes" and "no" (or otherwise as
> appropriate for the specific AC_CACHE_CHECK invocation).
>
> Can you please revert the double-quotes for those two variables that
> don't need it?

I think I understand now. I've applied this patch removing the quotes
from $ac_cv_header_endian_h which is set by AC_CHECK_HEADERS_ONCE and
$GL_GENERATE_ENDIAN_H which is always set to true or false.

Collin

>From 9154b1dc075796af79bce7634b70d447cf485569 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 30 May 2024 13:30:25 -0700
Subject: [PATCH] endian: Unquote variables that are always defined.

* m4/endian.m4 (gl_ENDIAN_H): Unquote $ac_cv_header_endian_h and
$GL_GENERATE_ENDIAN_H.
---
 ChangeLog  | 6 ++
 m4/endian_h.m4 | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2d96de1066..5439029905 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-30  Collin Funk  
+
+	endian: Unquote variables that are always defined.
+	* m4/endian.m4 (gl_ENDIAN_H): Unquote $ac_cv_header_endian_h and
+	$GL_GENERATE_ENDIAN_H.
+
 2024-05-30  Bruno Haible  
 
 	attribute: Try harder to avoid syntax errors.
diff --git a/m4/endian_h.m4 b/m4/endian_h.m4
index a3b43b25b7..3149b49227 100644
--- a/m4/endian_h.m4
+++ b/m4/endian_h.m4
@@ -1,5 +1,5 @@
 # endian_h.m4
-# serial 3
+# serial 4
 dnl Copyright 2024 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -13,7 +13,7 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
 
   AC_CHECK_HEADERS_ONCE([endian.h])
   gl_CHECK_NEXT_HEADERS([endian.h])
-  if test "$ac_cv_header_endian_h" = yes; then
+  if test $ac_cv_header_endian_h = yes; then
 HAVE_ENDIAN_H=1
 dnl Check if endian.h defines uint16_t, uint32_t, and uint64_t.
 AC_CACHE_CHECK([if endian.h defines stdint types],
@@ -88,7 +88,7 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
   fi
 
   dnl Check if endian.h works but is missing types from stdint.h.
-  if test "$GL_GENERATE_ENDIAN_H"; then
+  if test $GL_GENERATE_ENDIAN_H; then
 if test "$gl_cv_header_working_endian_h" = yes; then
   ENDIAN_H_JUST_MISSING_STDINT=1
 else
-- 
2.45.1

endian: Quote variables that may be undefined (regr. 2024-05-18).

[Collin Funk]

Hello,

While learning m4/autoconf I seemed to forget about the importance of
quoting variables (see a similar issue Paul fixed with my byteswap.m4
changes)...

In a testdir on an AIX machine (cfarm111) I see this warning:

./configure[7173]: test: argument expected
./configure[7181]: test: argument expected

This is because the variables are not defined on this system and expand
to nothing. I've attached this patch fixing it. Lesson learned.

Collin

>From 2fc3ada5f86f05a0f10d2fe737de996011462e48 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 30 May 2024 04:46:29 -0700
Subject: [PATCH] endian: Quote variables that may be undefined (regr.
 2024-05-18).

* m4/endian_h.m4 (gl_ENDIAN_H): Quote variables that are undefined on
some systems or may be defined by the user.
---
 ChangeLog  |  6 ++
 m4/endian_h.m4 | 12 ++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 18a508ec94..2792ccbb79 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-30  Collin Funk  
+
+	endian: Quote variables that may be undefined (regr. 2024-05-18).
+	* m4/endian_h.m4 (gl_ENDIAN_H): Quote variables that are undefined on
+	some systems or may be defined by the user.
+
 2024-05-30  Bruno Haible  
 
 	call_once: Work around Cygwin 3.5.3 bug.
diff --git a/m4/endian_h.m4 b/m4/endian_h.m4
index 29dab603e3..a3b43b25b7 100644
--- a/m4/endian_h.m4
+++ b/m4/endian_h.m4
@@ -1,5 +1,5 @@
 # endian_h.m4
-# serial 2
+# serial 3
 dnl Copyright 2024 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -13,7 +13,7 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
 
   AC_CHECK_HEADERS_ONCE([endian.h])
   gl_CHECK_NEXT_HEADERS([endian.h])
-  if test $ac_cv_header_endian_h = yes; then
+  if test "$ac_cv_header_endian_h" = yes; then
 HAVE_ENDIAN_H=1
 dnl Check if endian.h defines uint16_t, uint32_t, and uint64_t.
 AC_CACHE_CHECK([if endian.h defines stdint types],
@@ -80,16 +80,16 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
   fi
 
   dnl Check if endian.h should be generated.
-  if test $gl_cv_header_endian_h_stdint_types = yes \
- && test $gl_cv_header_working_endian_h = yes; then
+  if test "$gl_cv_header_endian_h_stdint_types" = yes \
+ && test "$gl_cv_header_working_endian_h" = yes; then
 GL_GENERATE_ENDIAN_H=false
   else
 GL_GENERATE_ENDIAN_H=true
   fi
 
   dnl Check if endian.h works but is missing types from stdint.h.
-  if test $GL_GENERATE_ENDIAN_H; then
-if test $gl_cv_header_working_endian_h = yes; then
+  if test "$GL_GENERATE_ENDIAN_H"; then
+if test "$gl_cv_header_working_endian_h" = yes; then
   ENDIAN_H_JUST_MISSING_STDINT=1
 else
   ENDIAN_H_JUST_MISSING_STDINT=0
-- 
2.45.1

Re: [PATCH] gnulib-tool.py: Simplify creation of module shell ids.

[Collin Funk]

Collin Funk  writes:
> In the documentation for \w it states [2]:
>
> Matches [a-zA-Z0-9_] if the ASCII flag is used.

I've committed this patch adding the ASCII flag.

I noticed some other duplicated regular expressions that could probably
be simplified. I'll take a look at seeing if re.ASCII is applicable to
those too.

Collin

>From 1b74469a3aaa1a4bbe94272fde87c2335afb89e1 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Wed, 29 May 2024 02:35:02 -0700
Subject: [PATCH] gnulib-tool.py: Don't emit non-ASCII shell output.

Reported by Bruno Haible in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00441.html>.

* pygnulib/GLModuleSystem.py (GLModule.shell_id_chars): Use the re.ASCII
flag for the regular expression.
---
 ChangeLog  | 8 
 pygnulib/GLModuleSystem.py | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 9c22b7fb38..87e25ab81e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-29  Collin Funk  
+
+	gnulib-tool.py: Don't emit non-ASCII shell output.
+	Reported by Bruno Haible in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00441.html>.
+	* pygnulib/GLModuleSystem.py (GLModule.shell_id_chars): Use the re.ASCII
+	flag for the regular expression.
+
 2024-05-28  Collin Funk  
 
 	gnulib-tool.py: Add missing docstring.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index f1774f64fc..5b8d331dd6 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -167,7 +167,7 @@ class GLModule:
re.M)
 
 # Regular expression matching module names that can be used as shell ids.
-shell_id_pattern: ClassVar[re.Pattern] = re.compile(r'^\w*$')
+shell_id_pattern: ClassVar[re.Pattern] = re.compile(r'^\w*$', re.ASCII)
 
 cache: dict[str, Any]
 content: str
-- 
2.45.1

Re: [PATCH] gnulib-tool.py: Simplify creation of module shell ids.

[Collin Funk]

Hi Bruno,

Bruno Haible  writes:
> But that's not the same thing: The previous code made sure that
> module names with non-ASCII characters use the MD5 hash code.
> The new code does not:
>
 re.match(re.compile(r'^[abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_]*$'),'abcäöü')
>
 re.match(re.compile(r'^\w*$'),'abcäöü')
> 
>
> If someone ever uses module names with non-ASCII characters, we do
> *not* want to pass these characters into shell variable names.

Oops, yes good point. I overlooked that. It looks like re.ASCII should
do the job [1].

In the documentation for \w it states [2]:

Matches [a-zA-Z0-9_] if the ASCII flag is used.

Collin

[1] https://docs.python.org/3/library/re.html#re.ASCII
[2] https://docs.python.org/3/library/re.html#regular-expression-syntax

Re: [PATCH] gnulib-tool.py: Simplify creation of module shell ids.

[Collin Funk]

Collin Funk  writes:
> +def getShellId(self) -> str:
> +if re.match(self.shell_id_pattern, self.name):
> +return self.name
> +return 
> hashlib.md5(f'{self.name}\n'.encode(ENCS['default'])).hexdigest()
> +

Oops, I forgot to write a docstring...

I've pushed this patch fixing it.

Also, I've added a comment about the '\n' following the module name. One
might be tempted to remove it. However, when using
--conditional-dependencies that would cause gnulib-tool.sh and
gnulib-tool.py to produce different output.

Collin

>From 001302d065dd00a90f33a296bd613c1a93b197bb Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Tue, 28 May 2024 23:26:19 -0700
Subject: [PATCH] gnulib-tool.py: Add missing docstring.

* pygnulib/GLModuleSystem.py (GLModule.getShellId): Add docstring
forgotten in the previous commit.
---
 ChangeLog  | 4 
 pygnulib/GLModuleSystem.py | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 9be0407793..9c22b7fb38 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2024-05-28  Collin Funk  
 
+	gnulib-tool.py: Add missing docstring.
+	* pygnulib/GLModuleSystem.py (GLModule.getShellId): Add docstring
+	forgotten in the previous commit.
+
 	gnulib-tool.py: Simplify creation of module shell ids.
 	* pygnulib/GLModuleSystem.py (GLModule.shell_id_chars): Remove class
 	variable.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 63d4f4a73e..f1774f64fc 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -379,8 +379,11 @@ def getLinkDirectiveRecursively(self) -> str:
 return lines_to_multiline(directives)
 
 def getShellId(self) -> str:
+'''Return an unique id suitable for use in shell scripts. If the
+module name is not a valid shell identifier use its MD5 digest.'''
 if re.match(self.shell_id_pattern, self.name):
 return self.name
+# Newline character needed for compatibility with gnulib-tool.sh.
 return hashlib.md5(f'{self.name}\n'.encode(ENCS['default'])).hexdigest()
 
 def getShellFunc(self) -> str:
-- 
2.45.1

[PATCH] gnulib-tool.py: Simplify creation of module shell ids.

[Collin Funk]

* pygnulib/GLModuleSystem.py (GLModule.shell_id_chars): Remove class
variable.
(GLModule.shell_id_pattern): New class variable.
(GLModule.getShellId): New function.
(GLModule.getShellFunc, GLModule.getShellVar)
(GLModule.getConditionalName): Use it.
---
 ChangeLog  | 10 
 pygnulib/GLModuleSystem.py | 48 --
 2 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a405680df3..9be0407793 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-05-28  Collin Funk  
+
+   gnulib-tool.py: Simplify creation of module shell ids.
+   * pygnulib/GLModuleSystem.py (GLModule.shell_id_chars): Remove class
+   variable.
+   (GLModule.shell_id_pattern): New class variable.
+   (GLModule.getShellId): New function.
+   (GLModule.getShellFunc, GLModule.getShellVar)
+   (GLModule.getConditionalName): Use it.
+
 2024-05-28  Bruno Haible  
 
pthread-once: Work around Cygwin 3.5.3 bug.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 872961ba91..63d4f4a73e 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -166,8 +166,8 @@ class GLModule:
+ r'Makefile\.am|Include|Link|License|Maintainer):$',
re.M)
 
-# List of characters allowed in shell identifiers.
-shell_id_chars: ClassVar[str] = 
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_'
+# Regular expression matching module names that can be used as shell ids.
+shell_id_pattern: ClassVar[re.Pattern] = re.compile(r'^\w*$')
 
 cache: dict[str, Any]
 content: str
@@ -378,56 +378,28 @@ def getLinkDirectiveRecursively(self) -> str:
   for line in section.splitlines() })
 return lines_to_multiline(directives)
 
+def getShellId(self) -> str:
+if re.match(self.shell_id_pattern, self.name):
+return self.name
+return 
hashlib.md5(f'{self.name}\n'.encode(ENCS['default'])).hexdigest()
+
 def getShellFunc(self) -> str:
 '''Computes the shell function name that will contain the m4 macros
 for the module.'''
 macro_prefix = self.config['macro_prefix']
-valid_shell_id = True
-for char in self.name:
-if char not in GLModule.shell_id_chars:
-valid_shell_id = False
-break
-if valid_shell_id:
-identifier = self.name
-else:
-hash_input = '%s\n' % self.name
-identifier = 
hashlib.md5(hash_input.encode(ENCS['default'])).hexdigest()
-result = 'func_%s_gnulib_m4code_%s' % (macro_prefix, identifier)
-return result
+return 'func_%s_gnulib_m4code_%s' % (macro_prefix, self.getShellId())
 
 def getShellVar(self) -> str:
 '''Compute the shell variable name the will be set to true once the
 m4 macros for the module have been executed.'''
 macro_prefix = self.config['macro_prefix']
-valid_shell_id = True
-for char in self.name:
-if char not in GLModule.shell_id_chars:
-valid_shell_id = False
-break
-if valid_shell_id:
-identifier = self.name
-else:
-hash_input = '%s\n' % self.name
-identifier = 
hashlib.md5(hash_input.encode(ENCS['default'])).hexdigest()
-result = '%s_gnulib_enabled_%s' % (macro_prefix, identifier)
-return result
+return '%s_gnulib_enabled_%s' % (macro_prefix, self.getShellId())
 
 def getConditionalName(self) -> str:
 '''Return the automake conditional name.
 GLConfig: macro_prefix.'''
 macro_prefix = self.config['macro_prefix']
-valid_shell_id = True
-for char in self.name:
-if char not in GLModule.shell_id_chars:
-valid_shell_id = False
-break
-if valid_shell_id:
-identifier = self.name
-else:
-hash_input = '%s\n' % self.name
-identifier = 
hashlib.md5(hash_input.encode(ENCS['default'])).hexdigest()
-result = '%s_GNULIB_ENABLED_%s' % (macro_prefix, identifier)
-return result
+return '%s_GNULIB_ENABLED_%s' % (macro_prefix, self.getShellId())
 
 def getDescription(self) -> str:
 '''Return description of the module.'''
-- 
2.45.1

Re: RFC: Adding tcgetwinsize and tcsetwinsize.

[Collin Funk]

Bruno Haible  writes:
>> > This needs thought first, because where on the Unix side you have
>> > two concepts:
>> >   - winsize from the OS,
>> >   - lines and columns from the terminfo description of $TERM,
>> > and the LINES and COLUMNS environment variables,
>> > on the Windows side you have only the Consoles.
>> > 
>> > How are these concepts delimited (on the Unix side)?
>
> IMO the documentation should tell the application writer:
>   - when should tcgetwinsize be used?
>   - when should $TERM be used?
>   - when should $LINES, $COLUMNS be used?
> Three concepts. Which to use in which situations?

Sure, perhaps I will write a texinfo page for that function before
anything else.

Section 8.3 "Other Environment Variables" has a decent description [1].
The newest draft is better since it mentions tcgetwinsize & tcsetwinsize.

The process goes something like:

1. If $LINES and $COLUMNS are set to positive decimal numbers use them.
   - POSIX recommends users leave these unset unless there is a reason
 to override system behavior (e.g. output narrower than the actual
 window).
2. If they are unset (or invalid) use a system interface such as
   tcgetwinsize or ioctl to query the terminal window size. Use that
   size or smaller.
3. If those interfaces fail or do not exist on your system then pick a
   small size appropriate for a terminal window. POSIX recommends
   scaling with the baud rate, but I feel like it is standard practice
   to assume things work and use 80x24 if not.

I think bash updates $LINES and $COLUMNS after every command by default,
but I'm not sure how other shells behave.

Collin

[1] https://pubs.opengroup.org/onlinepubs/9699919799/

doc: Mention byteswap.h as a system header.

[Collin Funk]

Just some minor forgotten chores after adding new header substitutes.

2024-05-27  Collin Funk  

maint.mk: Update system header list for #include syntax checks.
* top/maint.mk (gl_prefer_angle_bracket_headers_): Add byteswap.h,
endian.h, and stdbit.h.

doc: Mention byteswap.h as a system header.
* doc/gnulib-tool.texi (Style of #include statements): Add byteswap.h.

Collin

>From a03da3b1a377bf9e5375f91b3be28f16c6a32dbe Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 27 May 2024 21:26:48 -0700
Subject: [PATCH 1/2] doc: Mention byteswap.h as a system header.

* doc/gnulib-tool.texi (Style of #include statements): Add byteswap.h.
---
 ChangeLog| 5 +
 doc/gnulib-tool.texi | 1 +
 2 files changed, 6 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 9ba81c3b5f..707ab5175d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2024-05-27  Collin Funk  
+
+	doc: Mention byteswap.h as a system header.
+	* doc/gnulib-tool.texi (Style of #include statements): Add byteswap.h.
+
 2024-05-27  Bruno Haible  
 
 	nstrftime, c-nstrftime tests: Avoid test failures on native Windows.
diff --git a/doc/gnulib-tool.texi b/doc/gnulib-tool.texi
index 116734f4d7..0f059e1287 100644
--- a/doc/gnulib-tool.texi
+++ b/doc/gnulib-tool.texi
@@ -620,6 +620,7 @@ @node Style of #include statements
 but are not standardized:
 @itemize @asis{}
 @item @code{alloca.h}
+@item @code{byteswap.h}
 @item @code{error.h}
 @item @code{getopt.h}
 @item @code{malloc.h}
-- 
2.45.1

>From ad61731042a23c0ca31e14be99c0dcc49468 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 27 May 2024 21:35:43 -0700
Subject: [PATCH 2/2] maint.mk: Update system header list for #include syntax
 checks.

* top/maint.mk (gl_prefer_angle_bracket_headers_): Add byteswap.h,
endian.h, and stdbit.h.
---
 ChangeLog| 4 
 top/maint.mk | 6 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 707ab5175d..4ac06a605d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2024-05-27  Collin Funk  
 
+	maint.mk: Update system header list for #include syntax checks.
+	* top/maint.mk (gl_prefer_angle_bracket_headers_): Add byteswap.h,
+	endian.h, and stdbit.h.
+
 	doc: Mention byteswap.h as a system header.
 	* doc/gnulib-tool.texi (Style of #include statements): Add byteswap.h.
 
diff --git a/top/maint.mk b/top/maint.mk
index ecd8971900..c20059fbae 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -538,13 +538,16 @@ sc_require_config_h_first:
 	fi
 
 # Generated headers that override system headers.
-# Keep sorted.
+# These are documented in gnulib-tool.texi.  Keep sorted.
+# sed -n -e 's/^@item[[:space:]]\{1,\}@code{\([^}]\{1,\}\)}$/\1/p' $GNULIB_SRCDIR/doc/gnulib-tool.texi  | sort -u
 gl_prefer_angle_bracket_headers_ ?= \
   alloca.h		\
   arpa/inet.h		\
   assert.h		\
+  byteswap.h		\
   ctype.h		\
   dirent.h		\
+  endian.h		\
   errno.h		\
   error.h		\
   fcntl.h		\
@@ -575,6 +578,7 @@ gl_prefer_angle_bracket_headers_ ?= \
   spawn.h		\
   stdalign.h		\
   stdarg.h		\
+  stdbit.h		\
   stddef.h		\
   stdint.h		\
   stdio.h		\
-- 
2.45.1

Re: RFC: Adding tcgetwinsize and tcsetwinsize.

[Collin Funk]

Hi Bruno,

On 5/27/24 4:58 PM, Bruno Haible wrote:
> I would only add tcgetwinsize, for two reasons:
>   - The man page that you cite says "In general, the tcsetwinsize function
> is best avoided except by applications responsible for actually
> implementing terminal windows."
>   - How would you write a unit test for tcsetwinsize? Also, so much
> of its effects is unspecified.
> 
> Terminal stuff is generally a portability pain — see tests/test-openpty.c.
> Therefore I would limit myself to the functionality that applications
> actually need.

Good points. The tcgetwinsize seems more useful to me anyways.
Inetutils has its own functions which do the same thing for wrapping
and such. They appear to be from 4.4BSDLite2 so likely written 30+ years
ago.

>> Windows should be able to support them too [2].
> 
> This needs thought first, because where on the Unix side you have
> two concepts:
>   - winsize from the OS,
>   - lines and columns from the terminfo description of $TERM,
> and the LINES and COLUMNS environment variables,
> on the Windows side you have only the Consoles.
> 
> How are these concepts delimited (on the Unix side)?
> 
> How do these concepts map to each other?

I've used GetConsoleScreenBufferInfo once in the past and it worked
fine in Wine atleast [1]. I don't use Windows so perhaps it wouldn't
work there.

Here is an example of how you would write a function to get the height
of the console.

int
term_get_height (void)
{
  HANDLE console_output;
  CONSOLE_SCREEN_BUFFER_INFO buffer_info;
  console_output = GetStdHandle (STD_OUTPUT_HANDLE);
  if (console_output != INVALID_HANDLE_VALUE)
{
  if (GetConsoleScreenBufferInfo (console_output, _info))
return (int) buffer_info.dwSize.Y;
}
  return -1;
}

I guess in Gnulib you probably have to deal with the invalid parameter
handler there.

Collin

[1] https://learn.microsoft.com/en-us/windows/console/getconsolescreenbufferinfo

RFC: Adding tcgetwinsize and tcsetwinsize.

[Collin Funk]

Hi,

POSIX 2024 added these two functions in termios.h, tcgetwinsize and
tcsetwinsize. They get and set the terminal size respectively.

As far as I can tell only NetBSD has them implemented [1]. They seem
pretty useful and are easy to implement. I've attached two files that
should work on Linux and BSDs (and any other system with ioctl,
TIOCGWINSZ, TIOCSWINSZ).

Any thoughts on adding them? Windows should be able to support them
too [2]. I just need to define 'struct winsize' in termios.h.

Collin

[1] https://man.netbsd.org/tcgetwinsize.3
[2] https://learn.microsoft.com/en-us/windows/console/console-functions/* tcsetwinsize.c -- get the size of a terminal window.

   Copyright 2024 Free Software Foundation, Inc.

   This file is free software: you can redistribute it and/or modify
   it under the terms of the GNU Lesser General Public License as
   published by the Free Software Foundation; either version 2.1 of the
   License, or (at your option) any later version.

   This file is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public License
   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */

/* Written by Collin Funk.  */

#include 

/* Specification.  */
#include 

#include 

int
tcsetwinsize (int fildes, const struct winsize *winsize_p)
{
  return ioctl (fildes, TIOCSWINSZ, winsize_p);
}

/* tcgetwinsize.c -- get the size of a terminal window.

   Copyright 2024 Free Software Foundation, Inc.

   This file is free software: you can redistribute it and/or modify
   it under the terms of the GNU Lesser General Public License as
   published by the Free Software Foundation; either version 2.1 of the
   License, or (at your option) any later version.

   This file is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU Lesser General Public License for more details.

   You should have received a copy of the GNU Lesser General Public License
   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */

/* Written by Collin Funk.  */

#include 

/* Specification.  */
#include 

#include 

int
tcgetwinsize (int fildes, struct winsize *winsize_p)
{
  return ioctl (fildes, TIOCGWINSZ, winsize_p);
}

What is the history behind the check-module script?

[Collin Funk]

I never looked at the check-module script until now. Was it supposed
to be used as an automatic check to be run before committing or
something?

I noticed that it doesn't handle conditional dependencies like this:

Dependencies:
stat[test $NEED_STAT_PLEASE = 1]

This diff should fix that. I haven't committed it since I'm not sure if
this script is still relevant.

$ git diff .
diff --git a/check-module b/check-module
index c86e0569f0..30e23b4710 100755
--- a/check-module
+++ b/check-module
@@ -96,6 +96,7 @@ sub parse_module_file ($)
}
  elsif ($state eq ST_DEPENDENTS)
{
+  $line =~ s/\s*\[[^\]]*]$//;
  $dep_set{$line} = 1;
  (my $base = $module_file) =~ s,.*/,,;
  $line eq $base

Here is it what it looks like when run:

$ cd modules
$ ../check-module *
lib/c32is-impl.h: lc-charset-unicode.h is '#include'd, but not listed in 
module's Files: section
lib/mbrtoc32.c: lc-charset-unicode.h is '#include'd, but not listed in 
module's Files: section
check-module: can't open '# Just to guarantee consistency between ftell() 
and ftello().' for reading: No such file or directory

It seems that ftello is the only file with a comment in 'Depends-on:'.

Collin

Re: uchar-c23: Speed up mbrtoc32 on Solaris 11.4

[Collin Funk]

On 5/25/24 3:15 AM, Bruno Haible wrote:
> The Solaris 11.4 test run on GitHub is taking 4 hours, whereas it's only
> 1 hour (at most) on the other platforms.
> 
> In order to investigate this, I attached a time stamp to each output line:
> 
>   $make 2>&1 | gawk '{ print strftime("%H:%M:%S"), $0; fflush(); }' | tee log2
>   $make check 2>&1 | gawk '{ print strftime("%H:%M:%S"), $0; fflush(); }' | 
> tee log3

Good catch, thanks. I'm not sure how long this would have taken to get
caught without CI.

Collin

byteswap tests: Verify header can be used from C++.

[Collin Funk]

I've applied this patch adding a C++ test for the byteswap.h
substitute. I guess there wasn't a need before since it only defined
macros, but now it defines functions.

CollinFrom 8658d279727e9e54d01d7ae38a9c5170893d3507 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 25 May 2024 01:20:01 -0700
Subject: [PATCH] byteswap tests: Verify header can be used from C++.

* modules/byteswap-c++-tests: New file.
* tests/test-byteswap-c++.cc: New file.
* modules/byteswap-tests (Depends-on): Add byteswap-c++-tests.
---
 ChangeLog  |  7 +++
 modules/byteswap-c++-tests | 17 +
 modules/byteswap-tests |  1 +
 tests/test-byteswap-c++.cc | 27 +++
 4 files changed, 52 insertions(+)
 create mode 100644 modules/byteswap-c++-tests
 create mode 100644 tests/test-byteswap-c++.cc

diff --git a/ChangeLog b/ChangeLog
index 0a6def457d..52d51ba5a7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-05-25  Collin Funk  
+
+	byteswap tests: Verify header can be used from C++.
+	* modules/byteswap-c++-tests: New file.
+	* tests/test-byteswap-c++.cc: New file.
+	* modules/byteswap-tests (Depends-on): Add byteswap-c++-tests.
+
 2024-05-24  Collin Funk  
 
 	readutmp: Fix dependencies.
diff --git a/modules/byteswap-c++-tests b/modules/byteswap-c++-tests
new file mode 100644
index 00..05cdc94875
--- /dev/null
+++ b/modules/byteswap-c++-tests
@@ -0,0 +1,17 @@
+Files:
+tests/test-byteswap-c++.cc
+
+Status:
+c++-test
+
+Depends-on:
+ansi-c++-opt
+
+configure.ac:
+
+Makefile.am:
+if ANSICXX
+TESTS += test-byteswap-c++
+check_PROGRAMS += test-byteswap-c++
+test_byteswap_c___SOURCES = test-byteswap-c++.cc
+endif
diff --git a/modules/byteswap-tests b/modules/byteswap-tests
index 64baf0e404..e148c5f7f3 100644
--- a/modules/byteswap-tests
+++ b/modules/byteswap-tests
@@ -4,6 +4,7 @@ tests/macros.h
 
 Depends-on:
 stdint
+byteswap-c++-tests
 
 configure.ac:
 
diff --git a/tests/test-byteswap-c++.cc b/tests/test-byteswap-c++.cc
new file mode 100644
index 00..8276840233
--- /dev/null
+++ b/tests/test-byteswap-c++.cc
@@ -0,0 +1,27 @@
+/* Test the  substitute in C++ mode.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* Written by Collin Funk , 2024.  */
+
+#define GNULIB_NAMESPACE gnulib
+#include 
+
+#include 
+
+int
+main ()
+{
+}
-- 
2.45.1

Re: Native windows boot-time

[Collin Funk]

On 5/24/24 8:56 PM, Bruno Haible wrote:
> readutmp now needs a module dependency on gettimetoday, right?

Oops, yes. Because readutmp doesn't use boot-time as a dependency,
instead it uses the boot-time-aux.h file.

I've pushed the attached patch.

CollinFrom ff1594abd5d7d4a74bc3f7f089eaf101bcab8638 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Fri, 24 May 2024 20:59:51 -0700
Subject: [PATCH] readutmp: Fix dependencies.

* modules/readutmp (Depends-on): Add gettimeofday.
---
 ChangeLog| 3 +++
 modules/readutmp | 1 +
 2 files changed, 4 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 31fd396b43..0a6def457d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2024-05-24  Collin Funk  
 
+	readutmp: Fix dependencies.
+	* modules/readutmp (Depends-on): Add gettimeofday.
+
 	boot-time, readutmp: Add a Native Windows boot time fallback.
 	* lib/boot-time-aux.h (initialize, get_windows_boot_time_fallback): New
 	functions.
diff --git a/modules/readutmp b/modules/readutmp
index 469a7c2256..0127099187 100644
--- a/modules/readutmp
+++ b/modules/readutmp
@@ -12,6 +12,7 @@ Depends-on:
 extensions
 idx
 stat-time
+gettimeofday
 stdbool
 stdint
 strnlen
-- 
2.45.1

Re: Native windows boot-time

[Collin Funk]

Hi Bruno,

On 5/24/24 3:50 PM, Bruno Haible wrote:
> - In line 393 the #endif is misindented.
> - The readutmp module is a second user of boot-time-aux.h. It should also
>   make a call to get_windows_boot_time_fallback.

Ah, now I see why those functions are in a header file. I've fixed
those in the attached patch and pushed it.

>> I've only tested it with a mingw compiler and wine so far.
> 
> That's good enough. The CI will test it on "real" Windows.

I did a bit of testing using your ci-scratch method. My original patch
didn't work on Cygwin because the GetTickCount64 () symbol couldn't
be found. I was thinking that Cygwin needed the kernel.dll anyways but
maybe there are some restrictions to using the Windows API there.

Since Cygwin passes tests without the fallback I have just restricted
it to Native Windows.

CollinFrom 4c2435dc1fc80c3740204c59c1c7b6d9adbe54d3 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Fri, 24 May 2024 19:23:25 -0700
Subject: [PATCH] boot-time, readutmp: Add a Native Windows boot time fallback.

* lib/boot-time-aux.h (initialize, get_windows_boot_time_fallback): New
functions.
* lib/boot-time.c [_WIN32 && !__CYGWIN__]: Include Windows headers and
.
(get_boot_time_uncached): Use the fallback.
* lib/readutmp.c [_WIN32 && !__CYGWIN__]: Include Windows headers and
.
(read_utmp_from_file): Use the fallback.
* modules/boot-time (Depends-on): Add gettimeofday.
---
 ChangeLog   | 13 
 lib/boot-time-aux.h | 74 +
 lib/boot-time.c | 11 +++
 lib/readutmp.c  | 23 ++
 modules/boot-time   |  1 +
 5 files changed, 122 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index d9112a810a..31fd396b43 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2024-05-24  Collin Funk  
+
+	boot-time, readutmp: Add a Native Windows boot time fallback.
+	* lib/boot-time-aux.h (initialize, get_windows_boot_time_fallback): New
+	functions.
+	* lib/boot-time.c [_WIN32 && !__CYGWIN__]: Include Windows headers and
+	.
+	(get_boot_time_uncached): Use the fallback.
+	* lib/readutmp.c [_WIN32 && !__CYGWIN__]: Include Windows headers and
+	.
+	(read_utmp_from_file): Use the fallback.
+	* modules/boot-time (Depends-on): Add gettimeofday.
+
 2024-05-24  Bruno Haible  
 
 	putenv tests: Put the putenv() argument strings into writable memory.
diff --git a/lib/boot-time-aux.h b/lib/boot-time-aux.h
index b1add30239..8b98c4e573 100644
--- a/lib/boot-time-aux.h
+++ b/lib/boot-time-aux.h
@@ -345,4 +345,78 @@ get_windows_boot_time (struct timespec *p_boot_time)
   return -1;
 }
 
+# ifndef __CYGWIN__
+#  if !(_WIN32_WINNT >= _WIN32_WINNT_VISTA)
+
+/* Don't assume that UNICODE is not defined.  */
+#   undef LoadLibrary
+#   define LoadLibrary LoadLibraryA
+
+/* Avoid warnings from gcc -Wcast-function-type.  */
+#   define GetProcAddress \
+ (void *) GetProcAddress
+
+/* GetTickCount64 is only available on Windows Vista and later.  */
+typedef ULONGLONG (WINAPI * GetTickCount64FuncType) (void);
+
+static GetTickCount64FuncType GetTickCount64Func = NULL;
+static BOOL initialized = FALSE;
+
+static void
+initialize (void)
+{
+  HMODULE kernel32 = LoadLibrary ("kernel32.dll");
+  if (kernel32 != NULL)
+{
+  GetTickCount64Func =
+(GetTickCount64FuncType) GetProcAddress (kernel32, "GetTickCount64");
+}
+  initialized = TRUE;
+}
+
+#  else
+
+#   define GetTickCount64Func GetTickCount64
+
+#  endif
+
+/* Fallback for Windows in the form:
+ boot time = current time - uptime
+   This uses the GetTickCount64 function which is only available on Windows
+   Vista and later. See:
+   <https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-gettickcount64>.  */
+static int
+get_windows_boot_time_fallback (struct timespec *p_boot_time)
+{
+#  if !(_WIN32_WINNT >= _WIN32_WINNT_VISTA)
+  if (! initialized)
+initialize ();
+#  endif
+  if (GetTickCount64Func != NULL)
+{
+  ULONGLONG uptime_ms = GetTickCount64Func ();
+  struct timespec uptime;
+  struct timespec result;
+  struct timeval tv;
+  if (gettimeofday (, NULL) >= 0)
+{
+  uptime.tv_sec = uptime_ms / 1000;
+  uptime.tv_nsec = (uptime_ms % 1000) * 100;
+  result.tv_sec = tv.tv_sec;
+  result.tv_nsec = tv.tv_usec * 1000;
+  if (result.tv_nsec < uptime.tv_nsec)
+{
+  result.tv_nsec += 10;
+  result.tv_sec -= 1;
+}
+  result.tv_sec -= uptime.tv_sec;
+  result.tv_nsec -= uptime.tv_nsec;
+  *p_boot_time = result;
+  return 0;
+}
+}
+  return -1;
+}
+
+# endif
 #endif
diff --git a/lib/boot-time.c b/lib/boot-time.c
index c1171e8024..71562dcf75 100644
--- a/lib/boot-time.c
+++ b/lib/boot-time.c
@@ -43,6 +43,13 @@
 # include 
 #endif
 
+#if defined _WIN32 && ! defined __CYG

Re: Native windows boot-time

[Collin Funk]

Hi Bruno,

On 5/24/24 4:44 AM, Bruno Haible wrote:
> The longer the API is available, the better. Gnulib supports
> Windows XP as the minimum. For modules used by Emacs ('boot-time' in
> particular) it should run even on Windows 2000.
> 
> Which means that the code needs to fetch a pointer to the particular
> Windows API function at runtime, through GetProcAddress. See lib/isatty.c
> as an example.
> 
> Would you like to give it a try?

I wrote this patch just now. Any thoughts?

I've only tested it with a mingw compiler and wine so far.

I wasn't sure the proper way to get the current time. I didn't want to
depend on timespec_get since that would bring in more dependencies for
non-Windows systems (like linking with -lrt). The resolution isn't
needed either since GetTickCount64 has a resolution of 10-16 ms [1].

I suppose a Native Windows function would do fine, but I am not
familiar with the API. I presume that is the case for most others too.
Therefore I think gettimeofday makes sense since most systems should
just have it.

[1] 
https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-gettickcount64#remarks

CollinFrom acd118c0057f206f212cb45000aa852cfb083af6 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Fri, 24 May 2024 14:07:26 -0700
Subject: [PATCH] boot-time: Add a fallback function for Windows.

* lib/boot-time-aux.h (initialize, get_windows_boot_time_fallback): New
functions.
* lib/boot-time.c [_WIN32]: Include Windows headers and sys/time.h.
(get_boot_time_uncached): Use the new Windows fallback.
* modules/boot-time (Depends-on): Add gettimeofday.
---
 ChangeLog   |  9 ++
 lib/boot-time-aux.h | 72 +
 lib/boot-time.c |  9 ++
 modules/boot-time   |  1 +
 4 files changed, 91 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index d9112a810a..b26fe146f4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-05-24  Collin Funk  
+
+	boot-time: Add a fallback function for Windows.
+	* lib/boot-time-aux.h (initialize, get_windows_boot_time_fallback): New
+	functions.
+	* lib/boot-time.c [_WIN32]: Include Windows headers and sys/time.h.
+	(get_boot_time_uncached): Use the new Windows fallback.
+	* modules/boot-time (Depends-on): Add gettimeofday.
+
 2024-05-24  Bruno Haible  
 
 	putenv tests: Put the putenv() argument strings into writable memory.
diff --git a/lib/boot-time-aux.h b/lib/boot-time-aux.h
index b1add30239..e51c58ff6f 100644
--- a/lib/boot-time-aux.h
+++ b/lib/boot-time-aux.h
@@ -345,4 +345,76 @@ get_windows_boot_time (struct timespec *p_boot_time)
   return -1;
 }
 
+# if !(_WIN32_WINNT >= _WIN32_WINNT_VISTA)
+
+/* Don't assume that UNICODE is not defined.  */
+#  undef LoadLibrary
+#  define LoadLibrary LoadLibraryA
+
+/* Avoid warnings from gcc -Wcast-function-type.  */
+#  define GetProcAddress \
+(void *) GetProcAddress
+
+/* GetTickCount64 is only available on Windows Vista and later.  */
+typedef ULONGLONG (WINAPI * GetTickCount64FuncType) (void);
+
+static GetTickCount64FuncType GetTickCount64Func = NULL;
+static BOOL initialized = FALSE;
+
+static void
+initialize (void)
+{
+  HMODULE kernel32 = LoadLibrary ("kernel32.dll");
+  if (kernel32 != NULL)
+{
+  GetTickCount64Func =
+(GetTickCount64FuncType) GetProcAddress (kernel32, "GetTickCount64");
+}
+  initialized = TRUE;
+}
+
+# else
+
+#  define GetTickCount64Func GetTickCount64
+
+# endif
+
+/* Fallback for Windows in the form:
+ boot time = current time - uptime
+   This uses the GetTickCount64 function which is only available on Windows
+   Vista and later. See:
+   <https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-gettickcount64>.  */
+static int
+get_windows_boot_time_fallback (struct timespec *p_boot_time)
+{
+# if !(_WIN32_WINNT >= _WIN32_WINNT_VISTA)
+  if (! initialized)
+initialize ();
+#endif
+  if (GetTickCount64Func != NULL)
+{
+  ULONGLONG uptime_ms = GetTickCount64Func ();
+  struct timespec uptime;
+  struct timespec result;
+  struct timeval tv;
+  if (gettimeofday (, NULL) >= 0)
+{
+  uptime.tv_sec = uptime_ms / 1000;
+  uptime.tv_nsec = (uptime_ms % 1000) * 100;
+  result.tv_sec = tv.tv_sec;
+  result.tv_nsec = tv.tv_usec * 1000;
+  if (result.tv_nsec < uptime.tv_nsec)
+{
+  result.tv_nsec += 10;
+  result.tv_sec -= 1;
+}
+  result.tv_sec -= uptime.tv_sec;
+  result.tv_nsec -= uptime.tv_nsec;
+  *p_boot_time = result;
+  return 0;
+}
+}
+  return -1;
+}
+
 #endif
diff --git a/lib/boot-time.c b/lib/boot-time.c
index c1171e8024..c0a8ea6903 100644
--- a/lib/boot-time.c
+++ b/lib/boot-time.c
@@ -43,6 +43,13 @@
 # include 
 #endif
 
+#ifdef _WIN32
+# define WIN32_LEAN_AND_MEAN
+# include 
+# include 
+# include 
+#endif

Re: Native windows boot-time

[Collin Funk]

Hi Bruno,

On 5/24/24 4:44 AM, Bruno Haible wrote:
>> Does the get_windows_boot_time () not work on Native Windows? The
>> Mingw and MSVC actions fail it.
> 
> There's apparently a difference between a full installation of
> Windows 10 and the image that they use in the Microsoft Azure cloud.

Ah, I didn't even think about that.

>> What Windows versions does Gnulib care about? Apparently this one only
>> exists on Vista and later. [1]
> 
> The longer the API is available, the better. Gnulib supports
> Windows XP as the minimum. For modules used by Emacs ('boot-time' in
> particular) it should run even on Windows 2000.
> 
> Which means that the code needs to fetch a pointer to the particular
> Windows API function at runtime, through GetProcAddress. See lib/isatty.c
> as an example.
> 
> Would you like to give it a try?

Sure, I'll have a look at it later today. Thanks for the hints.

Collin

Re: [PATCH 2/2] putenv-tests: pacify gcc -Wdiscarded-qualifiers

[Collin Funk]

On 5/24/24 3:55 AM, Bruno Haible wrote:
>> I think this change may have uncovered a GCC bug? I noticed lots of
>> -Wanalyzer-putenv-of-auto-var spam in testdirs.
> 
> It is not spam. It is fully justified, since in
>   putenv ((char []) {"TEST_VAR=abc"})
> the argument is allocated in automatic storage. See ISO C § 6.5.2.5.(12).

I meant spam as in lots of output that wasn't there previously. Not
that the warning was incorrect. Apologies for the poor wording. :)

> static char *var = "TEST_VAR=abc";
> 
> depending on the warning options enabled. Namely, it warns when
> -Wwrite-strings is enabled:
> 
>   warning: initialization discards ‘const’ qualifier from pointer target type 
> [-Wdiscarded-qualifiers]
[...]
> No. The analyzer's warning is only about strings with limited lifetime,
> it is not about strings that are not writable. That is a different
> category of warnings.

Thanks for the explanation. I understand after looking at your
previous commit.

Unless I'm compiling something that uses '--enable-gcc-warnings' or
similar, I just copy the warnings from HACKING. In there the following
is listed:

$ WARN_CFLAGS_GCC13="$WARN_GCC13 -Wnested-externs -Wshadow=local 
-Wno-discarded-qualifiers"

Which explains why I didn't see the -Wdiscarded-qualifiers when
writing that test.

Collin

Re: [PATCH 2/2] putenv-tests: pacify gcc -Wdiscarded-qualifiers

[Collin Funk]

Hi Paul,

On 5/16/24 10:42 PM, Paul Eggert wrote:
> diff --git a/tests/test-putenv.c b/tests/test-putenv.c
> index 1768e7563a..564c86713a 100644
> --- a/tests/test-putenv.c
> +++ b/tests/test-putenv.c
> @@ -39,7 +39,7 @@ main (void)
>  
>/* Verify adding an environment variable.  */
>{
> -ASSERT (putenv ("TEST_VAR=abc") == 0);
> +ASSERT (putenv ((char []) {"TEST_VAR=abc"}) == 0);

I think this change may have uncovered a GCC bug? I noticed lots of
-Wanalyzer-putenv-of-auto-var spam in testdirs.

When I checkout the commit that I added these tests and run:

$ git checkout 259dd4a0655eb9b6cd2adead0934c6ee046a2dac
$ gnulib-tool --create-testdir --dir testdir1 putenv
$ ./configure CFLAGS="-fanalyzer"
$ make

I see no warnings for that file. When I checkout your commit:

$ git checkout 15cd8edb6ec9aed2585e10456d46eec09d5c1b8b

and run the same commands I see the spam again:

==
test-putenv.c:56:13: warning: ‘putenv’ on a pointer to automatic variable 
‘’ [POS34-C] [-Wanalyzer-putenv-of-auto-var]
   56 | ASSERT (putenv ((char []) {"TEST_VAR"}) == 0);
  | ^~~
macros.h:57:13: note: in definition of macro ‘ASSERT’
   57 |   if (!(expr))  
 \
  | ^~~~
  ‘main’: event 1
|
|   57 |   if (!(expr)) 
  \
|  |  ^
|  |  |
|  |  (1) following ‘false’ branch...
==

I believe the warning should be applied in both cases. The putenv
function places the pointer that it is given into the environment.
When it goes out of scope the behavior is undefined [1].

Looking at your commit b98993a1baaa2fc39b301676ecbd8bb29e1d9c96 [2]:

diff --git a/tests/test-unsetenv.c b/tests/test-unsetenv.c
index ddc412867f..d8e5b01192 100644
--- a/tests/test-unsetenv.c
+++ b/tests/test-unsetenv.c
@@ -32,7 +32,8 @@ SIGNATURE_CHECK (unsetenv, int, (char const *));
 int
 main (void)
 {
-  char entry[] = "b=2";
+  /* Static to pacify gcc -Wanalyzer-putenv-of-auto-var.  */
+  static char entry[] = "b=2";
 
   /* Test removal when multiple entries present.  */
   ASSERT (putenv ((char *) "a=1") == 0);
   ASSERT (putenv (entry) == 0);

The warning isn't very important since 'entry' never leaves scope.
However if that causes a warning why doesn't this line:

   ASSERT (putenv ((char *) "a=1") == 0);

I'm assuming the analyzer doesn't handle the types correctly. Bug
report time for me. :)

[1] 
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
[2] 
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=b98993a1baaa2fc39b301676ecbd8bb29e1d9c96

Collin

[PATCH] putenv tests: Silence -Wanalyzer-putenv-of-auto-var.

[Collin Funk]

* tests/test-putenv.c (main): Declare static variables to pass to
putenv.
---
 ChangeLog   |  6 ++
 tests/test-putenv.c | 12 +---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e09ff2a234..bc128db28b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-23  Collin Funk  
+
+   putenv tests: Silence -Wanalyzer-putenv-of-auto-var.
+   * tests/test-putenv.c (main): Declare static variables to pass to
+   putenv.
+
 2024-05-23  Paul Eggert  
 
POSIX.1-2024 has been approved
diff --git a/tests/test-putenv.c b/tests/test-putenv.c
index 37615e3af9..b772082d20 100644
--- a/tests/test-putenv.c
+++ b/tests/test-putenv.c
@@ -37,9 +37,13 @@ main (void)
   unsetenv ("TEST_VAR");
   ASSERT (getenv ("TEST_VAR") == NULL);
 
+  /* Use static on variables passed to the environment to pacify
+ -Wanalyzer-putenv-of-auto-var.  */
+
   /* Verify adding an environment variable.  */
   {
-ASSERT (putenv ((char []) {"TEST_VAR=abc"}) == 0);
+static char *var = "TEST_VAR=abc";
+ASSERT (putenv (var) == 0);
 ptr = getenv ("TEST_VAR");
 ASSERT (ptr != NULL);
 ASSERT (STREQ (ptr, "abc"));
@@ -47,13 +51,15 @@ main (void)
 
   /* Verify removing an environment variable.  */
   {
-ASSERT (putenv ((char []) {"TEST_VAR"}) == 0);
+static char *var = "TEST_VAR";
+ASSERT (putenv (var) == 0);
 ASSERT (getenv ("TEST_VAR") == NULL);
   }
 
   /* Verify the behavior when removing a variable not in the environment.  */
   {
-ASSERT (putenv ((char []) {"TEST_VAR"}) == 0);
+static char *var = "TEST_VAR";
+ASSERT (putenv (var) == 0);
 ASSERT (getenv ("TEST_VAR") == NULL);
   }
 
-- 
2.45.1

Native windows boot-time

[Collin Funk]

Hi Bruno,

Does the get_windows_boot_time () not work on Native Windows? The
Mingw and MSVC actions fail it.

I'm thinking that it could be implemented using GetTickCount64 ().
The Windows documentation says [1]:

Retrieves the number of milliseconds that have elapsed since the
system was started.

So I'm thinking that should fix the issue.

What Windows versions does Gnulib care about? Apparently this one only
exists on Vista and later. I'm not sure how you would do it on
versions before that.


Collin

[1] 
https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-gettickcount64

Re: mbrtoc32: Work around bug in Cygwin 3.5.3

[Collin Funk]

On 5/23/24 3:01 PM, Bruno Haible wrote:
> The mbrtoc32 function, newly added in Cygwin 3.5.0, is buggy.
> 
> These two patches provide a workaround (at the cost of deactivating
> the GB18030 locale support in Cygwin), and add a two test cases.

Looks good. Thanks for all the Cygwin fixes.

Collin

Re: readlinkat, areadlinkat: Avoid test failures on Cygwin 3.4.6

[Collin Funk]

On 5/23/24 5:55 AM, Bruno Haible wrote:
>> I just noticed in the Cygwin 32 logs that c32is* tests pass while on
>> Cygwin 64 they fail.
>
> That's because the Cygwin 64 build is version 3.5.3, whereas the Cygwin 32
> build is version 3.3.6. Look at the 'cygcheck' output in the log.

Hahaha. well that would explain it, thanks. I am not very familiar
with Cygwin, sorry. I used it for the first time with gnulib-tool.py.

Collin

Re: readlinkat, areadlinkat: Avoid test failures on Cygwin 3.4.6

[Collin Funk]

Hi Bruno,

On 5/23/24 3:48 AM, Bruno Haible wrote:
> On Cygwin 3.4.6, but not on Cygwin 3.5.3, I see these test failures:
> 
> FAIL: test-areadlinkat.exe
> FAIL: test-areadlinkat-with-size.exe
> FAIL: test-readlinkat.exe
> 
> The cause is an errno value EBADF, where EINVAL would be better.
> Not worth adding a workaround. This patch avoids the failures.

Agreed, Thanks.

I just noticed in the Cygwin 32 logs that c32is* tests pass while on
Cygwin 64 they fail. Each failure looks like can be fixed by one or
two #ifdef's but I would think if one passes they both would...

Collin

Re: how to build on machines without ssh access

[Collin Funk]

Hi Bruno,

On 5/22/24 7:53 AM, Bruno Haible wrote:
> Here's a technique that allows you to build on platforms for which you don't
> have a machine without direct access.
> 
> A machine with direct access (either a VM, or a machine with ssh access [1]) 
> is
> preferred, since it's more easy to work with. But for some platforms, GitHub
> provides an alternative way.

Thanks for the guide. How much free time does GitHub give you?

I tried to look into the Cygwin failures yesterday but gave up. Slow
VM + ./configure times there were not very fun. Perhaps I am impatient...

Collin

Update end-of-life.txt.

[Collin Funk]

I've pushed this patch updating end-of-life.txt in maint-tools.git.

NetBSD released 8.3 as a final security release, major version 8 is
not longer supported [1].

OpenBSD released 7.5 and therefore 7.3 is no longer supported [2] [3].

diff --git a/end-of-life.txt b/end-of-life.txt
index ab966afd..5b7ff555 100644
--- a/end-of-life.txt
+++ b/end-of-life.txt
@@ -36,16 +36,19 @@ End-of-life / end-of-support data for various OS releases
 
 * NetBSD
   NetBSD 7.0: 2020-06-30 †
-  NetBSD 8.0:
+  NetBSD 8.0: 2024-05-04 †
   NetBSD 9.0:
+  NetBSD 10.0:
   Reference: https://en.wikipedia.org/wiki/NetBSD#Releases
  https://endoflife.date/netbsd
+ https://www.netbsd.org/releases/formal.html#history
 
 * OpenBSD
   OpenBSD 7.1: 2023-04-10 †
   OpenBSD 7.2: 2023-10-16 †
-  OpenBSD 7.3: 2024-05
+  OpenBSD 7.3: 2024-04-05 †
   OpenBSD 7.4: 2024-11
+  OpenBSD 7.5: 2025-05
   Reference: https://en.wikipedia.org/wiki/OpenBSD#Releases
  https://endoflife.date/openbsd
 
Collin

[1] https://www.netbsd.org/releases/formal-8/NetBSD-8.3.html
[2] https://www.openbsd.org/75.html
[3] https://www.openbsd.org/faq/faq5.html#Flavors

Re: getusershell: Split file by lines instead of spaces.

[Collin Funk]

Hi Bruno,

On 5/21/24 5:43 PM, Bruno Haible wrote:
> The new patch is good. OK to push.

Done, thanks.

>> Interesting way to think about it, thanks. Do you have a strong math
>> background? It has been a while since I looked at that interval
>> notation.
> 
> Oh, I learned the interval notation at school. It's really nothing fancy.

I remember seeing it in school but I don't use it enough. It was quick
to relearn using set builder notation [1]. So feel free to use it in
any future explanations. :)

> The problem with the [start,end] convention is that it cannot
> represent an empty array slice. Therefore, when you are dealing
> with a problem in which an empty array slice is a valid special
> case, [start,end] is the wrong representation.
> 
> Try to formulate a non-trivial algorithm, such as bubble sort
> or searching an item in a sorted array using binary-search.
> If you do it ad-hoc with cursors or some such, chances a high
> that you will introduce off-by-one mistakes or endless loops.
> Try it with the [start,end) convention, and you will succeed.

Good point. I'd like to think I would use [start,end) when writing
those. But better to build good habits for even simple algorithms like
this. I'll keep that in mind for the future.

I made a mess out of my /etc/shells (lots of blank lines, trailing
comments, trailing spaces, etc) and ran with Valgrind before sending
that patch. Time to clean that up now.

Collin

[1] 
https://en.wikipedia.org/wiki/Interval_(mathematics)#Including_or_excluding_endpoints

Re: getusershell: Split file by lines instead of spaces.

[Collin Funk]

On 5/21/24 5:32 AM, Bruno Haible wrote:
> * If the file ends with a non-empty line without a newline, getline()
>   returns a string that does not end in a newline.
>   Quoting 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/getline.html:
>"including the delimiter character if one was encountered before EOF".

Oops, Thanks. I'm not sure why I was thinking that the line would
*always* end in the delimiter. Good catch.

> * The following code does not work with tokens that consist of a single
>   character:
> 
>   while (start < end && isspace ((unsigned char) *end))
> 
>   if (start < end && IS_ABSOLUTE_FILE_NAME (start))
> 
>   The condition "start < end" here tests whether the token has at least
>   two characters. Which is probably not what you intended. (In this case,
>   it is not fatal, since absolute file names of non-directories always
>   consist of at least 2 characters. But anyway.)

Actually, it was intentional. Perhaps I have a strange way of thinking
of things. That plus non-standard functions leave a lot of room for
"creativity". :)

I was thinking that "/" and "c:/" (on Windows) will never refer to an
actual shell. So no point in caring about it.

However with this in my /etc/shells:


/bin/bash
/
===

When printing all shells glibc prints both lines. Mine only prints
"/bin/bash".

Strange function, in my opinion. However, lets trust the BSD and glibc
developers who wrote it over me. :) No need to change it's behavior.

>   The cause is that you are working with start and end both being
>   inclusive, that is, with a string of length end-start+1. There are 4
>   possible intervals for two variables start and length:
> [start,end]
> [start,end)
> (start,end]
> (start,end)
>   If you use sometimes [start,end], sometimes [start,end), you cannot
>   remember working idioms and you will regularly make mistakes.
>   The best way to avoid such mistakes is to work with [start,end)
>   intervals each time you work with pointers into arrays.
>   Then the condition "start < end" tests for a non-empty subsequence,
>   and you can remember and reuse idioms.

Interesting way to think about it, thanks. Do you have a strong math
background? It has been a while since I looked at that interval
notation.

Typically the way I think of things is using a "cursor", which I guess
is just a pointer to a position in a known data structure.

So in that patch we have something like this:

  startend
^   ^
|   |
   [1] [2] [3] [4] [5] [6] [7] [8]
   '/' 'b' 'i' 'n' '/' 's' 'h' '\0'

Which would be [start,end] since *start and *end are part of the
string.

I've attached the patch which I think follows your suggestion of
[start, end):

  startend
^   ^
|   |
   [1] [2] [3] [4] [5] [6] [7] [8]
   '/' 'b' 'i' 'n' '/' 's' 'h' '\0'

I guess that probably makes everything easier for others to follow. I
didn't think about it too much until you mentioned it.

Feel free to correct any misunderstandings I may have in that
explination + the patch.

CollinFrom 45911704a56dcb2fcdfade0cfb9989d1df4a40e7 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Tue, 21 May 2024 16:35:09 -0700
Subject: [PATCH] getusershell: Split file by lines instead of spaces.

* lib/getusershell.c: Include string.h and filename.h
(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
unlocked stdio functions that are no longer used.
(readname): Remove function.
(getusershell): Use getline and process the string instead of using
readname. Return the first absolute file name.
* modules/getusershell (Depends-on): Remove unlocked-io-internal.
Add getline and filename.
* doc/multithread.texi (Multithreading Optimizations): Don't mention
GNULIB_GETUSERSHELL_SINGLE_THREAD.
---
 ChangeLog| 14 
 doc/multithread.texi |  4 ---
 lib/getusershell.c   | 83 ++--
 modules/getusershell |  3 +-
 4 files changed, 57 insertions(+), 47 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 099a249271..cb2bfbdb74 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2024-05-21  Collin Funk  
+
+	getusershell: Split file by lines instead of spaces.
+	* lib/getusershell.c: Include string.h and filename.h
+	(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
+	unlocked stdio functions that are no longer used.
+	(readname): Remove function.
+	(getusershell): Use getline and process the string instead of using
+	readname. Return the first absolute file name.
+	* modules/getusershell (Depends-on): Remove unlocked-io-internal.
+	Add getline and filename.
+	* doc/multithr

Re: [PATCH] getopt-posix: port better to Alpine 3.20.0_rc1

[Collin Funk]

On 5/21/24 10:10 AM, Paul Eggert wrote:
> Sometimes I wish 'bootstrap' and 'configure' were less chatty.

+1, but I can't think of a way to do so without hiding bugs.

Usually I just redirect it to a file since it is easier to read there.

Collin

Re: OmniOS CI failure for test-fnmatch-5.sh

[Collin Funk]

Hi Bruno,

On 5/21/24 2:13 PM, Bruno Haible wrote:
>> I pushed the attached patch adding the #if defined __illumos__.
>
> It apparently has the desired effect. So, let me do the same thing with
> the test-trim3.sh test, that fails like this:
> 
> FAIL: test-trim3.sh
> ===
> 
> ../../gltests/test-trim.c:150: assertion 'strcmp (result, "\241\244foo") == 
> 0' failed

Looking into the __illumos__ macro it appears to be relatively new.
The original bug report is 3 years old [1]. The change was added to
OmniOS in version r15038s in 2021 [2].

I think it is still worth using since your CI should be using a later
version. If someone is using an older version perhaps they will see
the __illumos__ check and see it was expected to fail.

If someone reports a bug then we will know what the issue is. I think
using those macros as "documentation" is important. We can tell a test
passes on OpenSolaris derivatives but not Oracle Solaris.

I'm not sure if you would rather use something more common like __sun
though.

Collin

[1] https://www.illumos.org/issues/13726
[2] https://omnios.org/article/release-r38s

Re: access, euidaccess tests: Avoid test failure for root user on Solaris

[Collin Funk]

On 5/21/24 6:45 AM, Bruno Haible wrote:
> The CI runs the Solaris 11 and Solaris 11 OmniOS tests as root, and shows
> these two test failures:
> 
> FAIL: test-access
> FAIL: test-euidaccess
> 
> This occurs because although the file does not have the 'x' bit set,
> for root faccessat() returns 0.
> 
> POSIX 
> says "New implementations are discouraged from returning X_OK unless at least
> one execution permission bit is set." — which means that Solaris counts as
> an "old" implementation here.

Thanks for the fix. After the fixing the fnmatch test I had a brief
look at this and got confused. I didn't know they were run as root (CI
is new to me).

Collin

Re: OmniOS CI failure for test-fnmatch-5.sh

[Collin Funk]

On 5/21/24 1:39 AM, Bruno Haible wrote:
>> I was taking a look at the CI stuff you wrote. Nice work, it seems
>> really helpful.
> 
> Thanks for helping! The goal here being to get all FAILs either fixed
> or declared as XFAIL, this really helps.

Sounds like a good plan. I can help with fixing the free systems but
the proprietary ones might need more XFAIL treatment. :)

I feel like MacOS will probably be problematic. I can see that it
already caused you a lot of trouble [1].

> Since this failure is not seen on Solaris 11.4, the 'defined __illumos__'
> is correct.

Yes, I saw that the regular Solaris 11 CI passed. I figured it was
best to leave the test running there. If it ever fails in the future
we can see what version causes problems.

>> I think it might be a bug in OmniOS's handling of GB 18030 but I am
>> unsure.
> 
> On these systems, every locale has its own character classifications
> tables. It can happen that they treat U+00D7 MULTIPLICATION SIGN
> as a punctuation character in the UTF-8 locale but not in the GB18030
> locale. (Glibc avoids this by computing the character classifications
> tables for all encodings from a single source.)
> 
>> Maybe I am missing a good English specification or lack the
>> ability to learn Mandarin...
> 
> You don't need to understand Mandarin in order to work GB18030.
> GB18030 is an ASCII-based encoding of all Unicode, like UTF-8, just with
> a different mapping table.

Ah, okay. Thanks for the explanations. I think I understand what is
going on now. I had a look at GNU Iconv and the charts on Wikipedia
too. No more need to learn Mandarin.

I pushed the attached patch adding the #if defined __illumos__.

Collin

[1] https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00330.htmlFrom d7beec8902330587cda62af308d1ea629b883d7d Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Tue, 21 May 2024 02:40:14 -0700
Subject: [PATCH] fnmatch tests: Avoid test failure on OmniOS.

Using the GB18030 locale OmniOS doesn't match U+00D7 MULTIPLICATION SIGN
as a punctuation character.

* tests/test-fnmatch.c (main): Skip the test. Discovered by CI test
using OmniOS r151048 and reproduced on OmniOS r151050.
---
 ChangeLog| 8 
 tests/test-fnmatch.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 256b56580c..f1e5051559 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-21  Collin Funk  
+
+	fnmatch tests: Avoid test failure on OmniOS.
+	Using the GB18030 locale OmniOS doesn't match U+00D7 MULTIPLICATION SIGN
+	as a punctuation character.
+	* tests/test-fnmatch.c (main): Skip the test. Discovered by CI test
+	using OmniOS r151048 and reproduced on OmniOS r151050.
+
 2024-05-20  Bruno Haible  
 
 	vasnprintf: Don't abort for pseudo-denormal arguments on macOS 12.
diff --git a/tests/test-fnmatch.c b/tests/test-fnmatch.c
index 588d76646e..96e2d1f949 100644
--- a/tests/test-fnmatch.c
+++ b/tests/test-fnmatch.c
@@ -893,7 +893,7 @@ main (int argc, char *argv[])
 /* U+2  */
 ASSERT (fnmatch ("x[[:print:]]y", "x\225\062\202\066y", 0) == 0);
 #endif
-#if !(defined __FreeBSD__ || defined __DragonFly__)
+#if !(defined __FreeBSD__ || defined __DragonFly__ || defined __illumos__)
 /* U+00D7 MULTIPLICATION SIGN */
 ASSERT (fnmatch ("x[[:punct:]]y", "x\241\301y", 0) == 0);
 #endif
-- 
2.45.1

OmniOS CI failure for test-fnmatch-5.sh

[Collin Funk]

Hi Bruno,

I was taking a look at the CI stuff you wrote. Nice work, it seems
really helpful.

I saw this test failure:

FAIL: test-fnmatch-5.sh
===

../../gltests/test-fnmatch.c:898: assertion 'fnmatch ("x[[:punct:]]y", 
"x\241\301y", 0) == 0' failed
Stack trace:
0x410355 main
../../gltests/test-fnmatch.c:898
0x405036 ???
???:0
0x404f97 ???
???:0
FAIL test-fnmatch-5.sh (exit status: 1)

In an OmniOS VM the same error occurs:

$ uname -a
SunOS omnios 5.11 omnios-r151050-6f87d0b5d63 i86pc i386 i86pc

The tests pass with that one check #ifdef'd out. Here is the diff I
used:

$ diff -u tests/test-fnmatch.c testdir1/gltests/test-fnmatch.c 
--- tests/test-fnmatch.c2024-05-20 01:15:09.806829699 -0700
+++ testdir1/gltests/test-fnmatch.c 2024-05-21 00:57:41.935481891 -0700
@@ -893,7 +893,7 @@
 /* U+2  */
 ASSERT (fnmatch ("x[[:print:]]y", "x\225\062\202\066y", 0) == 0);
 #endif
-#if !(defined __FreeBSD__ || defined __DragonFly__)
+#if !(defined __FreeBSD__ || defined __DragonFly__ || defined 
__illumos__)
 /* U+00D7 MULTIPLICATION SIGN */
 ASSERT (fnmatch ("x[[:punct:]]y", "x\241\301y", 0) == 0);
 #endif

I think it might be a bug in OmniOS's handling of GB 18030 but I am
unsure. Maybe I am missing a good English specification or lack the
ability to learn Mandarin...

Here is a small test program that might help you decide what is the
best solution here:

int
main (void)
{
  wchar_t wc;
  size_t result;
  mbstate_t state;
  if (setlocale (LC_ALL, "zh_CN.GB18030") == NULL)
abort ();
  memset (, '\0', sizeof (wchar_t));
  memset (, '\0', sizeof (mbstate_t));
  result = mbrtowc (, "\241\301", 3, );
  if (result == (size_t) -1 || result == (size_t) -2)
abort ();
  printf ("%s\n", iswpunct (wc) ? "PASS" : "FAIL");
  return 0;
}

On OmniOS:

$ ./a.out 
FAIL

On GNU/Linux:

$ ./a.out 
PASS

Collin

Re: getusershell: Split file by lines instead of spaces.

[Collin Funk]

Hi Bruno,

You said:
>> #if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
>> # define ADDITIONAL_DEFAULT_SHELLS \
>>   "c:/dos/command.com", "c:/windows/command.com", "c:/command.com",
>
> Inside '#if defined __MSDOS__' or '#if defined _WIN32' this is OK.
> I was worried there was explicit code to accept such prefixes on POSIX
> compatible platforms. Fortunately not.

Sorry for the confusion. I didn't mean to suggest adding drive
prefixes on non-Windows systems hahaha. That would be an "interesting"
choice.

>> I've attached a patch following this method.
> 
> I see a method 'next_shell' that is only called in a single place. Would
> it make sense to inline it (and move its function comments into the
> body of the caller function)?

Good point. I thought it was a bit strange to pass the file stream and
buffer as arguments when they are static global variables in that
file.

I've attached a patch moving it to the end of getusershell. Makes
things easier to understand IMO. I haven't pushed it yet so let me
know what you think.

> The main change is to call getline() instead of parsing the file
> character-by-character. I guess that the motivation is that the
> behaviour of trimming leading whitespace and trimming trailing whitespace
> before '#' would lead to convoluted code with the previous approach?

Pretty much, yes. I tried to do that originally but I don't think you
would have liked the patch. :)

At the start of 'readname' there is a loop to skip spaces:

 while ((c == getc (stream)) != EOF
&& isspace ((unsigned char) c))
   /* Do nothing.  */;

Essentially in the main for (;;) loop you would have to compare every
character for '#'. If one is found then you have to do a loop similar
to that and then break afterwards.

Since the file format is for the most part "one shell per line", with
a few caveats, I think it makes sense to just use getline(). Makes
things easier to read in my opinion.

Judging by the copyright date on that file (1991), I assume getline
wasn't portable enough to use (or didn't exist?) then. If I remember
correctly it was a glibc extension that was later adopted by POSIX.

>> The patch removes 'GNULIB_GETUSERSHELL_SINGLE_THREAD' which uses
>> getc_unlocked instead of getc for optimization ...
>>
>> I don't see too much harm in it though since on most platforms getline
>> should lock much less then repeatedly calling getc.
> 
> Right. When the function uses getline instead of getc, it will lock 10x
> less than before. I agree there is not much need to optimize this.
> Especially since we don't have a getline_unlocked function. And since
> locking has become cheaper since this code was written. (Futexes were
> invented in 2002-2003.)

Interesting. I don't know very much about Futexes.

Do most libc implementations keep track of if a stream needs to be
locked? I'm thinking about single-threaded programs that don't define
GNULIB_GETUSERSHELL_SINGLE_THREAD. I imagine the stream doesn't locked
x10 more, but a status flag is checked x10 more or something.

CollinFrom 9b67292f9dd7c1b0ecfd15993ee43a99eb557b76 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 20 May 2024 21:03:39 -0700
Subject: [PATCH] getusershell: Split file by lines instead of spaces.

* lib/getusershell.c: Include string.h and filename.h
(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
unlocked stdio functions that are no longer used.
(readname): Remove function.
(getusershell): Use getline and process the string instead of using
readname. Return the first absolute file name.
* modules/getusershell (Depends-on): Remove unlocked-io-internal.
Add getline and filename.
* doc/multithread.texi (Multithreading Optimizations): Don't mention
GNULIB_GETUSERSHELL_SINGLE_THREAD.
---
 ChangeLog| 14 +
 doc/multithread.texi |  4 ---
 lib/getusershell.c   | 73 +++-
 modules/getusershell |  3 +-
 4 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 256b56580c..157041e2ea 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2024-05-20  Collin Funk  
+
+	getusershell: Split file by lines instead of spaces.
+	* lib/getusershell.c: Include string.h and filename.h
+	(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
+	unlocked stdio functions that are no longer used.
+	(readname): Remove function.
+	(getusershell): Use getline and process the string instead of using
+	readname. Return the first absolute file name.
+	* modules/getusershell (Depends-on): Remove unlocked-io-internal.
+	Add getline and filename.
+	* doc/multithread.texi (Multithreading Optimizations): Don't mention
+	GNULIB_GETUSERSHELL_SINGLE_THREAD.
+
 2024-05-20  Bruno Haible  
 
 	vasnprintf: Don't abort for pseudo-denormal argume

Re: NetBSD utimens, utimensat, etc. failures

[Collin Funk]

Hi Bruno,

On 5/20/24 3:54 AM, Bruno Haible wrote:
>> I'll leave it for review.
> 
> Looks good to me. Just please change the #if conditions to not test the
> *values* of __linux__, __sun, __NetBSD__. That is, the proper way to test
> for Linux, Solaris, NetBSD is
>   defined __linux__
>   defined __sun
>   defined __NetBSD__

Yeah, I noticed that after sending the diff. I think it shouldn't have
caused any problems since gcc defines them to 1 IIRC. But I agree
defined is better.

>> If it is okay I can push with documentation updates, xfail removal, 
>> ChangeLog.
> 
> Yes please!

Done in the two attached patches.

It looks like you added the UTIME_OMIT ctime bug to the documentation
in this commit:

  commit e6c7f8be2fe11e72c3fff2503be9ab3f798b787a
  Author: Bruno Haible 
  Date:   Sat Jul 25 23:27:40 2020 +0200

  doc: Update for NetBSD 7.1, 8.0, 9.0.

Not too sure if that was a mistake in the bulk doc update or if the
code wasn't updated. I think it should be correct now but feel free to
look into it more if you would like:

$ git diff e6c7f8be2fe11e72c3fff2503be9ab3f798b787a^ 
e6c7f8be2fe11e72c3fff2503be9ab3f798b787a doc/posix-functions/futimens.texi 
doc/posix-functions/utimensat.texi

CollinFrom 7e842de9d80ddd94abd61f529702b2e27f31c175 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 20 May 2024 14:23:41 -0700
Subject: [PATCH 1/2] utimensat, utimens: Work around NetBSD 10.0 bugs.

* lib/utimens.c (fdutimens): Work around a NetBSD 10.0 UTIME_OMIT bug in
the same way as Linux kernel 2.6.32 and Solaris 11.1.
(lutimens): Likewise.
* lib/utimensat.c (rpl_utimensat): Likewise. Workaround a NetBSD 10.0
bug where invalid tv_nsec values are not rejected in the same way as
Linux kernel 2.6.22.19 on hppa.
* doc/posix-functions/utimensat.texi: Document the invalid tv_nsec bug.
---
 ChangeLog  | 11 +++
 doc/posix-functions/utimensat.texi |  2 +-
 lib/utimens.c  |  8 +---
 lib/utimensat.c| 11 +++
 4 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 88258bec2c..5dd7fe7e69 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2024-05-20  Collin Funk  
+
+	utimensat, utimens: Work around NetBSD 10.0 bugs.
+	* lib/utimens.c (fdutimens): Work around a NetBSD 10.0 UTIME_OMIT bug in
+	the same way as Linux kernel 2.6.32 and Solaris 11.1.
+	(lutimens): Likewise.
+	* lib/utimensat.c (rpl_utimensat): Likewise. Workaround a NetBSD 10.0
+	bug where invalid tv_nsec values are not rejected in the same way as
+	Linux kernel 2.6.22.19 on hppa.
+	* doc/posix-functions/utimensat.texi: Document the invalid tv_nsec bug.
+
 2024-05-20  Paul Eggert  
 
 	byteswap: fix problem on macOS
diff --git a/doc/posix-functions/utimensat.texi b/doc/posix-functions/utimensat.texi
index d7b4f6914f..1ba560fb0a 100644
--- a/doc/posix-functions/utimensat.texi
+++ b/doc/posix-functions/utimensat.texi
@@ -35,7 +35,7 @@ @node utimensat
 @item
 Out-of-range values of @code{tv_nsec} do not lead to a failure on some
 platforms:
-Linux kernel 2.6.22.19 on hppa.
+Linux kernel 2.6.22.19 on hppa, NetBSD 10.0.
 @item
 On some platforms, this function mis-handles a trailing slash:
 AIX 7.2.
diff --git a/lib/utimens.c b/lib/utimens.c
index 4bfb9c91a7..6b9f62a53c 100644
--- a/lib/utimens.c
+++ b/lib/utimens.c
@@ -220,7 +220,7 @@ fdutimens (int fd, char const *file, struct timespec const timespec[2])
   if (0 <= utimensat_works_really)
 {
   int result;
-# if __linux__ || __sun
+# if defined __linux__ || defined __sun || defined __NetBSD__
   /* As recently as Linux kernel 2.6.32 (Dec 2009), several file
  systems (xfs, ntfs-3g) have bugs with a single UTIME_OMIT,
  but work if both times are either explicitly specified or
@@ -230,6 +230,7 @@ fdutimens (int fd, char const *file, struct timespec const timespec[2])
  where UTIME_OMIT would have worked.
 
  The same bug occurs in Solaris 11.1 (Apr 2013).
+ The same bug occurs in NetBSD 10.0 (May 2024).
 
  FIXME: Simplify this in 2024, when these file system bugs are
  no longer common on Gnulib target platforms.  */
@@ -440,7 +441,7 @@ fdutimens (int fd, char const *file, struct timespec const timespec[2])
 #  endif
 if (futimes (fd, t) == 0)
   {
-#  if __linux__ && __GLIBC__
+#  if defined __linux__ && defined __GLIBC__
 /* Work around a longstanding glibc bug, still present as
of 2010-12-27.  On older Linux kernels that lack both
utimensat and utimes, glibc's futimes rounds instead of
@@ -553,7 +554,7 @@ lutimens (char const *file, struct timespec const timespec[2])
   if (0 <= lutimensat_works_really)
 {
   int result;
-# if __linux__ || __sun
+# if defined __linux__ || defined __sun || defined __NetBSD__
   /* As recently as Linux kernel 2.6.32 (Dec 2009), several file

Re: byteswap side-effect defect report from Coverity

[Collin Funk]

Hi Bruno,

On 5/20/24 12:40 PM, Bruno Haible wrote:
>> Interesting. I just learned what a Coverity scan is. Do I have to have
>> permission to view the defects?
> 
> I think one needs permission to view and classify these defects, yes.
> But it's more boring than anything else, since more than 90% are false
> alarms. So, if you don't mind, it's sufficient if Paul and I view and
> classify these defects.

I see. That is fine with me. I can see that a lot of them are
"CWE-676: Use of Potentially Dangerous Function", which seems more
annoying then helpful. I imagine it is just a bunch of 
functions that are mostly fine.

> If you really want to do something boring, you could review
> 'gcc -fanalyzer' reports (which is something Paul and I occasionally
> do as well) or 'clang -fanalyzer' reports (which neither of us has done
> so far, AFAIK).

I use 'gcc -fanalyzer' occasionally. I wasn't aware that clang
supported it too.

Collin

Re: byteswap side-effect defect report from Coverity

[Collin Funk]

Hi Paul,

On 5/20/24 9:13 AM, Paul Eggert wrote:
> Gnulib does not support glibc 2.1.x and older, so this should not be a 
> problem when porting to glibc. However, I worried that other platforms might 
> have the bug, until I noticed that m4/byteswap.m4 already inadvertently tests 
> for it. I installed the attached patch to document the test.

I was worried about other systems too. After you mentioned that macros
don't work on double arguments correctly I used it in byteswap.m4
since it seemed like a nice compile-time check. I probably should have
commented it though, thanks.

> We can ignore this Coverity defect report as a false positive, just as we 
> ignore many other defect reports.

Interesting. I just learned what a Coverity scan is. Do I have to have
permission to view the defects?

Collin

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

Hi Bruno,

On 5/20/24 3:28 AM, Bruno Haible wrote:
>> It looks like the current code wants drive-prefixes accepted, i.e.
>> 'c:/ugly/windows/stuff'.
> 
> ?? I don't see such code in gnulib/lib/getusershell.c.

It is defined in the 'ADDITIONAL_DEFAULT_SHELLS' macro [1]:


#if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
# define ADDITIONAL_DEFAULT_SHELLS \
  "c:/dos/command.com", "c:/windows/command.com", "c:/command.com",
#else
# define ADDITIONAL_DEFAULT_SHELLS /* empty */
#endif

/* List of shells to use if the shells file is missing. */
static char const* const default_shells[] =
{
  ADDITIONAL_DEFAULT_SHELLS
  "/bin/sh", "/bin/csh", "/usr/bin/sh", "/usr/bin/csh", NULL
};


>> That sort of breaks the behavior of glibc and BSD where:
>>
>>  input -> getusershell () output
>>  'bin/bash' -> '/bash'
> 
> ?? The code in gnulib/lib/getusershell.c does not test for a '/'.

I probably explained it poorly, sorry. Perhaps the important section
of code in glibc will help explain [2].

I was trying to point out that glibc and BSD will only return absolute
file names. Since they do not target Windows that means just looking
for a '/'.

Here is a test program:

=
#define _GNU_SOURCE 1
#include 
#include 
#include 
int
main (void)
{
  char *p;
  while ((p = getusershell ()))
printf ("%s\n", p);
  return 0;
}
=

My /etc/shells:

=

this-part-gets-removed/this-doesn't/bash
/bin/sh
=

And then the program output:

$ uname -o
GNU/Linux
$ ./a.out 
/this-doesn't/bash
/bin/sh

The file format isn't very well defined, in my opinion [3]. I think
the common assumption is that administrators put sane absolute file
names one-per-line. Like this:

/bin/bash
/bin/sh
/bin/ksh
/usr/bin/csh

The other patch I sent would behave similarly enough to glibc and BSD
in my opinion while allowing for drive prefixes on Windows and spaces
since I think they are common there.

The removal of the first non-absolute part is strange in my opinion.

Collin

[1] https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/getusershell.c#n48
[2] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/getusershell.c;h=4221095dca743dfa5067637f1aad4651bd5cf279;hb=2be3352f0b1ebaa39596393fffe1062275186669#l130
[3] 
https://man.freebsd.org/cgi/man.cgi?query=shells=5=0=FreeBSD+14.0-RELEASE+and+Ports

Re: NetBSD dup3

[Collin Funk]

Hi Bruno,

On 5/18/24 5:03 PM, Bruno Haible wrote:
> And it's a good idea to reference the bug report in a comment:
> @c https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58266
> So that in the future it helps us to understand what the bug is
> about and how to reproduce it in newer versions of NetBSD.

The bug report got addressed today. In the following commit [1]:

  PR/58266: Collin Funk: Fail if from == to, like FreeBSD and Linux. The test
  is done in dup3 before any other tests so even if a bad descriptor it is
  passed we will return EINVAL not EBADFD like Linux does.

and then a second to deal with syscall versioning [2].

Looking into their comment about EINVAL vs EBADF, they follow what
FreeBSD and OpenBSD do [3][4]. I think we may have the ordering wrong
in lib/dup3.c:

  if (newfd < 0 || newfd >= getdtablesize () || fcntl (oldfd, F_GETFD) == -1)
{
  errno = EBADF;
  return -1;
}

  if (newfd == oldfd)
{
  errno = EINVAL;
  return -1;
}

In the Linux kernel sources they are done in this order [5]:

if ((flags & ~O_CLOEXEC) != 0)
  return -EINVAL;

if (unlikely(oldfd == newfd))
  return -EINVAL;

if (newfd >= rlimit(RLIMIT_NOFILE))
   return -EBADF;

Unless glibc wraps the syscall in it's own functions where the order
is changed. I'm not too sure how that stuff works but I couldn't seem
to find it.

Collin

[1] 
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/sys_descrip.c.diff?r1=1.48=1.49_with_tag=MAIN
[2] 
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/sys_descrip.c.diff?r1=1.49=1.50_with_tag=MAIN
[3] https://github.com/freebsd/freebsd-src/blob/main/lib/libc/gen/dup3.c
[4] 
https://github.com/openbsd/src/blob/8b68e9dedc5b1a2ef9fae4d11f703c6d7be32352/sys/kern/kern_descrip.c#L328
[5] 
https://github.com/torvalds/linux/blob/eb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c/fs/file.c#L1354

getusershell: Split file by lines instead of spaces.

[Collin Funk]

On 5/19/24 7:48 PM, Collin Funk wrote:
> It looks like the current code wants drive-prefixes accepted, i.e.
> 'c:/ugly/windows/stuff'.
> 
> That sort of breaks the behavior of glibc and BSD where:
> 
>  input -> getusershell () output
>  'bin/bash' -> '/bash'
> 
> Since it seems silly to accept some drive prefix in the middle of a
> string.
> 
> I can't imagine anyone putting non-absolute file names in that file.
> So I'll probably restrict it to one absolute file name per-line and
> then trim any leading whitespace plus comments. Perhaps getline and
> filename.h macros will help there.

I've attached a patch following this method.

It should behave the same as BSD and glibc except for the fact that it
allows whitespace in file names. I guess you could scan the line
before returning on non-Windows systems, but I'm not sure it is worth
caring about. Only the root user can edit /etc/shells and programs
should check it actually exists anyways.

The patch removes 'GNULIB_GETUSERSHELL_SINGLE_THREAD' which uses
getc_unlocked instead of getc for optimization so I will wait for
review before pushing.

I don't see too much harm in it though since on most platforms getline
should lock much less then repeatedly calling getc. But I figured it
would be rude not to ask first. :)

CollinFrom 192fa52c6efcc290fecb12c00fd69bd9f57c4b49 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sun, 19 May 2024 21:40:25 -0700
Subject: [PATCH] getusershell: Split file by lines instead of spaces.

* lib/getusershell.c: Include string.h and filename.h.
(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
unlocked stdio functions that are no longer used.
(readname): Remove function.
(next_shell): New function, based on readname.
(getusershell): Use next_shell instead of readname.
* doc/multithread.texi (Multithreading Optimizations): Don't mention
GNULIB_GETUSERSHELL_SINGLE_THREAD.
---
 ChangeLog| 12 
 doc/multithread.texi |  4 ---
 lib/getusershell.c   | 69 
 modules/getusershell |  3 +-
 4 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 89c5aa6993..ffe772bf2c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2024-05-19  Collin Funk  
+
+	getusershell: Split file by lines instead of spaces.
+	* lib/getusershell.c: Include string.h and filename.h.
+	(GNULIB_GETUSERSHELL_SINGLE_THREAD): Remove conditional to include
+	unlocked stdio functions that are no longer used.
+	(readname): Remove function.
+	(next_shell): New function, based on readname.
+	(getusershell): Use next_shell instead of readname.
+	* doc/multithread.texi (Multithreading Optimizations): Don't mention
+	GNULIB_GETUSERSHELL_SINGLE_THREAD.
+
 2024-05-19  Collin Funk  
 
 	getusershell: Work around musl bugs.
diff --git a/doc/multithread.texi b/doc/multithread.texi
index 7d8126e8f3..424e1d2a75 100644
--- a/doc/multithread.texi
+++ b/doc/multithread.texi
@@ -293,10 +293,6 @@ @node Multithreading Optimizations
 You can get this macro defined by including the Gnulib module
 @code{wchar-single}.
 @item
-You may define the C macro @code{GNULIB_GETUSERSHELL_SINGLE_THREAD}, if all the
-programs in your package invoke the functions @code{setusershell},
-@code{getusershell}, @code{endusershell} only from a single thread.
-@item
 You may define the C macro @code{GNULIB_EXCLUDE_SINGLE_THREAD}, if all the
 programs in your package invoke the functions of the @code{exclude} module
 only from a single thread.
diff --git a/lib/getusershell.c b/lib/getusershell.c
index 4da5bff37f..a146dbfe89 100644
--- a/lib/getusershell.c
+++ b/lib/getusershell.c
@@ -34,16 +34,14 @@
 #endif
 
 #include 
+#include 
 #include 
 
 #include "stdio--.h"
+#include "filename.h"
 #include "xalloc.h"
 
-#if GNULIB_GETUSERSHELL_SINGLE_THREAD
-# include "unlocked-io.h"
-#endif
-
-static idx_t readname (char **, idx_t *, FILE *);
+static char *next_shell (char **, size_t *, FILE *);
 
 #if ! defined ADDITIONAL_DEFAULT_SHELLS && defined __MSDOS__
 # define ADDITIONAL_DEFAULT_SHELLS \
@@ -70,7 +68,7 @@ static FILE *shellstream = NULL;
 static char *line = NULL;
 
 /* Number of bytes allocated for 'line'. */
-static idx_t line_size = 0;
+static size_t line_size = 0;
 
 /* Return an entry from the shells file, ignoring comment lines.
If the file doesn't exist, use the list in DEFAULT_SHELLS (above).
@@ -99,12 +97,7 @@ getusershell (void)
 }
 }
 
-  while (readname (, _size, shellstream))
-{
-  if (*line != '#')
-return line;
-}
-  return NULL;  /* End of file. */
+  return next_shell (, _size, shellstream);
 }
 
 /* Rewind the shells file. */
@@ -129,35 +122,47 @@ endusershell (void)
 }
 }
 
-/* Read a line from STREAM, removing any newline at the end.
+/* Read lines from STREAM, removing leading and trailing
+   whitespace and com

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

On 5/19/24 7:17 PM, Bruno Haible wrote:
> You don't need an Alpine Linux machine to debug this. You can debug it
> on a glibc system like this:
>   1. create a testdir for the module getusershell,
>   2. configure through
>ac_cv_func_getusershell=no ./configure
>   3. use the unit test, or compile a test program with -Itestdir/gllib
>  -Itestdir and link it with testdir/gllib/libgnu.a.

Ah, I forgot about this thanks.

It looks like the current code wants drive-prefixes accepted, i.e.
'c:/ugly/windows/stuff'.

That sort of breaks the behavior of glibc and BSD where:

 input -> getusershell () output
 'bin/bash' -> '/bash'

Since it seems silly to accept some drive prefix in the middle of a
string.

I can't imagine anyone putting non-absolute file names in that file.
So I'll probably restrict it to one absolute file name per-line and
then trim any leading whitespace plus comments. Perhaps getline and
filename.h macros will help there.

Collin

Re: [PATCH] getusershell: Work around musl bugs.

[Collin Funk]

Hi Bruno,

On 5/19/24 5:03 PM, Bruno Haible wrote:
> Perfect. Thanks a lot!

Sadly, it looks like this uncovered that the gnulib version is buggy
too.

Given this etc/shells:


# valid login shells

/bin/sh  # abc
/bin/ash # 123 
/bin/bash  # def



$ ./gltests/test-getusershell 
valid
login
shells
/bin/sh
abc
/bin/ash
123
/bin/bash
def
abc
/bin/sh
abc

I'll have a look at fixing that now.

Collin

[PATCH] getusershell: Work around musl bugs.

[Collin Funk]

Reported by Bruno Haible in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00261.html>.

* doc/glibc-functions/getusershell.texi: Mention the musl bug.
* lib/unistd.in.h (getusershell, setusershell, endusershell): Allow the
functions to be declared with the rpl_ prefix.
* m4/getusershell.m4 (gl_FUNC_GETUSERSHELL): Prepare functions to be
replaced on musl systems.
(gl_PREREQ_GETUSERSHELL): New macro.
* m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Initialize
REPLACE_GETUSERSHELL.
* modules/getusershell (Depends-on): Update module conditions to account
for the function being available but replaced by Gnulib.
(configure.ac): Likewise. Invoke gl_PREREQ_GETUSERSHELL.
---
 ChangeLog | 17 
 doc/glibc-functions/getusershell.texi |  5 
 lib/unistd.in.h   | 39 ++-
 m4/getusershell.m4| 11 +++-
 m4/unistd_h.m4|  3 ++-
 modules/getusershell  | 12 ++---
 modules/unistd|  1 +
 7 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1e8cc94054..89c5aa6993 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2024-05-19  Collin Funk  
+
+   getusershell: Work around musl bugs.
+   Reported by Bruno Haible in
+   <https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00261.html>.
+   * doc/glibc-functions/getusershell.texi: Mention the musl bug.
+   * lib/unistd.in.h (getusershell, setusershell, endusershell): Allow the
+   functions to be declared with the rpl_ prefix.
+   * m4/getusershell.m4 (gl_FUNC_GETUSERSHELL): Prepare functions to be
+   replaced on musl systems.
+   (gl_PREREQ_GETUSERSHELL): New macro.
+   * m4/unistd_h.m4 (gl_UNISTD_H_DEFAULTS): Initialize
+   REPLACE_GETUSERSHELL.
+   * modules/getusershell (Depends-on): Update module conditions to account
+   for the function being available but replaced by Gnulib.
+   (configure.ac): Likewise. Invoke gl_PREREQ_GETUSERSHELL.
+
 2024-05-18  Bruno Haible  
 
abort-debug: Prefer libbacktrace to execinfo.
diff --git a/doc/glibc-functions/getusershell.texi 
b/doc/glibc-functions/getusershell.texi
index 83b6d4971e..379e6d893f 100644
--- a/doc/glibc-functions/getusershell.texi
+++ b/doc/glibc-functions/getusershell.texi
@@ -14,6 +14,11 @@ @node getusershell
 @item
 This function is missing a declaration on some platforms:
 Solaris 9.
+
+@item
+This function mistakenly returns comments and empty lines on some platforms:
+@c https://www.openwall.com/lists/musl/2024/05/18/1
+musl libc 1.2.4
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/lib/unistd.in.h b/lib/unistd.in.h
index 7dbed38969..e01629af25 100644
--- a/lib/unistd.in.h
+++ b/lib/unistd.in.h
@@ -1531,12 +1531,21 @@ _GL_CXXALIASWARN (getpid);
 
 
 #if @GNULIB_GETUSERSHELL@
+# if @REPLACE_GETUSERSHELL@
 /* Return the next valid login shell on the system, or NULL when the end of
the list has been reached.  */
-# if !@HAVE_DECL_GETUSERSHELL@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#undef getusershell
+#define getusershell rpl_getusershell
+#  endif
+_GL_FUNCDECL_RPL (getusershell, char *, (void));
+_GL_CXXALIAS_RPL (getusershell, char *, (void));
+# else
+#  if !@HAVE_DECL_GETUSERSHELL@
 _GL_FUNCDECL_SYS (getusershell, char *, (void));
-# endif
+#  endif
 _GL_CXXALIAS_SYS (getusershell, char *, (void));
+# endif
 _GL_CXXALIASWARN (getusershell);
 #elif defined GNULIB_POSIXCHECK
 # undef getusershell
@@ -1548,10 +1557,19 @@ _GL_WARN_ON_USE (getusershell, "getusershell is 
unportable - "
 
 #if @GNULIB_GETUSERSHELL@
 /* Rewind to pointer that is advanced at each getusershell() call.  */
-# if !@HAVE_DECL_GETUSERSHELL@
+# if @REPLACE_GETUSERSHELL@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#undef setusershell
+#define setusershell rpl_setusershell
+#  endif
+_GL_FUNCDECL_RPL (setusershell, void, (void));
+_GL_CXXALIAS_RPL (setusershell, void, (void));
+# else
+#  if !@HAVE_DECL_GETUSERSHELL@
 _GL_FUNCDECL_SYS (setusershell, void, (void));
-# endif
+#  endif
 _GL_CXXALIAS_SYS (setusershell, void, (void));
+# endif
 _GL_CXXALIASWARN (setusershell);
 #elif defined GNULIB_POSIXCHECK
 # undef setusershell
@@ -1564,10 +1582,19 @@ _GL_WARN_ON_USE (setusershell, "setusershell is 
unportable - "
 #if @GNULIB_GETUSERSHELL@
 /* Free the pointer that is advanced at each getusershell() call and
associated resources.  */
-# if !@HAVE_DECL_GETUSERSHELL@
+# if @REPLACE_GETUSERSHELL@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#undef endusershell
+#define endusershell rpl_endusershell
+#  endif
+_GL_FUNCDECL_RPL (endusershell, void, (void));
+_GL_CXXALIAS_RPL (endusershell, void, (void));
+# else
+#  if !@HAVE_DECL_GETUSERSHELL@
 _GL_FUNCDECL_SYS (endusershell, void, (void));
-# endi

Re: NetBSD utimens, utimensat, etc. failures

[Collin Funk]

On 5/19/24 2:29 AM, Collin Funk wrote:
> With that those two test cases seem to fail for the same reason as the
> others:
> 
> $ ./gltests/test-fdutimensat 
> test-utimens.h:149: assertion 'ctime_compare (, ) < 0' failed
> $ ./gltests/test-utimensat   
> test-utimens.h:149: assertion 'ctime_compare (, ) < 0' failed
> 
> It looks like the behavior is different when one argument has
> tv_nsec == 0 and the other has tv_nsec == UTIME_OMIT. But I am not
> very familiar with these functions.

This patch fixes all the failures in my NetBSD virtual machine.

Essentially, NetBSD 10.0 has the same issues as Linux 2.6 hppa had for
this code.

I'll leave it for review. If it is okay I can push with documentation
updates, xfail removal, ChangeLog.

diff --git a/lib/utimens.c b/lib/utimens.c
index 4bfb9c91a7..6168f36cf2 100644
--- a/lib/utimens.c
+++ b/lib/utimens.c
@@ -220,7 +220,7 @@ fdutimens (int fd, char const *file, struct timespec const 
timespec[2])
   if (0 <= utimensat_works_really)
 {
   int result;
-# if __linux__ || __sun
+# if __linux__ || __sun || __NetBSD__
   /* As recently as Linux kernel 2.6.32 (Dec 2009), several file
  systems (xfs, ntfs-3g) have bugs with a single UTIME_OMIT,
  but work if both times are either explicitly specified or
@@ -230,6 +230,7 @@ fdutimens (int fd, char const *file, struct timespec const 
timespec[2])
  where UTIME_OMIT would have worked.
 
  The same bug occurs in Solaris 11.1 (Apr 2013).
+ The same bug occurs in NetBSD 10.0 (May 2024).
 
  FIXME: Simplify this in 2024, when these file system bugs are
  no longer common on Gnulib target platforms.  */
@@ -553,7 +554,7 @@ lutimens (char const *file, struct timespec const 
timespec[2])
   if (0 <= lutimensat_works_really)
 {
   int result;
-# if __linux__ || __sun
+# if __linux__ || __sun || __NetBSD__
   /* As recently as Linux kernel 2.6.32 (Dec 2009), several file
  systems (xfs, ntfs-3g) have bugs with a single UTIME_OMIT,
  but work if both times are either explicitly specified or
@@ -563,6 +564,7 @@ lutimens (char const *file, struct timespec const 
timespec[2])
  UTIME_OMIT would have worked.
 
  The same bug occurs in Solaris 11.1 (Apr 2013).
+ The same bug occurs in NetBSD 10.0 (May 2024).
 
  FIXME: Simplify this for Linux in 2016 and for Solaris in
  2024, when file system bugs are no longer common.  */
diff --git a/lib/utimensat.c b/lib/utimensat.c
index 1321264269..b44207b4be 100644
--- a/lib/utimensat.c
+++ b/lib/utimensat.c
@@ -77,7 +77,7 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
int flag)
 #  undef utimensat
 {
-#  if defined __linux__ || defined __sun
+#  if defined __linux__ || defined __sun || defined __NetBSD__
   struct timespec ts[2];
 #  endif
 
@@ -86,7 +86,7 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
   if (0 <= utimensat_works_really)
 {
   int result;
-#  if defined __linux__ || defined __sun
+#  if defined __linux__ || defined __sun || defined __NetBSD__
   struct stat st;
   /* As recently as Linux kernel 2.6.32 (Dec 2009), several file
  systems (xfs, ntfs-3g) have bugs with a single UTIME_OMIT,
@@ -97,6 +97,7 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
  UTIME_OMIT would have worked.
 
  The same bug occurs in Solaris 11.1 (Apr 2013).
+ The same bug occurs in NetBSD 10.0 (May 2024).
 
  FIXME: Simplify this in 2024, when these file system bugs are
  no longer common on Gnulib target platforms.  */
@@ -117,9 +118,11 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
 ts[1] = times[1];
   times = ts;
 }
-#   ifdef __hppa__
+#   if defined __hppa__ || defined __NetBSD__
   /* Linux kernel 2.6.22.19 on hppa does not reject invalid tv_nsec
- values.  */
+ values.
+
+ The same bug occurs in NetBSD 10.0 (May 2024).  */
   else if (times
&& ((times[0].tv_nsec != UTIME_NOW
 && ! (0 <= times[0].tv_nsec

Collin

NetBSD utimens, utimensat, etc. failures

[Collin Funk]

Hi Bruno,

On your NetBSD test failures you had these:

> FAIL: test-fdutimensat
> ==
>
> ../../gltests/test-utimens.h:75: assertion 'func (BASE "file", ts) == -1' 
> failed
> FAIL test-fdutimensat (exit status: 134)
>
> FAIL: test-futimens
> ===
>
> ../../gltests/test-futimens.h:170: assertion 'ctime_compare (, ) < 0' 
> failed
> FAIL test-futimens (exit status: 134)
>
> FAIL: test-utimens
> ==
>
> ../../gltests/test-utimens.h:149: assertion 'ctime_compare (, ) < 0' 
> failed
> FAIL test-utimens (exit status: 134)
>
> FAIL: test-utimensat
> 
>
> ../../gltests/test-utimens.h:75: assertion 'func (BASE "file", ts) == -1' 
> failed
> FAIL test-utimensat (exit status: 134)

I started looking into it. The two test cases 'test-utimensat' and
'test-fdutimensat' both fail in test-utimens.h on line 75.

It appears that NetBSD doesn't properly check the tv_nsec like a few
other platforms. This diff seems to be the correct way of handling it:

$ git diff master
diff --git a/lib/utimensat.c b/lib/utimensat.c
index 1321264269..ed7b2c22fc 100644
--- a/lib/utimensat.c
+++ b/lib/utimensat.c
@@ -133,8 +133,9 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
 }
 #   endif
 #  endif
-#  if defined __APPLE__ && defined __MACH__
-  /* macOS 10.13 does not reject invalid tv_nsec values either.  */
+#  if (defined __APPLE__ && defined __MACH__) || defined __NetBSD__
+  /* macOS 10.13 and NetBSD 10.0 do not reject invalid tv_nsec values
+ either.  */
   if (times
   && ((times[0].tv_nsec != UTIME_OMIT
&& times[0].tv_nsec != UTIME_NOW
@@ -148,6 +149,8 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
   errno = EINVAL;
   return -1;
 }
+  /* macOS 10.13 does not handle trailing slashes correctly.  */
+#   ifndef __NetBSD__
   size_t len = strlen (file);
   if (len > 0 && file[len - 1] == '/')
 {
@@ -160,6 +163,7 @@ rpl_utimensat (int fd, char const *file, struct timespec 
const times[2],
   return -1;
 }
 }
+#   endif
 #  endif
   result = utimensat (fd, file, times, flag);
   /* Linux kernel 2.6.25 has a bug where it returns EINVAL for


With that those two test cases seem to fail for the same reason as the
others:

$ ./gltests/test-fdutimensat 
test-utimens.h:149: assertion 'ctime_compare (, ) < 0' failed
$ ./gltests/test-utimensat   
test-utimens.h:149: assertion 'ctime_compare (, ) < 0' failed

It looks like the behavior is different when one argument has
tv_nsec == 0 and the other has tv_nsec == UTIME_OMIT. But I am not
very familiar with these functions.

Collin

Re: getusershell tests: Fail if empty lines are returned.

[Collin Funk]

Hi Bruno,

On 5/18/24 12:16 AM, Bruno Haible wrote:
>> I'll submit a bug report on the musl lists now.
> 
> Thanks.

I submitted a report yesterday but forgot to link here [1].

>> I don't think this function is used too much
> 
> Probably no one noticed the bug before, because the typical use of
> this function is to test whether a given shell string is valid,
> as in [1], and for this use, the bug hardly matters.

True. I think most programs care about the current users default shell
(struct passwd.pw_shell) anyways, not the systems available shells.

I still think that the Musl behavior is unexpected enough that it
warrants overriding though.

I don't see a way to check the behavior of getusershell () in
configure checks though. Typically /etc/shells can only be written by
root and a administrator can change it at any time. IIRC most
distributions use a hook to edit it upon installing (or uninstalling)
a shell package. Who knows if some add empty lines or comments in the
process.

What do you think about just replacing it on all Musl systems? I can
write a patch if that solution seems reasonable to you.

Collin

[1] https://www.openwall.com/lists/musl/2024/05/18/1

Re: [PATCH] sha512-buffer: port back to 32-bit-only hosts

[Collin Funk]

Hi Paul,

Patch looks good.

On 5/18/24 7:04 PM, Paul Eggert wrote:
> Port to platforms lacking 64-bit integers (something that Emacs
> still attempts to do, in theory) by adding an u64bswap primitive
> to u64.h and using that, instead of using bswap_64.  This fixes a
> bug I made in commit 0d45ec7c033c165ad73a6509c7fa84aa67edf4ea
> dated Sun Jun 17 14:35:37 2018 -0700.

Out of personal interest, do you happen to know what platforms Emacs
builds on that lack 64-bit types?

My first computer was 32-bit x86 and I have never run into a place
they are missing.

To be honest I never looked into how GCC implements 64-bit integers on
i386. I'm guessing two 32-bit operations with one involving the carry
flag? I guess that is what the u64.h functions are doing but the
compiler knows the instructions. :)

I think for i386 that is add, adc for addition and sub, sbb for
subtraction but I am a bit rusty.

Collin

dup3: Update documentation and expected test results.

[Collin Funk]

Hi Bruno,

On 5/18/24 5:03 PM, Bruno Haible wrote:
>> Until then, maybe this should be added to the documentation? You
>> tested NetBSD 9.0 and NetBSD 10.0 and I reproduced on NetBSD 10.0.
>> From what I can tell from a brief look at the git history (don't know
>> how to use cvs, so github mirror) the behavior hasn't changed since it
>> was introduced in NetBSD 6.0.
> 
> For our doc's purposes, the moment when the bug has been introduced in
> NetBSD is not relevant. What we document is the newest version in which
> we could reproduce the bug. Please document it!

Ah, okay that makes sense. I misunderstood the version numbers. I was
under the impression that they were when the bug was introduced.

I guess that would be a full-time job itself to maintain. And there
isn't much benefit to documenting every bug in EOL operating systems
anyways.

> And it's a good idea to reference the bug report in a comment:
> @c https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58266
> So that in the future it helps us to understand what the bug is
> about and how to reproduce it in newer versions of NetBSD.
> 
> Also, you can now revert the latest commit of modules/dup3-tests.

Added documentation and removed the xfail stuff in the attached patch.
Very small so I figured a single commit is fine. On my NetBSD 10.0 VM
running:

  $ gnulib-tool --create-testdir --dir testdir1 dup3
  $ cd testdir1
  $ make check

and everything passes.

> Thanks again!

No worries. Thank you for explaining all the doc/*, lib/*, and test/*
conventions to me. Now that gnulib-tool.py is working, outside of a
few minor bugs (e.g. gl_DOC_BASE issue in gnulib-tool.py.TODO), I can
help investigate bugs and make improvements elsewhere too.

CollinFrom c7398c508d7e0222c1d7a74c14d93b23bf57fe05 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 18 May 2024 17:34:09 -0700
Subject: [PATCH] dup3: Update documentation and expected test results.

* doc/glibc-functions/dup3.texi: Mention NetBSD bug fixed by the Gnulib
implementation after the previous commit.
* modules/dup3-tests (Depends-on): Remove test-xfail.
(Makefile.am): Don't expect test-dup3 to fail on NetBSD.
---
 ChangeLog | 6 ++
 doc/glibc-functions/dup3.texi | 5 +
 modules/dup3-tests| 5 -
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 71a111ee7d..fc2c42283c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2024-05-18  Collin Funk  
 
+	dup3: Update documentation and expected test results.
+	* doc/glibc-functions/dup3.texi: Mention NetBSD bug fixed by the Gnulib
+	implementation after the previous commit.
+	* modules/dup3-tests (Depends-on): Remove test-xfail.
+	(Makefile.am): Don't expect test-dup3 to fail on NetBSD.
+
 	dup3: Fix behavior for equal file descriptors on NetBSD.
 	* lib/dup3.c (dup3) [__NetBSD__]: Check for equal file descriptors upon
 	a successful call to dup3. If they are equal fail with errno == EINVAL.
diff --git a/doc/glibc-functions/dup3.texi b/doc/glibc-functions/dup3.texi
index dfe47b4725..c04e6bd7a4 100644
--- a/doc/glibc-functions/dup3.texi
+++ b/doc/glibc-functions/dup3.texi
@@ -15,6 +15,11 @@ @node dup3
 @item
 This function can crash on some platforms:
 Cygwin 1.7.25.
+
+@item
+This function mistakenly succeeds when given two equal file descriptors on some platforms:
+@c https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58266
+NetBSD 10.0.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/modules/dup3-tests b/modules/dup3-tests
index eff2e21307..7d52771730 100644
--- a/modules/dup3-tests
+++ b/modules/dup3-tests
@@ -8,14 +8,9 @@ getdtablesize
 msvc-nothrow
 open
 close
-test-xfail
 
 configure.ac:
 
 Makefile.am:
 TESTS += test-dup3
 check_PROGRAMS += test-dup3
-
-if OS_IS_NETBSD
-XFAIL_TESTS += test-dup3
-endif
-- 
2.45.1

Re: NetBSD dup3

[Collin Funk]

Hi Bruno,

On 5/18/24 4:54 AM, Bruno Haible wrote:
>> +/* On NetBSD dup3 is a no-op when oldfd == newfd, but we expect
>> +   an error with errno == EINVAL.  */
> 
> (What is "we", and who expects something from whom?)
> I would find it less confusing if written like this:
> 
>/* On NetBSD dup3 is a no-op when oldfd == newfd, but we are
>   expected to return -1 with errno == EINVAL.  */
> or
>/* On NetBSD dup3 is a no-op when oldfd == newfd, but we are
>   expected to fail with error EINVAL.  */

Yeah, that comment isn't very well-written now that I read it again.
I've pushed the patch with a ChangeLog + the second of your
suggestions.

>> Should I report this to the NetBSD people? I guess it isn't
>> standardized so they are free to do as they wish.
> 
> Ideally every bug that we find in any *BSD should be reported. It's
> mostly a question of time, and of likelihood that they fix it.
> 
> In this case, the argument that "look, FreeBSD and OpenBSD do it right [1][2]"
> is stronger than the argument "you do it differently than glibc, and glibc
> is right" that we could use to argue w.r.t. other bugs.

Haha, I assume the "glibc does it right, you do it wrong, be like
glibc" argument has been used unsucessfully before?

I've submitted a bug report with the links to documentation and a test
program for them [1]. I think I even found the right place to change
the behavior, so hopefully someone takes a look at it soon.

Until then, maybe this should be added to the documentation? You
tested NetBSD 9.0 and NetBSD 10.0 and I reproduced on NetBSD 10.0.
>From what I can tell from a brief look at the git history (don't know
how to use cvs, so github mirror) the behavior hasn't changed since it
was introduced in NetBSD 6.0.

Collin

[1] https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58266
[2] https://man.netbsd.org/dup3.2

Re: endian: New module.

[Collin Funk]

On 5/18/24 7:45 AM, Bruno Haible wrote:
>> Can you check the attached patch? I think that it should work or
>> at least be in the correct direction...
> 
> In the endian.in.h file: The comments on the 3 last lines are not
> consistent with the '#endif' scopes.

Oops, good catch.

> In the Makefile.am snippet:
> A few lines are indented with spaces, not with a tab. It would
> not cause an error here, since these are only continuation lines,
> I think. But nevertheless better be consistent with how other
> modules do it.

In my Emacs configuration I have a hook that makes makefile-mode use
tabs instead of spaces. For nearly all other modes I use spaces. In
the case of modules/* they are interpreted as text files, so the tab
key inserts spaces.

Perhaps I'll figure out a good way to deal with that if/when it starts
annoying me. Until then I can just insert regular tabs.

> Other than that, it looks correct.

I've applied this patch. Same as before but with those two things
fixed.

CollinFrom 83dd4db866cc5dde2fddcf1944f3b5cc3732e48e Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 18 May 2024 06:36:55 -0700
Subject: [PATCH] endian: Make sure system headers can be included.

Reported by Bruno Haible in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00290.html>.

* lib/endian.in.h (be16toh, be32toh, be64toh, htobe16, htobe32, htobe64)
(le16toh, le32toh, le64toh, htole16, htole32, htole64): Don't define
functions if the system has working versions.
* m4/endian_h.m4 (gl_ENDIAN_H): Separate checks for stdint types and
proper macro/function definitions.
* modules/endian (Depends-on): Add include_next. Update module
dependency conditions.
(Makefile.am): Perform sed replacements on the header substitute.
---
 ChangeLog   | 14 ++
 lib/endian.in.h | 46 +---
 m4/endian_h.m4  | 70 +++--
 modules/endian  | 15 ---
 4 files changed, 120 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 7de9b90f6d..3ab2dc021a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2024-05-18  Collin Funk  
+
+	endian: Make sure system headers can be included.
+	Reported by Bruno Haible in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00290.html>.
+	* lib/endian.in.h (be16toh, be32toh, be64toh, htobe16, htobe32, htobe64)
+	(le16toh, le32toh, le64toh, htole16, htole32, htole64): Don't define
+	functions if the system has working versions.
+	* m4/endian_h.m4 (gl_ENDIAN_H): Separate checks for stdint types and
+	proper macro/function definitions.
+	* modules/endian (Depends-on): Add include_next. Update module
+	dependency conditions.
+	(Makefile.am): Perform sed replacements on the header substitute.
+
 2024-05-18  Bruno Haible  
 
 	abort-debug: Integrate with CONTINUE_AFTER_ASSERT.
diff --git a/lib/endian.in.h b/lib/endian.in.h
index 5d755fd7cf..bd65ae8aab 100644
--- a/lib/endian.in.h
+++ b/lib/endian.in.h
@@ -17,8 +17,30 @@
 
 /* Written by Collin Funk.  */
 
-#ifndef _GL_ENDIAN_H
-#define _GL_ENDIAN_H 1
+#ifndef _@GUARD_PREFIX@_ENDIAN_H
+
+#if __GNUC__ >= 3
+@PRAGMA_SYSTEM_HEADER@
+#endif
+@PRAGMA_COLUMNS@
+
+#if @HAVE_ENDIAN_H@
+
+/* The include_next requires a split double-inclusion guard.  */
+# @INCLUDE_NEXT@ @NEXT_ENDIAN_H@
+
+#endif
+
+
+/* glibc defines all macros and functions but is missing types from
+   stdint.h.  */
+#if @ENDIAN_H_JUST_MISSING_STDINT@
+# include 
+#else
+
+/* Others platforms.  */
+#ifndef _@GUARD_PREFIX@_ENDIAN_H
+#define _@GUARD_PREFIX@_ENDIAN_H 1
 
 /* This file uses _GL_INLINE, WORDS_BIGENDIAN.  */
 #if !_GL_CONFIG_H_INCLUDED
@@ -47,6 +69,22 @@ _GL_INLINE_HEADER_BEGIN
 # define BYTE_ORDER LITTLE_ENDIAN
 #endif
 
+/* Make sure function-like macros get undefined.  */
+#if @HAVE_ENDIAN_H@
+# undef be16toh
+# undef be32toh
+# undef be64toh
+# undef htobe16
+# undef htobe32
+# undef htobe64
+# undef le16toh
+# undef le32toh
+# undef le64toh
+# undef htole16
+# undef htole32
+# undef htole64
+#endif
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -193,4 +231,6 @@ htole64 (uint64_t x)
 
 _GL_INLINE_HEADER_END
 
-#endif /* _GL_ENDIAN_H */
+#endif /* @ENDIAN_H_JUST_MISSING_STDINT@ */
+#endif /* _@GUARD_PREFIX@_ENDIAN_H */
+#endif /* _@GUARD_PREFIX@_ENDIAN_H */
diff --git a/m4/endian_h.m4 b/m4/endian_h.m4
index ec0d111ae3..29dab603e3 100644
--- a/m4/endian_h.m4
+++ b/m4/endian_h.m4
@@ -1,5 +1,5 @@
 # endian_h.m4
-# serial 1
+# serial 2
 dnl Copyright 2024 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -12,8 +12,25 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
   AC_REQUIRE([gl_BIGENDIAN])
 
   AC_CHECK_HEADERS_ONCE([endian.h])
+  gl_CHECK_NEXT_HEADERS([endian.h])
   if test $ac_cv_header_endian_h = yes; then
-AC_CACHE_CHECK([if endian.h conforms to POSIX],
+HAVE_ENDIAN_H=1
+dnl Check if endian.h defines ui

Re: endian: New module.

[Collin Funk]

On 5/18/24 4:46 AM, Bruno Haible wrote:
>> I see. What is the correct solution here?
> 
> See the idioms used by, say, arpa_inet.in.h.
> 
>> Or something like this:
>>
>> #if @HAVE_ENDIAN_H@
>> # @INCLUDE_NEXT@ @NEXT_ENDIAN_H@
>> #endif
> 
> This is part of it. But there's more details needed.

Can you check the attached patch? I think that it should work or
at least be in the correct direction...

On my system in a testdir with all modules I don't get the error, so
it is a bit hard to test. :(

I've split the configure checks into one checking for uint16_t, etc.
and the other checking for macros/function definitions. The reason for
this is that glibc defines macros/functions properly, so we just need
the stdint.h types. I assume this might occur on other systems but
that is just a guess.

For other platforms the configure checks might fail because
function-like macros are used. In that case we can include endian.h
from the system and #undef the macros before our own definition.

Also, double checking that I understand the #include_next idioms would
be appreciated.

Feel free to push this patch if it is okay. I don't want this module
to mess with your CI jobs.

Collin From 670b8be73f4b79d4a6993dafcd12ae81721a85b6 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 18 May 2024 06:36:55 -0700
Subject: [PATCH] endian: Make sure system headers can be included.

Reported by Bruno Haible in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00290.html>.

* lib/endian.in.h (be16toh, be32toh, be64toh, htobe16, htobe32, htobe64)
(le16toh, le32toh, le64toh, htole16, htole32, htole64): Don't define
functions if the system has working versions.
* m4/endian_h.m4 (gl_ENDIAN_H): Separate checks for stdint types and
proper macro/function definitions.
* modules/endian (Depends-on): Add include_next. Update module
dependency conditions.
(Makefile.am): Perform sed replacements on the header substitute.
---
 ChangeLog   | 14 ++
 lib/endian.in.h | 46 +---
 m4/endian_h.m4  | 70 +++--
 modules/endian  | 15 ---
 4 files changed, 120 insertions(+), 25 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 29adf56ab7..90953430be 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2024-05-18  Collin Funk  
+
+	endian: Make sure system headers can be included.
+	Reported by Bruno Haible in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00290.html>.
+	* lib/endian.in.h (be16toh, be32toh, be64toh, htobe16, htobe32, htobe64)
+	(le16toh, le32toh, le64toh, htole16, htole32, htole64): Don't define
+	functions if the system has working versions.
+	* m4/endian_h.m4 (gl_ENDIAN_H): Separate checks for stdint types and
+	proper macro/function definitions.
+	* modules/endian (Depends-on): Add include_next. Update module
+	dependency conditions.
+	(Makefile.am): Perform sed replacements on the header substitute.
+
 2024-05-18  Bruno Haible  
 
 	endian tests: Verify that it can be used from C++.
diff --git a/lib/endian.in.h b/lib/endian.in.h
index 5d755fd7cf..65473201ea 100644
--- a/lib/endian.in.h
+++ b/lib/endian.in.h
@@ -17,8 +17,30 @@
 
 /* Written by Collin Funk.  */
 
-#ifndef _GL_ENDIAN_H
-#define _GL_ENDIAN_H 1
+#ifndef _@GUARD_PREFIX@_ENDIAN_H
+
+#if __GNUC__ >= 3
+@PRAGMA_SYSTEM_HEADER@
+#endif
+@PRAGMA_COLUMNS@
+
+#if @HAVE_ENDIAN_H@
+
+/* The include_next requires a split double-inclusion guard.  */
+# @INCLUDE_NEXT@ @NEXT_ENDIAN_H@
+
+#endif
+
+
+/* glibc defines all macros and functions but is missing types from
+   stdint.h.  */
+#if @ENDIAN_H_JUST_MISSING_STDINT@
+# include 
+#else
+
+/* Others platforms.  */
+#ifndef _@GUARD_PREFIX@_ENDIAN_H
+#define _@GUARD_PREFIX@_ENDIAN_H 1
 
 /* This file uses _GL_INLINE, WORDS_BIGENDIAN.  */
 #if !_GL_CONFIG_H_INCLUDED
@@ -47,6 +69,22 @@ _GL_INLINE_HEADER_BEGIN
 # define BYTE_ORDER LITTLE_ENDIAN
 #endif
 
+/* Make sure function-like macros get undefined.  */
+#if @HAVE_ENDIAN_H@
+# undef be16toh
+# undef be32toh
+# undef be64toh
+# undef htobe16
+# undef htobe32
+# undef htobe64
+# undef le16toh
+# undef le32toh
+# undef le64toh
+# undef htole16
+# undef htole32
+# undef htole64
+#endif
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -193,4 +231,6 @@ htole64 (uint64_t x)
 
 _GL_INLINE_HEADER_END
 
-#endif /* _GL_ENDIAN_H */
+#endif /* _@GUARD_PREFIX@_ENDIAN_H */
+#endif /* _@GUARD_PREFIX@_ENDIAN_H */
+#endif /* @ENDIAN_H_JUST_MISSING_STDINT@ */
diff --git a/m4/endian_h.m4 b/m4/endian_h.m4
index ec0d111ae3..29dab603e3 100644
--- a/m4/endian_h.m4
+++ b/m4/endian_h.m4
@@ -1,5 +1,5 @@
 # endian_h.m4
-# serial 1
+# serial 2
 dnl Copyright 2024 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -12,8 +12,25 @@ AC_DEFUN_ONCE([gl_ENDIAN_H]
   AC_REQUIRE([gl_BIGENDIAN])
 
   AC_CHECK_HEADERS_ONCE([endia

Re: endian: New module.

[Collin Funk]

Hi Bruno,

On 5/18/24 4:12 AM, Bruno Haible wrote:
> While testing a testdir for module 'endian' on gcc110.fsffrance.org
> (in the GCC compilefarm), I get a compilation error. This machine has
> a pretty old libc (glibc 2.17), but that ought to work anyway.

Oops, sorry.

I sent a request for a compile farm account today since I realized I
have no way of testing other architectures (other then VMs I guess).

> In other words: When overriding a system header, you must conditionally
> #include_next that system header. (There are exception to this rule,
> such as  or . But in general this rule is valid.)
> The system header can have made any number of additional definitions
> or declarations, and other system headers (and user code) may rely on
> these definitions or declarations.

I see. What is the correct solution here? I guess you could define
__MACROS but that is a bit hacky in my opinion.

Maybe defining a macro for ALMOST_WORKING_ENDIAN_H (i.e. missing types).

Or something like this:

#if @HAVE_ENDIAN_H@
# @INCLUDE_NEXT@ @NEXT_ENDIAN_H@
#endif

/* Undefine macros.  */
#undef betoh16
#undef betoh32
#undef betoh64
[]

/* Override them if they are functions.  */
#define betoh16 __gl_betoh16
#define betoh32 __gl_betoh32
#define betoh64 __gl_betoh64

I haven't ever used the '#include_next' directive so I am just basing
this off of what I have seen in Gnulib.

Collin

Re: NetBSD dup3

[Collin Funk]

Hi Bruno,

On 5/18/24 3:42 AM, Bruno Haible wrote:
> An interesting approach. But I think this added code comes too early:
> In case of oldfd == newfd && newfd < 0, it would fail with EINVAL instead
> of EBADF.
> 
> How about trying the system call first, and test for newfd == oldfd only if
> that system call succeeds?

Ah, I think you are right. The ordering is important. It looks like
FreeBSD and OpenBSD behave as expected [1] [2]. Therefore I think it
is best to #ifdef it for NetBSD. No need to perform the check elsewhere.

How does this look? Passes tests on NetBSD 10.00.

$ git diff .
diff --git a/lib/dup3.c b/lib/dup3.c
index a810d3be19..8c4e983df1 100644
--- a/lib/dup3.c
+++ b/lib/dup3.c
@@ -45,6 +45,15 @@ dup3 (int oldfd, int newfd, int flags)
 if (!(result < 0 && errno == ENOSYS))
   {
 have_dup3_really = 1;
+/* On NetBSD dup3 is a no-op when oldfd == newfd, but we expect
+   an error with errno == EINVAL.  */
+# ifdef __NetBSD__
+if (newfd == oldfd)
+  {
+errno = EINVAL;
+return -1;
+  }
+# endif
 # if REPLACE_FCHDIR
 if (0 <= result)
   result = _gl_register_dup (oldfd, newfd);

Should I report this to the NetBSD people? I guess it isn't
standardized so they are free to do as they wish. But it seems strange
to behave differently then everyone else...

Collin

[1] https://man.freebsd.org/cgi/man.cgi?query=dup3=3
[2] https://man.openbsd.org/dup.2

Re: tests: Mark tests that fail on NetBSD as expected failures

[Collin Funk]

Hi Bruno,

On 5/17/24 2:10 PM, Bruno Haible wrote:
> On NetBSD 9.0 and 10.0, I always see the same tests fail:
> 
> FAIL: test-dup3
> ===
> 
> ../../gltests/test-dup3.c:113: assertion 'dup3 (fd, fd, o_flags) == -1' failed
> FAIL test-dup3 (exit status: 134)

It looks like dup3 behaves differently on NetBSD. It is documented
correctly, but I am not sure the reasoning behind the change.

For NetBSD the return value [1]:

 These calls return the new file descriptor value.  In the case of dup2()
 and dup3() this is always the same as newfd.  If an error occurs, the
 value -1 is returned and errno is set to indicate what happened.

For GNU/Linux [2]:

If oldfd equals newfd, then dup3() fails with the error
EINVAL.

Currently dup3 is replaced unconditionally. I made this change in a
NetBSD virtual machine and the test program passes:

$ git diff .
diff --git a/lib/dup3.c b/lib/dup3.c
index a810d3be19..7674f042a4 100644
--- a/lib/dup3.c
+++ b/lib/dup3.c
@@ -34,6 +34,15 @@ dup3 (int oldfd, int newfd, int flags)
   /* Avoid a cygwin crasher. */
   setdtablesize (newfd + 1);
 # endif
+
+#ifdef __NetBSD__
+  if (newfd == oldfd)
+{
+  errno = EINVAL;
+  return -1;
+}
+#endif
+
   /* Try the system call first, if it exists.  (We may be running with a glibc
  that has the function but with an older kernel that lacks it.)  */
   {

Sort of messy but does the trick. I haven't applied it in case you
have any better ideas.

Collin

[1] https://man.netbsd.org/dup3.2
[2] https://man7.org/linux/man-pages/man2/dup3.2.html

endian: New module.

[Collin Funk]

I've pushed the two patches adding a module to substitute endian.h.
Pretty much the same as last time except using inline functions
instead of macros and making sure variables are used in the m4 file.

I've only mentioned the module in the documentation. It might make
sense to put it in posix-headers/* once the next POSIX revision is
actually released. Since now it is just a draft and is subject to
change.

Right now the configure test probably fails on all platforms. POSIX
requires it to define uint16_t, uint32_t, and uint64_t but glibc
doesn't. I've submitted a bug report for that [1].

Also, the next POSIX revision seems like it is going to require
int64_t and uint64_t support [2]. I've left the 64-bit functions
#ifdef'd out though. I figured that was better since the module can be
used as a dependency or in programs who still support old systems
(assuming the 64-bit versions don't get used of course). I'm not sure
if it is worth splitting things into separate modules just for that
though.

Also I ran the test cases on GCC, Clang, and Oracle CC, all x86-64. I
don't have access to any other architectures. Someone running them on
a big endian system would be very much appreciated, to catch any typos
I may have made there. :)

Collin

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=31749
[2] https://austingroupbugs.net/view.php?id=1799From 95f7085c87236bea297f9251ecb58e0bd821552c Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Sat, 18 May 2024 00:10:33 -0700
Subject: [PATCH 1/2] endian: New module.

* doc/glibc-headers/endian.texi, doc/gnulib-tool.texi: Mention it.
* lib/endian.c: New file.
* lib/endian.in.h: New file.
* m4/endian_h.m4: New file.
* modules/endian: New file.
---
 ChangeLog |   9 ++
 doc/glibc-headers/endian.texi |   8 +-
 doc/gnulib-tool.texi  |   1 +
 lib/endian.c  |  23 
 lib/endian.in.h   | 196 ++
 m4/endian_h.m4|  71 
 modules/endian|  44 
 7 files changed, 348 insertions(+), 4 deletions(-)
 create mode 100644 lib/endian.c
 create mode 100644 lib/endian.in.h
 create mode 100644 m4/endian_h.m4
 create mode 100644 modules/endian

diff --git a/ChangeLog b/ChangeLog
index 8ddc85b138..4c2d9b516d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-05-18  Collin Funk  
+
+	endian: New module.
+	* doc/glibc-headers/endian.texi, doc/gnulib-tool.texi: Mention it.
+	* lib/endian.c: New file.
+	* lib/endian.in.h: New file.
+	* m4/endian_h.m4: New file.
+	* modules/endian: New file.
+
 2024-05-17  Bruno Haible  
 
 	tests: Fix link errors (regression today).
diff --git a/doc/glibc-headers/endian.texi b/doc/glibc-headers/endian.texi
index c45b875501..5c7cb72429 100644
--- a/doc/glibc-headers/endian.texi
+++ b/doc/glibc-headers/endian.texi
@@ -5,15 +5,15 @@ @node endian.h
 Defines the macros @code{BYTE_ORDER}, @code{LITTLE_ENDIAN}, @code{BIG_ENDIAN},
 @code{PDP_ENDIAN}.
 
-Gnulib module: ---
+Gnulib module: endian
 
 Portability problems fixed by Gnulib:
 @itemize
+@item
+This header file is missing on some platforms:
+macOS 11.1, FreeBSD 13.0, NetBSD 7.1, OpenBSD 3.8, Minix 3.1.8, AIX 5.1, HP-UX 11, Solaris 11.4, mingw, MSVC 14.
 @end itemize
 
 Portability problems not fixed by Gnulib:
 @itemize
-@item
-This header file is missing on some platforms:
-macOS 11.1, FreeBSD 13.0, NetBSD 7.1, OpenBSD 3.8, Minix 3.1.8, AIX 5.1, HP-UX 11, Solaris 11.4, mingw, MSVC 14.
 @end itemize
diff --git a/doc/gnulib-tool.texi b/doc/gnulib-tool.texi
index 5a2fd19cac..116734f4d7 100644
--- a/doc/gnulib-tool.texi
+++ b/doc/gnulib-tool.texi
@@ -561,6 +561,7 @@ @node Style of #include statements
 @item @code{assert.h}
 @item @code{ctype.h}
 @item @code{dirent.h}
+@item @code{endian.h}
 @item @code{errno.h}
 @item @code{fcntl.h}
 @item @code{fenv.h}
diff --git a/lib/endian.c b/lib/endian.c
new file mode 100644
index 00..3e7e56f523
--- /dev/null
+++ b/lib/endian.c
@@ -0,0 +1,23 @@
+/* Inline functions for .
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This file is free software: you can redistribute it and/or modify
+   it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   This file is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* Written by Collin Funk.  */
+
+#include 
+
+#define _GL_ENDIAN_INLINE _GL_EXTERN_INLINE
+#include 
diff --git a/lib/endian.in.h b/lib/endian.in.h
new file mode 100644
index 00..5d755fd7cf
--- /dev/null
+++ b/lib/endia

getusershell tests: Fail if empty lines are returned.

[Collin Funk]

Hi Bruno,

I setup an Alpine VM and looked into the getusershell () failure you
reported earlier.

On alpine the default /etc/shells:

# valid login shells
/bin/sh
/bin/ash
/bin/bash

Fails because of the comment as I explained earlier. When adding a few
blank lines before the comment and an assertion:

$ ./gltests/test-getusershell 
test-getusershell.c:55: assertion 'ptr[0] != '\0'' failed
Aborted

This differs from Glibc's behavior where empty lines are ignored. It
looks like Glibc is lightly modified from 4.4BSD's sources since
FreeBSD's code looks similar in that area. I've pushed the attached
patch checking for empty lines since all the other BSDs likely pass it
too.

I'll submit a bug report on the musl lists now. I don't think this
function is used too much, but it seems sort of bad that a default
/etc/shells starts with a comment which is then returned upon the
first call...

CollinFrom f77eb82cec43e043762c7e746375bc1879aa4849 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Fri, 17 May 2024 18:53:51 -0700
Subject: [PATCH] getusershell tests: Fail if empty lines are returned.

* tests/test-getusershell.c (first_pass): Check the result of malloc.
Make sure '\0' isn't returned from getusershell when there is an empty
line in /etc/shells.
---
 ChangeLog | 7 +++
 tests/test-getusershell.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 888ab7fdd7..e1a840eb04 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-05-17  Collin Funk  
+
+	getusershell tests: Fail if empty lines are returned.
+	* tests/test-getusershell.c (first_pass): Check the result of malloc.
+	Make sure '\0' isn't returned from getusershell when there is an empty
+	line in /etc/shells.
+
 2024-05-17  Bruno Haible  
 
 	unistd: Fix compilation error with MSVC in C++ mode.
diff --git a/tests/test-getusershell.c b/tests/test-getusershell.c
index f009e8902a..ea9d7ac6ed 100644
--- a/tests/test-getusershell.c
+++ b/tests/test-getusershell.c
@@ -46,11 +46,13 @@ first_pass (void)
   /* Avoid reallocation.  */
   shell_count = 16;
   shells = malloc (shell_count * sizeof (char *));
+  ASSERT (shells != NULL);
 
   for (; (ptr = getusershell ()); ++i)
 {
-  /* Make sure comments are ignored.  */
+  /* Make sure comments and empty lines are ignored.  */
   ASSERT (ptr[0] != '#');
+  ASSERT (ptr[0] != '\0');
   if (i >= shell_count)
 {
   shell_count *= 2;
-- 
2.45.1

Re: [PATCH] byteswap: port better to limited platforms

[Collin Funk]

On 5/17/24 3:49 PM, Paul Eggert wrote:
> POSIX does not require uint64_t, and the C standard
> does not require uint16_t or uint32_t either, so port
> to platforms that lack these types.  The POSIX limitation
> is the only significant one in practice.  I ran into this
> issue when updating Emacs, which still ports to platforms
> lacking 64-bit types.

Oops, sorry about that. I know Emacs uses this module but I didn't
realize it supported platforms without 64-bit types.

> Also, redo to avoid unnecssary parentheses, as these are now
> functions not macros.

Thanks. I started removing parentheses but it messed with Emacs
indentation. I guess c-mode probably only cares about the outer
parentheses now that I think about it.

Collin

Re: [PATCH] byteswap: port better to limited platforms

[Collin Funk]

On 5/17/24 4:51 PM, Bruno Haible wrote:
> I think this produces wrong code with gcc 4.4.7 and older. See:
> 
> -- foo.c --
> unsigned long long x = 0xff00;
> ---
> 
> $ gcc -Wall -S foo.c
> foo.c:1: warning: integer constant is too large for ‘long’ type

I think I've seen warnings like this before with newer GCC versions
but I am not sure if correct code was produced or not.

I have always disliked the need for suffixes after integer constants
in C (U, ULL, ULL, etc.).

If stdint correctly defines UINT16_C, UINT32_C, and UINT64_C then I
would just use those.

POSIX states [1]:

The macro INTN_C(value) shall expand to an integer constant
expression corresponding to the type int_least N _t. The macro
UINTN_C(value) shall expand to an integer constant expression
corresponding to the type uint_least N _t. For example, if
uint_least64_t is a name for the type unsigned long long, then
UINT64_C(0x123) might expand to the integer constant 0x123ULL.

I guess the regular U, UL, ULL suffix would work by themselves or a
cast if preferred.

Collin

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html

Re: tests: Mark tests that fail on NetBSD as expected failures

[Collin Funk]

On 5/17/24 2:10 PM, Bruno Haible wrote:
> Since I don't have time to investigate them and, OTOH, I don't want that
> the NetBSD test failures mark the entire continuous integration run as
> "failed", here are patches to mark them as expected failures.

Seems reasonable. Perhaps it is worth mentioning somewhere that XFAIL
may refer to an actual bug that needs investigation?

Collin 

Re: [PATCH] getusershell: Add tests.

[Collin Funk]

On 5/17/24 6:45 AM, Bruno Haible wrote:
>> It also looks like musl will return empty lines (getline () with
>> length 1), which I believe is incorrect from the FreeBSD man page
>> description.
>
> So, once you have a musl libc VM yourself, you are ready for reporting
> your first musl libc bug. The reporting address is .

Sounds good, thanks for the link.

I'll experiment with the behavior on GNU/Linux and FreeBSD when
'/etc/shells' has an empty line too. I didn't add a check for that but
assuming everyone but musl passes then I'll add it.

Collin

Re: [PATCH] getusershell: Add tests.

[Collin Funk]

Hi Bruno,

On 5/17/24 5:59 AM, Bruno Haible wrote:
> The test fails on Alpine Linux.
> 
> 
> FAIL: test-getusershell
> ===
> 
> ../../gltests/test-getusershell.c:53: assertion 'ptr[0] != '#'' failed
> Aborted (core dumped)
> FAIL test-getusershell (exit status: 134)

Hahaha, I added that check expecting that it would never cause issues.

I remember you saying a few days ago saying that "every function is
worth testing", so here is more evidence. :)

> If you want to look into it: It's easy to install Alpine Linux in a VM.
> Cf. maint-tools/platforms/test-environments.txt. Here's my writeup:

I'll set one up this weekend so I have a musl environment for running
tests.

However, I think this failure has an easy explanation now that I look
at the musl sources.

The glibc implementation is derived from BSD. In the FreeBSD man page
for shells(5) the following is said [1]:

A hash mark (``#'') indicates the beginning of a comment;
subsequent characters up to the end of the line are not
interpreted by the routines which search the file. Blank lines are
also ignored.

In other words getusershell () should ignore comments which glibc also does.

It appears that musl doesn't check for comments which explains that
failure [2].

It also looks like musl will return empty lines (getline () with
length 1), which I believe is incorrect from the FreeBSD man page
description.

If you still have your alpine machine up you can remove the comments
from '/etc/shells' and check if it passes.

[1] 
https://man.freebsd.org/cgi/man.cgi?query=shells=5=0=FreeBSD+14.0-RELEASE+and+Ports
[2] https://git.musl-libc.org/cgit/musl/tree/src/legacy/getusershell.c

Collin

Re: doc: Mention gnulib putenv module?

[Collin Funk]

On 5/17/24 2:38 AM, Bruno Haible wrote:
> Now we can use the usual doc structure.
> 
> 
> 2024-05-17  Bruno Haible  
> 
>   putenv-gnu: Update documentation.
>   * doc/posix-functions/putenv.texi: Refer also to the glibc
>   documentation. Use the usual doc structure.

Looks good. Thanks!

Collin

doc: Update outdated module name.

[Collin Funk]

In the NEWS file:

2009-04-01  visibility  Renamed to lib-symbol-visibility.

The commit that did this missed a reference to 'visibility' in the
documentation. This patch fixes it.

CollinFrom fe8453457c44194354bea89347f713562f67aae2 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Fri, 17 May 2024 02:35:10 -0700
Subject: [PATCH] doc: Update outdated module name.

* doc/ld-version-script.texi (LD Version Scripts): Refer to
'lib-symbol-visibility' instead of 'visibility'.
---
 ChangeLog  | 6 ++
 doc/ld-version-script.texi | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 742a20d012..7a08db245c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-17  Collin Funk  
+
+	doc: Update outdated module name.
+	* doc/ld-version-script.texi (LD Version Scripts): Refer to
+	'lib-symbol-visibility' instead of 'visibility'.
+
 2024-05-16  Collin Funk  
 
 	byteswap: Use __has_builtin portably.
diff --git a/doc/ld-version-script.texi b/doc/ld-version-script.texi
index ac7e01e6d1..cc77dc5cba 100644
--- a/doc/ld-version-script.texi
+++ b/doc/ld-version-script.texi
@@ -73,5 +73,5 @@ @node LD Version Scripts
 @end smallexample
 
 For more discussions about symbol visibility, rather than shared
-library versioning, see the @code{visibility} module
+library versioning, see the @code{lib-symbol-visibility} module
 (@pxref{Exported Symbols of Shared Libraries}).
-- 
2.45.0

Re: byteswap: Use inline functions instead of macros.

[Collin Funk]

Hi Paul,

On 5/16/24 10:16 PM, Paul Eggert wrote:
>> +#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3))\
>> + || (defined __has_builtin && __has_builtin (__builtin_bswap32))
> 
> Unfortunately this usage is not portable, not for __has_builtin or for any of 
> the other __has_XX primitives. This is because older compilers will replace 
> "defined __has_builtin && __has_builtin (__builtin_bswap32)" with "1 && 0 
> (__builtin_bswap32)" and then complain about the bad syntax.

Oops, I wasn't aware of this. Was that a bug in old versions?

> You can safely use __has_builtin (X) only inside a block that is protected by 
> checking that __has_builtin is defined. See stdbit.in.h for an example.

I've applied the attached patch. It should be pretty the same method
as stdbit.in.h.

I'm assuming that _GL_[module-name]_(rest-of-macro) can be assumed
safe?

CollinFrom 73541d459cdd964e71f40ad447e50a7253a6f20d Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 16 May 2024 23:18:16 -0700
Subject: [PATCH] byteswap: Use __has_builtin portably.

Reported by Paul Eggert in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00249.html>.

* lib/byteswap.in.h (_GL_BYTESWAP_HAS_BUILTIN_BSWAP16)
(_GL_BYTESWAP_HAS_BUILTIN_BSWAP32)
(_GL_BYTESWAP_HAS_BUILTIN_BSWAP64): Define using the GCC version or
__has_builtin after checking that it is defined.
(bswap_16, bswap_32, bswap_64): Use the macros.
* modules/byteswap (Depends-on): Add stdbool as a conditional
dependency.
---
 ChangeLog | 13 +
 lib/byteswap.in.h | 29 +++--
 modules/byteswap  |  1 +
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5238067cf9..742a20d012 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2024-05-16  Collin Funk  
+
+	byteswap: Use __has_builtin portably.
+	Reported by Paul Eggert in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00249.html>.
+	* lib/byteswap.in.h (_GL_BYTESWAP_HAS_BUILTIN_BSWAP16)
+	(_GL_BYTESWAP_HAS_BUILTIN_BSWAP32)
+	(_GL_BYTESWAP_HAS_BUILTIN_BSWAP64): Define using the GCC version or
+	__has_builtin after checking that it is defined.
+	(bswap_16, bswap_32, bswap_64): Use the macros.
+	* modules/byteswap (Depends-on): Add stdbool as a conditional
+	dependency.
+
 2024-05-16  Paul Eggert  
 
 	putenv-tests: pacify gcc -Wdiscarded-qualifiers
diff --git a/lib/byteswap.in.h b/lib/byteswap.in.h
index cfa3f04031..ae05d59441 100644
--- a/lib/byteswap.in.h
+++ b/lib/byteswap.in.h
@@ -34,13 +34,32 @@ _GL_INLINE_HEADER_BEGIN
 extern "C" {
 #endif
 
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8)
+# define _GL_BYTESWAP_HAS_BUILTIN_BSWAP16 true
+#elif defined __has_builtin
+# if __has_builtin (__builtin_bswap16)
+#  define _GL_BYTESWAP_HAS_BUILTIN_BSWAP16 true
+# endif
+#endif
+
+#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
+# define _GL_BYTESWAP_HAS_BUILTIN_BSWAP32 true
+# define _GL_BYTESWAP_HAS_BUILTIN_BSWAP64 true
+#elif defined __has_builtin
+# if __has_builtin (__builtin_bswap32)
+#  define _GL_BYTESWAP_HAS_BUILTIN_BSWAP32 true
+# endif
+# if __has_builtin (__builtin_bswap64)
+#  define _GL_BYTESWAP_HAS_BUILTIN_BSWAP64 true
+# endif
+#endif
+
 /* Given an unsigned 16-bit argument X, return the value corresponding to
X with reversed byte order.  */
 _GL_BYTESWAP_INLINE uint16_t
 bswap_16 (uint16_t x)
 {
-#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8))\
- || (defined __has_builtin && __has_builtin (__builtin_bswap16))
+#ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP16
   return __builtin_bswap16 (x);
 #else
   return (x) >> 8) & 0xff) | (((x) & 0xff) << 8)));
@@ -52,8 +71,7 @@ bswap_16 (uint16_t x)
 _GL_BYTESWAP_INLINE uint32_t
 bswap_32 (uint32_t x)
 {
-#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3))\
- || (defined __has_builtin && __has_builtin (__builtin_bswap32))
+#ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP32
   return __builtin_bswap32 (x);
 #else
   return x) & 0xff00u) >> 24) | (((x) & 0x00ffu) >> 8)
@@ -66,8 +84,7 @@ bswap_32 (uint32_t x)
 _GL_BYTESWAP_INLINE uint64_t
 bswap_64 (uint64_t x)
 {
-#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3))\
- || (defined __has_builtin && __has_builtin (__builtin_bswap64))
+#ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP64
   return __builtin_bswap64 (x);
 #else
   return x) & 0xff00ull) >> 56)
diff --git a/modules/byteswap b/modules/byteswap
index 82fe424a61..6e777a699b 100644
--- a/modules/byteswap
+++ b/modules/byteswap
@@ -9,6 +9,7 @@ m4/byteswap.m4
 Depends-on:
 gen-header
 extern-inline   [$GL_GENERATE_BYTESWAP_H]
+stdbool [$GL_GENERATE_BYTESWAP_H]
 stdint  [$GL_GENERATE_BYTESWAP_H]
 
 configure.ac:
-- 
2.45.0

byteswap: Use inline functions instead of macros.

[Collin Funk]

I've applied the two attached patches. The first uses inline functions
for byteswap.h and improves the configure check.

I now see what Paul was saying about floating-point arguments [1].
With the following program:

===
#define bswap_16(x) x) & 0x00FF) << 8) | \
 (((x) & 0xFF00) >> 8))
int
main (void)
{
  return bswap_16 (0.0);
}
===

$ gcc main.c 
main.c: In function ‘main’:
main.c:2:28: error: invalid operands to binary & (have ‘double’ and ‘int’)
2 | #define bswap_16(x) x) & 0x00FF) << 8) | \
  |^
main.c:7:10: note: in expansion of macro ‘bswap_16’
7 |   return bswap_16 (0.0);
  |  ^~~~
main.c:3:28: error: invalid operands to binary & (have ‘double’ and ‘int’)
3 |  (((x) & 0xFF00) >> 8))
  |^
main.c:7:10: note: in expansion of macro ‘bswap_16’
7 |   return bswap_16 (0.0);
  |   


Using the functions from glibc's byteswap.h instead of that macro:

$ gcc main.c 
$ ./a.out 
$ echo $?
0


Doesn't seem like something that is done often but better to behave
more like glibc in my opinion. Using inline functions also avoids
evaluating the argument multiple times like I mentioned previously.
This matches glibc's behavior too.

I'm pretty sure the checks I added to byteswap.m4 should be good
enough. If not the test cases I added in patch 2 should catch it at
least.

[1] https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00188.html

CollinFrom 0858943ba21449b1f8fd0c0ed8c2342cdac3c374 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 16 May 2024 20:37:14 -0700
Subject: [PATCH 1/2] byteswap: Use inline functions instead of macros.

* lib/byteswap.c: New file.
* lib/byteswap.in.h (bswap_16, bswap_32, bswap_64): Use inline functions
instead of macros.
* m4/byteswap.m4 (gl_BYTESWAP): Check that bswap functions can be used
on double values.
* modules/byteswap (Files): Add lib/byteswap.c.
(Depends-on): Add extern-inline and stdint as conditional dependencies.
(Makefile.am): Add lib/byteswap.c to lib_SOURCES.
---
 ChangeLog | 12 
 lib/byteswap.c| 21 +
 lib/byteswap.in.h | 76 +--
 m4/byteswap.m4| 27 ++---
 modules/byteswap  |  4 +++
 5 files changed, 121 insertions(+), 19 deletions(-)
 create mode 100644 lib/byteswap.c

diff --git a/ChangeLog b/ChangeLog
index ba52fbcdf1..ddfe701dd4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2024-05-16  Collin Funk  
+
+	byteswap: Use inline functions instead of macros.
+	* lib/byteswap.c: New file.
+	* lib/byteswap.in.h (bswap_16, bswap_32, bswap_64): Use inline functions
+	instead of macros.
+	* m4/byteswap.m4 (gl_BYTESWAP): Check that bswap functions can be used
+	on double values.
+	* modules/byteswap (Files): Add lib/byteswap.c.
+	(Depends-on): Add extern-inline and stdint as conditional dependencies.
+	(Makefile.am): Add lib/byteswap.c to lib_SOURCES.
+
 2024-05-16  Collin Funk  
 
 	gnulib-tool.py: Fix return value when exiting with Ctrl-C.
diff --git a/lib/byteswap.c b/lib/byteswap.c
new file mode 100644
index 00..6e3dad74ea
--- /dev/null
+++ b/lib/byteswap.c
@@ -0,0 +1,21 @@
+/* Inline functions for .
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This file is free software: you can redistribute it and/or modify
+   it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   This file is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include 
+
+#define _GL_BYTESWAP_INLINE _GL_EXTERN_INLINE
+#include 
diff --git a/lib/byteswap.in.h b/lib/byteswap.in.h
index 8e49efad05..cfa3f04031 100644
--- a/lib/byteswap.in.h
+++ b/lib/byteswap.in.h
@@ -16,29 +16,75 @@
along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #ifndef _GL_BYTESWAP_H
-#define _GL_BYTESWAP_H
+#define _GL_BYTESWAP_H 1
+
+/* This file uses _GL_INLINE.  */
+#if !_GL_CONFIG_H_INCLUDED
+ #error "Please include config.h first."
+#endif
+
+#include 
+
+_GL_INLINE_HEADER_BEGIN
+#ifndef _GL_BYTESWAP_INLINE
+# define _GL_BYTESWAP_INLINE _GL_INLINE
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
 
 /* Given an unsigned 16-bit argument X, return the value corresponding to
X with reversed byte order.  */
-#define bswap_16(x) x) & 0x00FF) << 8) | \
- 

Re: gnulib-tool Python tracebacks after control-C

[Collin Funk]

Hi Bruno,

On 5/16/24 5:52 AM, Bruno Haible wrote:
> Yes, that's what I meant. Instead of "*** Stop." maybe "*** Interrupted."
> (I don't know which of the two, in English, more clearly indicates that it
> cannot be continued. On one hand, a program which received a SIGSTOP
> can be continued via SIGCONT. On the other hand, in French, "interruption"
> means a temporary stop.)

Ah, interesting. I think in English it would mostly depend on the
context. "Interruption" means you are stopping something from
progressing, but I don't think the word itself describes if it is
temporary or not. "Stop" more strongly indicates a permanent halting
of the activity, but that isn't always the case. Language is hard I
guess. :)

I've committed the attached patch since I think the return value is
more important. I'm fine with "*** Stop.", but anyone can change it if
they would like.

I modified the previous diff a bit. Since cli_exception is just an
exception hook, I think it makes the code less awkward to catch the
KeyboardInterrupt separately in another "except ..." line in main().

$ gnulib-tool --create-testdir --dir=testdir1 --single-configure stdbit-h
Module list with included dependencies (indented):
c99
extern-inline
[...]
  tests/test-stdbit-h.c
  tests/test-stdbool.c
executing aclocal -I glm4
^C/home/collin/.local/src/gnulib/gnulib-tool.py: *** Stop.

CollinFrom 46be8fb69d76bed6cb4cc3d4ec5515adb84f033a Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 16 May 2024 18:26:50 -0700
Subject: [PATCH] gnulib-tool.py: Fix return value when exiting with Ctrl-C.

* pygnulib/main.py (main_with_exception_handling): Catch
KeyboardInterrupts and exit with a return code of 1.
---
 ChangeLog| 6 ++
 pygnulib/main.py | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index bb2deb6766..ba52fbcdf1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-16  Collin Funk  
+
+	gnulib-tool.py: Fix return value when exiting with Ctrl-C.
+	* pygnulib/main.py (main_with_exception_handling): Catch
+	KeyboardInterrupts and exit with a return code of 1.
+
 2024-05-16  Collin Funk  
 
 	unsetenv tests: Update module dependencies.
diff --git a/pygnulib/main.py b/pygnulib/main.py
index b693e71d7f..68be0ba28f 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -1380,6 +1380,9 @@ def main_with_exception_handling() -> None:
 try:  # Try to execute
 with tempfile.TemporaryDirectory(prefix='glpy') as temporary_directory:
 main(temporary_directory)
+except KeyboardInterrupt:
+sys.stderr.write('%s: *** Stop.\n' % APP['name'])
+sys.exit(1)
 except GLError as error:
 errmode = 0  # gnulib-style errors
 errno = error.errno
-- 
2.45.0

Re: doc: Mention gnulib putenv module?

[Collin Funk]

Hi Bruno,

On 5/16/24 1:54 PM, Bruno Haible wrote:
>> Thanks for the guide.
> 
> And also update doc/posix-functions/putenv.texi.
> 
>> Should I update occurrences of 'putenv' to
>> 'putenv-gnu' in dependencies
> 
> Yes. (But this can come in a second commit.)
> 
>> and then mark 'putenv' with the
>> 'obsolete' status? That way './bootstrap' will print a small warning
>> letting people know to change it eventually.
> 
> Mark it as "deprecated", not "obsolete", please. See [1].

Done in the two attached patches. I ran the following in Coreutils:

./bootstrap --no-git --skip-po --gnulib-srcdir="$GNULIB_SRCDIR"

and everything worked fine. Notice and correct import, so hopefully no
breakage should occur. :)

CollinFrom 5d2987e6ca5a7d06b4135ab17314051d363621f2 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Thu, 16 May 2024 15:56:47 -0700
Subject: [PATCH 1/2] Rename module 'putenv' to 'putenv-gnu'.

* modules/putenv-gnu: Renamed from modules/putenv.
(Description): Mention the removal of environment variables.
* modules/putenv-gnu-tests: Renamed from modules/putenv-tests.
* modules/putenv: New file, an indirection to the new module.
* doc/posix-functions/putenv.texi: Mention the new module name.
* NEWS: Mention the change.
---
 ChangeLog  | 10 +++
 NEWS   |  2 ++
 doc/posix-functions/putenv.texi|  4 +--
 modules/putenv | 24 +--
 modules/putenv-gnu | 34 ++
 modules/{putenv-tests => putenv-gnu-tests} |  0
 6 files changed, 56 insertions(+), 18 deletions(-)
 create mode 100644 modules/putenv-gnu
 rename modules/{putenv-tests => putenv-gnu-tests} (100%)

diff --git a/ChangeLog b/ChangeLog
index 8bccdfd2e1..f2bfa4246d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-05-16  Collin Funk  
+
+	Rename module 'putenv' to 'putenv-gnu'.
+	* modules/putenv-gnu: Renamed from modules/putenv.
+	(Description): Mention the removal of environment variables.
+	* modules/putenv-gnu-tests: Renamed from modules/putenv-tests.
+	* modules/putenv: New file, an indirection to the new module.
+	* doc/posix-functions/putenv.texi: Mention the new module name.
+	* NEWS: Mention the change.
+
 2024-05-16  Collin Funk  
 
 	putenv: Add tests.
diff --git a/NEWS b/NEWS
index 6caf3ad8e1..57540c8375 100644
--- a/NEWS
+++ b/NEWS
@@ -74,6 +74,8 @@ User visible incompatible changes
 
 DateModules Changes
 
+2024-05-16  putenv  This module is renamed to 'putenv-gnu'.
+
 2024-02-21  *printf-posix   These modules no longer support the 'n' directive
 by default.  In order to keep the 'n' directive
 enabled, you need to additionally request the
diff --git a/doc/posix-functions/putenv.texi b/doc/posix-functions/putenv.texi
index b0ca4ab8b5..ff06ea5062 100644
--- a/doc/posix-functions/putenv.texi
+++ b/doc/posix-functions/putenv.texi
@@ -4,7 +4,7 @@ @node putenv
 
 POSIX specification:@* @url{https://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html}
 
-Gnulib module: ---
+Gnulib module: putenv-gnu
 
 Portability problems fixed by Gnulib:
 @itemize
@@ -14,6 +14,6 @@ @node putenv
 @itemize
 @end itemize
 
-Extension: Gnulib provides a module @samp{putenv} that substitutes a
+Extension: Gnulib provides a module @samp{putenv-gnu} that substitutes a
 @code{putenv} implementation that can also be used to remove environment
 variables.
diff --git a/modules/putenv b/modules/putenv
index a5a7c8d795..acc0659d8b 100644
--- a/modules/putenv
+++ b/modules/putenv
@@ -1,28 +1,20 @@
 Description:
 putenv() function: change or add an environment variable.
 
+Status:
+deprecated
+
+Notice:
+This module is deprecated. Use the module 'putenv-gnu' instead.
+
 Files:
-lib/putenv.c
-m4/putenv.m4
 
 Depends-on:
-stdlib
-environ [test $REPLACE_PUTENV = 1]
-free-posix  [test $REPLACE_PUTENV = 1]
-malloc-posix[test $REPLACE_PUTENV = 1]
+putenv-gnu
 
 configure.ac:
-gl_FUNC_PUTENV
-gl_CONDITIONAL([GL_COND_OBJ_PUTENV], [test $REPLACE_PUTENV = 1])
-AM_COND_IF([GL_COND_OBJ_PUTENV], [
-  gl_PREREQ_PUTENV
-])
-gl_STDLIB_MODULE_INDICATOR([putenv])
 
 Makefile.am:
-if GL_COND_OBJ_PUTENV
-lib_SOURCES += putenv.c
-endif
 
 Include:
 
@@ -31,4 +23,4 @@ License:
 LGPL
 
 Maintainer:
-Jim Meyering, glibc
+all
diff --git a/modules/putenv-gnu b/modules/putenv-gnu
new file mode 100644
index 00..be929f60be
--- /dev/null
+++ b/modules/putenv-gnu
@@ -0,0 +1,34 @@
+Description:
+putenv() function: change, add, or remove an environment variable.
+
+Files:
+lib/putenv.c
+m4/putenv.m4
+
+Depends-on:
+stdlib
+environ [test $REPLACE_PUTENV = 1]
+free-posix  [test $REPLACE_PUTENV = 1]
+malloc-posix[test $REPLACE_PUTENV = 1]
+
+configure.ac:
+gl_FUNC_PUTENV
+gl_CONDITIONAL([GL_COND_

Re: doc: Mention gnulib putenv module?

[Collin Funk]

Hi Bruno,

On 5/16/24 5:45 AM, Bruno Haible wrote:
> The way to deal with it, that is most consistent with the rest of Gnulib,
> is as follows:
> 
>   - Rename the 'putenv' module to 'putenv-gnu'.
> 
>   - Also change its description from:
> putenv() function: change or add an environment variable.
> to:
> putenv() function: change, add, or remove an environment variable.
> 
>   - Rename the 'putenv-tests' module to 'putenv-gnu-tests'.
> 
>   - Add a new 'putenv' module that is merely an indirection to 'putenv-gnu'
> (for backward compatibility, in order not to break coreutils, gcal, guile,
> octave, etc.). In the NEWS file mention
>2024-MM-DD  putenv  This module is renamed to 'putenv-gnu'.

Thanks for the guide. Should I update occurrences of 'putenv' to
'putenv-gnu' in dependencies and then mark 'putenv' with the
'obsolete' status? That way './bootstrap' will print a small warning
letting people know to change it eventually.

Collin

Re: doc: Mention gnulib putenv module?

[Collin Funk]

Hi Bruno,

On 5/16/24 3:19 AM, Bruno Haible wrote:
>> Is there a reason why the Gnulib putenv module isn't mentioned on this
>> line or can I apply this diff with a ChangeLog entry?
> 
> It is mentioned at the end of this file.

I saw. I was thinking maybe it should be mentioned on that line
though. Similar to how execinfo was missing until a few days ago.

> But maybe something could be clarified:
>   - Does POSIX still not allow to remove an environment variable using
> this function?

I don't think that POSIX Issue 7 disallows it [1]. It appears that the
behavior when '=' is not in the string is up for the implementation
decide. This was reported in 2022 and the next revision will clarify
that the environment variable can be removed or the function can fail
with errno == EINVAL and a non-zero return value [2].


>   - On which platforms does the native putenv() not allow to remove an
> environment variable using this function?

Good question. The POSIX report is well written so I won't retype it
here [2]. :)

> I think this is related to very very old shells not having an 'unset'
> primitive.

Interesting. POSIX says that setenv() is preferred anyways.

Collin

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html
[2] https://www.austingroupbugs.net/view.php?id=1598

[PATCH] putenv: Add tests.

[Collin Funk]

* tests/test-putenv.c: New file.
* modules/putenv-tests: New file.
---
 ChangeLog|  6 +
 modules/putenv-tests | 13 ++
 tests/test-putenv.c  | 61 
 3 files changed, 80 insertions(+)
 create mode 100644 modules/putenv-tests
 create mode 100644 tests/test-putenv.c

diff --git a/ChangeLog b/ChangeLog
index 7690099cde..8bccdfd2e1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-05-16  Collin Funk  
+
+   putenv: Add tests.
+   * tests/test-putenv.c: New file.
+   * modules/putenv-tests: New file.
+
 2024-05-15  Collin Funk  
 
gnulib-tool.py: Don't print tracebacks when Ctrl-C is pressed.
diff --git a/modules/putenv-tests b/modules/putenv-tests
new file mode 100644
index 00..43abb8e6c4
--- /dev/null
+++ b/modules/putenv-tests
@@ -0,0 +1,13 @@
+Files:
+tests/test-putenv.c
+tests/macros.h
+tests/signature.h
+
+Depends-on:
+unsetenv
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-putenv
+check_PROGRAMS += test-putenv
diff --git a/tests/test-putenv.c b/tests/test-putenv.c
new file mode 100644
index 00..1768e7563a
--- /dev/null
+++ b/tests/test-putenv.c
@@ -0,0 +1,61 @@
+/* Test the putenv function.
+   Copyright (C) 2024 Free Software Foundation, Inc.
+
+   This file is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published
+   by the Free Software Foundation, either version 3 of the License,
+   or (at your option) any later version.
+
+   This file is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+/* Written by Collin Funk , 2024.  */
+
+#include 
+
+/* Specification.  */
+#include 
+
+#include "signature.h"
+SIGNATURE_CHECK (putenv, int, (char *));
+
+#include 
+
+#include "macros.h"
+
+int
+main (void)
+{
+  char *ptr;
+
+  /* Verify the environment is clean.  */
+  unsetenv ("TEST_VAR");
+  ASSERT (getenv ("TEST_VAR") == NULL);
+
+  /* Verify adding an environment variable.  */
+  {
+ASSERT (putenv ("TEST_VAR=abc") == 0);
+ptr = getenv ("TEST_VAR");
+ASSERT (ptr != NULL);
+ASSERT (STREQ (ptr, "abc"));
+  }
+
+  /* Verify removing an environment variable.  */
+  {
+ASSERT (putenv ("TEST_VAR") == 0);
+ASSERT (getenv ("TEST_VAR") == NULL);
+  }
+
+  /* Verify the behavior when removing a variable not in the environment.  */
+  {
+ASSERT (putenv ("TEST_VAR") == 0);
+ASSERT (getenv ("TEST_VAR") == NULL);
+  }
+
+  return 0;
+}
-- 
2.45.0

doc: Mention gnulib putenv module?

[Collin Funk]

Is there a reason why the Gnulib putenv module isn't mentioned on this
line or can I apply this diff with a ChangeLog entry?

diff --git a/doc/posix-functions/putenv.texi b/doc/posix-functions/putenv.texi
index b0ca4ab8b5..d9e15f5e38 100644
--- a/doc/posix-functions/putenv.texi
+++ b/doc/posix-functions/putenv.texi
@@ -4,7 +4,7 @@ @node putenv
 
 POSIX specification:@* 
@url{https://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html}
 
-Gnulib module: ---
+Gnulib module: putenv
 
 Portability problems fixed by Gnulib:
 @itemize

Collin

Re: gnulib-tool Python tracebacks after control-C

[Collin Funk]

On 5/15/24 8:45 PM, Bruno Haible wrote:
> Can you make gnulib-tool.py print a diagnostic in this case as well?
> It does not need to be exactly "caught signal SIGINT", like bash does.
> Just an indication that
>   - gnulib-tool.py
>   - is being terminated
>   - due to a signal 2.
> Preferrably formatted in the same way as the other fatal errors, i.e.
> starting with
>   '%s: *** ' % APP['name']
> 
> The exit code is nonzero (128 + 2); this is good.

Good point about the error number. I suppose we could just exit on
KeyboardInterrupt and print a message to stderr. Since after the
exception we just exit anyways something like this would work:

$ git diff .
diff --git a/pygnulib/main.py b/pygnulib/main.py
index b693e71d7f..0dea039957 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -1370,6 +1370,9 @@ def cli_exception(exc_type, exc_value, exc_traceback) -> 
None:
 thrown when Ctrl-C is pressed.'''
 if not issubclass(exc_type, KeyboardInterrupt):
 sys.__excepthook__(exc_type, exc_value, exc_traceback)
+else:
+sys.stderr.write('%s: *** Stop.\n' % APP['name'])
+sys.exit(1)

Then using it:

$ rm -rf ../testdir1; ./gnulib-tool.py --create-testdir --dir=../testdir1 
--single-configure stdbit-h
Module list with included dependencies (indented):
c99
extern-inline
gen-header
std-gnu11
  stdbit-h
stdbit-h-tests
stdbool
stdbool-tests
File list:
  lib/dummy.c
  lib/stdbit.c
  lib/stdbit.in.h
  m4/00gnulib.m4
  m4/c-bool.m4
  m4/extern-inline.m4
  m4/gnulib-common.m4
  m4/std-gnu11.m4
  m4/stdbit_h.m4
  m4/zzgnulib.m4
  tests/macros.h
  tests/test-stdbit-h.c
  tests/test-stdbool.c
executing aclocal -I glm4
^C/home/collin/.local/src/gnulib/gnulib-tool.py: *** Stop.
$ echo $?
1

Is that similar to what you were thinking of? I'm not sure what to put
for the error message. Feel free to commit something if this works for you.

Collin

Re: gnulib-tool Python tracebacks after control-C

[Collin Funk]

Hi Pádraig,

On 5/13/24 2:32 PM, Pádraig Brady wrote:
> https://github.com/pixelb/crudini/blob/cbb3b85e/crudini.py#L1134
> Note that also handles a closed stdin (and stdout elsewhere in that util).

I just pushed the attached patch following your code there. Works well
for me. Thanks for the help.

$ gnulib-tool --create-testdir --dir testdir1
^C
$

CollinFrom 43ca9607f03b697df0dfc356ef1a3029551c9897 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Wed, 15 May 2024 20:25:38 -0700
Subject: [PATCH] gnulib-tool.py: Don't print tracebacks when Ctrl-C is
 pressed.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Pádraig Brady in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00200.html>.

* pygnulib/main.py (cli_exception): New function.
(main_with_exception_handling): Use it.
---
 ChangeLog|  8 
 pygnulib/main.py | 11 +++
 2 files changed, 19 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 9a69d7e2aa..7690099cde 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-15  Collin Funk  
+
+	gnulib-tool.py: Don't print tracebacks when Ctrl-C is pressed.
+	Suggested by Pádraig Brady in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00200.html>.
+	* pygnulib/main.py (cli_exception): New function.
+	(main_with_exception_handling): Use it.
+
 2024-05-15  Bruno Haible  
 
 	stdbit-h: Add tests.
diff --git a/pygnulib/main.py b/pygnulib/main.py
index dc0b2f4366..b693e71d7f 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -1365,7 +1365,18 @@ def main(temp_directory: str) -> None:
 pass
 
 
+def cli_exception(exc_type, exc_value, exc_traceback) -> None:
+'''Exception hook that does not print a traceback for KeyboardInterrupts
+thrown when Ctrl-C is pressed.'''
+if not issubclass(exc_type, KeyboardInterrupt):
+sys.__excepthook__(exc_type, exc_value, exc_traceback)
+
+
 def main_with_exception_handling() -> None:
+# Don't print tracebacks for KeyboardInterrupts when stdin is a tty.
+if sys.stdin and sys.stdin.isatty():
+sys.excepthook = cli_exception
+
 try:  # Try to execute
 with tempfile.TemporaryDirectory(prefix='glpy') as temporary_directory:
 main(temporary_directory)
-- 
2.45.0

Re: endian: New module.

[Collin Funk]

On 5/15/24 12:03 PM, Paul Eggert wrote:
>> If we can ensure byteswap.h functions are defined as functions,
>> wouldn't it make sense to just define these as macros to them?
> 
> Not sure why macros would be helpful. If functions suffice for good 
> performance when compiling with -O2, it's better to use functions than macros.

True. I'll revisit my patches and perfer inline functions. Your stdbit
implementation serves as a good example for me. Thanks!

Collin

Re: stdbit-h, stdc_*: New modules, part of the stdbit module

[Collin Funk]

Hi Bruno,

On 5/15/24 4:06 PM, Bruno Haible wrote:
> The module 'stdbit' defines 70 functions, grouped into 14 function-like
> macros. Most packages will only need 1 or 2 among these 14 function groups.
> (For example, diffutils needs only 'stdc_bit_width'.)

Maybe it makes sense to provided a module covering all the stdbit.h
functions?

I can see a program wanting a majority/all of them and in that case it
might be nicer just to import a single module.

Also, should these functions be used to implement the count-* versions
used previously and ffs, ffsl, ffsll? That way any optimization
wizardry can be dealt with in a single place and benefit them all.

Collin

Re: [PATCH] getusershell: Add tests.

[Collin Funk]

Hi Bruno,

On 5/15/24 3:06 AM, Bruno Haible wrote:
> I'm adding a signature test. (So far, we have a signature test for these
> functions only in C++ mode, in tests/test-unistd-c++.cc.)

Thanks! I would have added them if I saw them there.

Should all glibc functions have a signature check? For these ones I
was conflicted on adding them since they are not standardized, but
provided on BSD and glibc.

Collin