gnulib-tool.py: Emit libtests in testdirs generated Makefile.am.

[Collin Funk]

I just pushed the two patches I sent previously.

Patch 0001 is the fix for missing libtests in testdirs.

Patch 0002 is the type hints for classes.

Hopefully no other bug reports for gnulib-tool.py since the info-gnu
announcement other than the one I just patched is a good sign? :)

CollinFrom afca3c77593e3108b0ddfa8ffdf05d7ecfb8137f Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 29 Apr 2024 22:15:56 -0700
Subject: [PATCH 1/2] gnulib-tool.py: Emit libtests in testdirs generated
 Makefile.am.

Reported by Bruno Haible in
.

* pygnulib/GLTestDir.py (GLTestDir.execute): Modify the GLEmiter's
config variable instead of the GLTestDir's so that it can be accessed
when emitting the Makefile.am.
---
 ChangeLog | 9 +
 pygnulib/GLTestDir.py | 3 +--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c52dee0b6f..0fdeee8cf9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-04-29  Collin Funk  
+
+	gnulib-tool.py: Emit libtests in testdirs generated Makefile.am.
+	Reported by Bruno Haible in
+	.
+	* pygnulib/GLTestDir.py (GLTestDir.execute): Modify the GLEmiter's
+	config variable instead of the GLTestDir's so that it can be accessed
+	when emitting the Makefile.am.
+
 2024-04-28  Collin Funk  
 
 	doc: Update macro list in gnulib-cache.m4 documentation.
diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
index 002eb30267..11b067e085 100644
--- a/pygnulib/GLTestDir.py
+++ b/pygnulib/GLTestDir.py
@@ -244,8 +244,7 @@ def execute(self) -> None:
 if file.startswith('lib/'):
 libtests = True
 break
-if libtests:
-self.config.setLibtests(True)
+self.emitter.config.setLibtests(libtests)
 
 if single_configure:
 # Add the dummy module to the main module list if needed.
-- 
2.44.0

From 20061b496065f9a45bddf5a7452c09615f45361c Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 29 Apr 2024 22:20:31 -0700
Subject: [PATCH 2/2] gnulib-tool.py: Add type hints to classes.

* pygnulib/*.py: Add type hints for all instance and class variables.
* pygnulib/GLMakefileTable.py (GLMakefileTable.__getitem__): Fix return
type hint since the dictionary has str values.
---
 ChangeLog   |  7 +++
 pygnulib/GLConfig.py|  2 ++
 pygnulib/GLEmiter.py|  3 +++
 pygnulib/GLError.py |  7 ++-
 pygnulib/GLFileSystem.py|  9 +
 pygnulib/GLImport.py|  8 
 pygnulib/GLMakefileTable.py |  5 -
 pygnulib/GLModuleSystem.py  | 33 +++--
 pygnulib/GLTestDir.py   | 12 
 9 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0fdeee8cf9..bd5bb93c0b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-29  Collin Funk  
+
+	gnulib-tool.py: Add type hints to classes.
+	* pygnulib/*.py: Add type hints for all instance and class variables.
+	* pygnulib/GLMakefileTable.py (GLMakefileTable.__getitem__): Fix return
+	type hint since the dictionary has str values.
+
 2024-04-29  Collin Funk  
 
 	gnulib-tool.py: Emit libtests in testdirs generated Makefile.am.
diff --git a/pygnulib/GLConfig.py b/pygnulib/GLConfig.py
index 16fa490fc6..92aa49d700 100644
--- a/pygnulib/GLConfig.py
+++ b/pygnulib/GLConfig.py
@@ -41,6 +41,8 @@ class GLConfig:
 By default all attributes are set to empty string, empty list or zero.
 The most common value, however, is a None value.'''
 
+table: dict[str, Any]
+
 def __init__(self,
  destdir: str | None = None,
  localpath: list[str] | None = None,
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index ed6eae4997..3fbf796aaa 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -98,6 +98,9 @@ def _eliminate_NMD(snippet: str, automake_subdir: bool) -> str:
 class GLEmiter:
 '''This class is used to emit the contents of necessary files.'''
 
+info: GLInfo
+config: GLConfig
+
 def __init__(self, config: GLConfig) -> None:
 '''Create GLEmiter instance.'''
 self.info = GLInfo()
diff --git a/pygnulib/GLError.py b/pygnulib/GLError.py
index 184d65f59c..4288820c97 100644
--- a/pygnulib/GLError.py
+++ b/pygnulib/GLError.py
@@ -19,6 +19,7 @@
 # Define global imports
 #===
 import os
+from typing import Any
 
 
 #===
@@ -27,7 +28,11 @@
 class GLError(Exception):
 '''Exception handler for pygnulib classes.'''
 
-def __init__(self, errno: int, errinfo: str | float | None = None) -> None:
+errno: int
+errinfo: Any
+args: tuple[int, Any]
+
+def __init__(self, errno: int, errinfo:

Re: warnings in unit tests

[Collin Funk]

Hi Bruno,

On 4/29/24 3:12 PM, Bruno Haible wrote:
> Note that different warning policies may contradict each other. For example,
> some people want to see a warning for
> 
> int *table = malloc (n * sizeof (int));
> 
> because it has an implicit conversion / "lacks a cast". While other people
> want to see a warning for
> 
> int *table = (int *) malloc (n * sizeof (int));
> 
> because it has a cast and "casts are dubious". It is impossible to satisfy
> both of these policies at the same time.

Yes, I've seen both in gnulib. I'm pretty sure the cast is required
for C++ (though I think gcc has a warning to make it less strict).

Maybe a 5th category is code taken from another GNU program. Or 4.5th
category since there are only a few glibc files and mini-gmp IIRC.
In that case the original developer and their preferences would have
to be respected of course. :)

> Back to the four sets of code:
> 
> 1) This warning policy is the responsibility of that package's maintainer,
>obviously.
> 
> 2) These header files are used in compilation units of the package, with
>CFLAGS or AM_CFLAGS set by the package's maintainer for that package.
>Therefore in these files we need to avoid even -Wundef, -Wvla, and
>other kinds of warnings that we wouldn't enable in our code.
> 
> 3) The rest of the lib/ code is under our responsibility, not the
>responsibility of a package's maintainer. We try to avoid warnings
>from "reasonable" warning options. More details in the HACKING file.
> 
> 4) The unit tests are also in our responsibility, not the responsibility
>of a package's maintainer. Here, the primary concern is that is must
>be *easy* to contribute new unit tests. -Wmissing-variable-declarations
>warnings _could_ — as Paul wrote — be avoided by adding an 'extern'
>declaration for each global variable. But this is extra effort that
>would hinder the addition of new unit tests.

That makes sense to me. Thanks for the explanation.

> Collin, if you want to find relevant findings in the unit tests, by
> using gcc or clang warning options, do *not* use a coreutils build
> for this purpose, but a gnulib testdir instead. (Because the latter
> is not biased by coding style preferences of any package maintainer.)
> 
> Or if you really want to use a coreutils build, first update the
> GL_CFLAG_GNULIB_WARNINGS definition in m4/gnulib-common.m4, so that
> it eliminates useless kinds of warnings.

Ah, thanks for the tip. That sounds quicker than modifying the
Makefiles by hand.

Collin

Re: warnings in unit tests

[Bruno Haible]

Hi Collin,

> > For test cases this is more a judgment call, but I prefer doing either
> > the above or adjusting the warning flags, to ignoring warnings, as the
> > other warnings can be useful at time.
> 
> Yeah, I could see these warnings making it hard to see ones that
> actually matter. Lets see what Bruno thinks.

Different warning policies need to apply in these fours sets code:

1) Code that originates in the package that uses gnulib.
   Example: coreutils/src/*

2) Code from public header files in gnulib/lib/
   Example: lib/vasnprintf.h because module 'vasnprintf' designates this file
   as its public header file.

3) All other code from gnulib/lib/
   Example: lib/argp-namefrob.h which is not a public header file, lib/*.c.

4) All code in gnulib/tests/


Note that different warning policies may contradict each other. For example,
some people want to see a warning for

int *table = malloc (n * sizeof (int));

because it has an implicit conversion / "lacks a cast". While other people
want to see a warning for

int *table = (int *) malloc (n * sizeof (int));

because it has a cast and "casts are dubious". It is impossible to satisfy
both of these policies at the same time.


Back to the four sets of code:

1) This warning policy is the responsibility of that package's maintainer,
   obviously.

2) These header files are used in compilation units of the package, with
   CFLAGS or AM_CFLAGS set by the package's maintainer for that package.
   Therefore in these files we need to avoid even -Wundef, -Wvla, and
   other kinds of warnings that we wouldn't enable in our code.

3) The rest of the lib/ code is under our responsibility, not the
   responsibility of a package's maintainer. We try to avoid warnings
   from "reasonable" warning options. More details in the HACKING file.

4) The unit tests are also in our responsibility, not the responsibility
   of a package's maintainer. Here, the primary concern is that is must
   be *easy* to contribute new unit tests. -Wmissing-variable-declarations
   warnings _could_ — as Paul wrote — be avoided by adding an 'extern'
   declaration for each global variable. But this is extra effort that
   would hinder the addition of new unit tests.

> If we decide to follow the coding style you mentioned

No. It must be possible to contribute a unit test with a simple
global variable. Therefore -Wmissing-variable-declarations is not
adequate for unit tests.

Collin, if you want to find relevant findings in the unit tests, by
using gcc or clang warning options, do *not* use a coreutils build
for this purpose, but a gnulib testdir instead. (Because the latter
is not biased by coding style preferences of any package maintainer.)

Or if you really want to use a coreutils build, first update the
GL_CFLAG_GNULIB_WARNINGS definition in m4/gnulib-common.m4, so that
it eliminates useless kinds of warnings.

Bruno

Re: gnulib-tool: Use the Python implementation by default

[Collin Funk]

Hi Dmitry,

On 4/29/24 11:57 AM, Dmitry Selyutin wrote:
> I've been tracking your progress for a while, even though sporadically and
> remaining silent. I'd like to say "thank you" to Bruno and Collin, who made
> it this far and never surrendered. :-)

Thank you for the kind words! I'm glad you are happy with the changes
and don't mind that I showed up and started messing with your code. :)

> Truth to be told, the code I implemented leaves much to be desired,
> and you were extremely passionate and professional about its quality
> (frankly, it deserved a worse reaction; I apologize for the quality
> and overall style). :-)

No need to apologize! I hope any criticism that I had didn't come off
harsh. If someone told me to rewrite 'gnulib-tool' from scratch the
result of my work would be *far* worse than yours. Thank you for doing
the hard work for me! :D

Collin

Re: gnulib-tool.py: Add type hints to classes.

[Bruno Haible]

Hi Collin,

> This patch adds type hints to the Python classes.

Looks good. Thanks!

> Same as previously done in GLFileTable that I wrote. The only new
> thing introduced is the syntax for class variables, so this line in a
> class definition:
> 
>  section_label_pattern = re.compile(...)
> 
> becomes this:
> 
>  section_label_pattern: ClassVar[re.Pattern] = re.compile(...)

It's pretty self-explaining, therefore OK to use this new ClassVar[...].

Bruno

Re: gnulib-tool: Use the Python implementation by default

[Bruno Haible]

Hi Dmitry,

The biggest "thank you" is yours, since you did the majority of the work
(my estimations: you 4 months, Collin 2 months, me 1 month).

OO is hard. I was and am still impressed about how your dissection of the
code into classes (GLConfig, GLModule, GLModuleSystem, etc.) stood the test
of time. Collin and I did not have to move code around between classes or
create new concepts that were not present.

Bruno

Re: gnulib-tool: Use the Python implementation by default

[Dmitry Selyutin]

Hi folks, sorry for the long silence!

I've been tracking your progress for a while, even though sporadically and
remaining silent. I'd like to say "thank you" to Bruno and Collin, who made
it this far and never surrendered. :-) Truth to be told, the code I
implemented leaves much to be desired, and you were extremely passionate
and professional about its quality (frankly, it deserved a worse reaction;
I apologize for the quality and overall style). :-) Many thanks to you for
making this come true!

Re: doc: Update macro list in gnulib-cache.m4 documentation.

[Bruno Haible]

Collin Funk wrote:
> I've applied the attached patch updating the macro list in the
> gnulib-cache.m4 documentation.

Thanks! Appreciated.

Re: [PATCH] gnulib: close file handler before exiting

[Bruno Haible]

Hi,

> diff --git a/lib/gen-uni-tables.c b/lib/gen-uni-tables.c
> index 3ebcd83..94c687f 100644
> --- a/lib/gen-uni-tables.c
> +++ b/lib/gen-uni-tables.c
> @@ -242,6 +242,8 @@ fill_attributes (const char *unicodedata_filename)
>  {
>fprintf (stderr, "missing end range in '%s':%d\n",
> unicodedata_filename, lineno);
> +
> +  fclose (stream);
>exit (1);
>  }
>if (!(field1[0] == '<'

Thanks for the patch. It would not be wrong to close the stream, here,
right before exit(), but

  - It is unnecessary, since it happens during exit() anyway.
  - The GNU Coding Standards [1] say about memory allocation:
  "Memory analysis tools such as valgrind can be useful, but don’t
   complicate a program merely to avoid their false alarms. For
   example, if memory is used until just before a process exits,
   don’t free it simply to silence such a tool."
and the same argument can be made for resource leaks (file
descriptor leaks).

Bruno

[1] https://www.gnu.org/prep/standards/html_node/Memory-Usage.html

[PATCH] gnulib: close file handler before exiting

[zhaoshuang]

---
 lib/gen-uni-tables.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/gen-uni-tables.c b/lib/gen-uni-tables.c
index 3ebcd83..94c687f 100644
--- a/lib/gen-uni-tables.c
+++ b/lib/gen-uni-tables.c
@@ -242,6 +242,8 @@ fill_attributes (const char *unicodedata_filename)
 {
   fprintf (stderr, "missing end range in '%s':%d\n",
unicodedata_filename, lineno);
+
+  fclose (stream);
   exit (1);
 }
   if (!(field1[0] == '<'
@@ -250,6 +252,8 @@ fill_attributes (const char *unicodedata_filename)
 {
   fprintf (stderr, "missing end range in '%s':%d\n",
unicodedata_filename, lineno);
+
+  fclose (stream);
   exit (1);
 }
   field1[strlen (field1) - 7] = '\0';
-- 
2.20.1

gnulib-tool.py: Add type hints to classes.

[Collin Funk]

This patch adds type hints to the Python classes.

Same as previously done in GLFileTable that I wrote. The only new
thing introduced is the syntax for class variables, so this line in a
class definition:

 section_label_pattern = re.compile(...)

becomes this:

 section_label_pattern: ClassVar[re.Pattern] = re.compile(...)

That variable is shared between all GLModule objects to match the
start of sections in the module description (e.g. 'Depends-on:').

I guess it is also used for checkers to give warnings when modifying
class variables on an instance variable:

var = GLModule(...)
var.section_label_pattern = 'warning here'
GLModule.section_label_pattern = 'no warning here'

But I haven't checked that. I remember finding those confusing when I
first used Python, so I think it serves as a good reminder. No harm as
far as comparability goes since it was introduced in version 3.5 [1].

I'll probably wait until late today or tomorrow to push this + the
--create-megatestdir fix [2].

[1] https://docs.python.org/3/library/typing.html#typing.ClassVarx
[2] https://lists.gnu.org/archive/html/bug-gnulib/2024-04/msg00501.html

CollinFrom 613a0c35a7a03d1c716eba8ae98d5018f04a57f5 Mon Sep 17 00:00:00 2001
From: Collin Funk 
Date: Mon, 29 Apr 2024 03:25:32 -0700
Subject: [PATCH] gnulib-tool.py: Add type hints to classes.

* pygnulib/*.py: Add type hints for all instance and class variables.
* pygnulib/GLMakefileTable.py (GLMakefileTable.__getitem__): Fix return
type hint since the dictionary has str values.
---
 ChangeLog   |  7 +++
 pygnulib/GLConfig.py|  2 ++
 pygnulib/GLEmiter.py|  3 +++
 pygnulib/GLError.py |  7 ++-
 pygnulib/GLFileSystem.py|  9 +
 pygnulib/GLImport.py|  8 
 pygnulib/GLMakefileTable.py |  5 -
 pygnulib/GLModuleSystem.py  | 33 +++--
 pygnulib/GLTestDir.py   | 12 
 9 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c52dee0b6f..8472be20ee 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-29  Collin Funk  
+
+	gnulib-tool.py: Add type hints to classes.
+	* pygnulib/*.py: Add type hints for all instance and class variables.
+	* pygnulib/GLMakefileTable.py (GLMakefileTable.__getitem__): Fix return
+	type hint since the dictionary has str values.
+
 2024-04-28  Collin Funk  
 
 	doc: Update macro list in gnulib-cache.m4 documentation.
diff --git a/pygnulib/GLConfig.py b/pygnulib/GLConfig.py
index 16fa490fc6..92aa49d700 100644
--- a/pygnulib/GLConfig.py
+++ b/pygnulib/GLConfig.py
@@ -41,6 +41,8 @@ class GLConfig:
 By default all attributes are set to empty string, empty list or zero.
 The most common value, however, is a None value.'''
 
+table: dict[str, Any]
+
 def __init__(self,
  destdir: str | None = None,
  localpath: list[str] | None = None,
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index ed6eae4997..3fbf796aaa 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -98,6 +98,9 @@ def _eliminate_NMD(snippet: str, automake_subdir: bool) -> str:
 class GLEmiter:
 '''This class is used to emit the contents of necessary files.'''
 
+info: GLInfo
+config: GLConfig
+
 def __init__(self, config: GLConfig) -> None:
 '''Create GLEmiter instance.'''
 self.info = GLInfo()
diff --git a/pygnulib/GLError.py b/pygnulib/GLError.py
index 184d65f59c..4288820c97 100644
--- a/pygnulib/GLError.py
+++ b/pygnulib/GLError.py
@@ -19,6 +19,7 @@
 # Define global imports
 #===
 import os
+from typing import Any
 
 
 #===
@@ -27,7 +28,11 @@
 class GLError(Exception):
 '''Exception handler for pygnulib classes.'''
 
-def __init__(self, errno: int, errinfo: str | float | None = None) -> None:
+errno: int
+errinfo: Any
+args: tuple[int, Any]
+
+def __init__(self, errno: int, errinfo: Any = None) -> None:
 '''Each error has following parameters:
 errno: code of error; used to catch error type
   1: file does not exist in GLFileSystem: 
diff --git a/pygnulib/GLFileSystem.py b/pygnulib/GLFileSystem.py
index 028ba3885e..f8b7f54ab3 100644
--- a/pygnulib/GLFileSystem.py
+++ b/pygnulib/GLFileSystem.py
@@ -46,6 +46,8 @@ class GLFileSystem:
 Its main method lookup(file) is used to find file in these directories or
 combine it using Linux 'patch' utility.'''
 
+config: GLConfig
+
 def __init__(self, config: GLConfig) -> None:
 '''Create new GLFileSystem instance. The only argument is localpath,
 which can be an empty list.'''
@@ -139,6 +141,13 @@ def shouldLink(self, original: str, lookedup: str) -> bool:
 class GLFileAssistant:
 '''GLFileAssistant is used to help with file processing.'''
 
+original: str | 

Re: Unknown type name 'wint_t' when targeting Cygwin

[Markus Mützel]

Hi Bruno,

Bruno Haible wrote

> Which Cygwin version, please?

That error occurred, e.g., in a CI run
https://github.com/gnu-octave/octave/actions/runs/8873331621/job/24358996111

The log of that run contains the following line:
Starting cygwin install, version 2.932

Is that the Cygwin version?
All packages should be the latest stable versions that are distributed by the 
Cygwin project.

> Also, what is the gcc command line of this particular compilation unit?
> (`make V=1`)

The CI uses a parallel make. IIUC, the following lines in the log match and 
reproduce one of the errors:

/bin/sh ../libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. 
-I../../libgnu -I.. -Wno-cast-qual -Wno-conversion -Wno-float-equal 
-Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter 
-Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion 
-Wno-type-limits -Wno-unsuffixed-float-constants -g -O2 -MT 
libgnu_la-areadlink.lo -MD -MP -MF .deps/libgnu_la-areadlink.Tpo -c -o 
libgnu_la-areadlink.lo `test -f 'areadlink.c' || echo 
'../../libgnu/'`areadlink.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../../libgnu -I.. -Wno-cast-qual 
-Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef 
-Wno-unused-function -Wno-unused-parameter -Wno-float-conversion 
-Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits 
-Wno-unsuffixed-float-constants -g -O2 -MT libgnu_la-areadlink.lo -MD -MP -MF 
.deps/libgnu_la-areadlink.Tpo -c ../../libgnu/areadlink.c  -DDLL_EXPORT -DPIC 
-o .libs/libgnu_la-areadlink.o

In file included from /usr/include/sys/reent.h:16,
 from /usr/include/stdlib.h:18,
 from ./stdlib.h:36,
 from ../../libgnu/areadlink.h:21,
 from ../../libgnu/areadlink.c:25:
/usr/include/sys/_types.h:167:5: error: unknown type name 'wint_t'
  167 | wint_t __wch;
  | ^~


Please, let me know if that is the information that you are looking for or if 
you need further details.

Markus

Re: Unknown type name 'wint_t' when targeting Cygwin

[Collin Funk]

Hi Bruno,

On 4/29/24 12:02 AM, Bruno Haible wrote:
> Which Cygwin version, please?
> 
> Also, what is the gcc command line of this particular compilation unit?
> (`make V=1`)

While you wait for the answer to this, I noticed that Cygwin does
things similar to glibc. In sys/_types.h [1]:

#define __need_size_t
#define __need_wint_t
#include 

Hopefully that leads you down the right direction.

[1] 
https://github.com/cygwin/cygwin/blob/579064bf4d408e99ed7556f36a3050c7ee99dee6/newlib/libc/include/sys/_types.h#L23

Collin

Re: Unknown type name 'wint_t' when targeting Cygwin

[Bruno Haible]

Hi Markus,

Markus Mützel wrote:
> We recently updated gnulib to a newer revision in GNU Octave (currently 
> 92d80242ad1344b5364ca9bd1d995d68c3a73ef7).
> 
> Since then we are seeing compilation errors like the following when targeting 
> Cygwin:

Which Cygwin version, please?

Also, what is the gcc command line of this particular compilation unit?
(`make V=1`)

I need these infos in order to reproduce the problem, without wasting
time trying with the wrong Cygwin version or the wrong CFLAGS.

Bruno