[PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Collin Funk]

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

Hello, here is a patch implementing the --gnu-make option for
gnulib-tool.py. Sorry in advance for the long ChangeLong entry. It
makes it look like I did work than I actually did :). All of these
commits were grouped together and were similar so I felt like it
didn't make too much sense to handle them separately.

I ended up testing these changes with Paul Eggert's merge-gnulib
script used by Emacs [1]. Maybe there is a better way to test it but
this seemed to work pretty well. First I would update the sources with
the regular gnulib-tool, save the generated Makefile, and then run the
script again with gnulib-tool.py.

cd /path/to/src/emacs/
./admin/merge-gnulib
mv lib/gnulib.mk.in lib/gnulib.mk.in.old
sed -i -e 's/gnulib-tool /gnulib-tool.py /g' admin/merge-gnulib
./admin/merge-gnulib
diff lib/gnulib.mk.in lib/gnulib.mk.in.old

The output seems to be correct. It isn't perfect but you can see what
parts still need to be implemented. For example, a lot of the variables
are named incorrectly because of the commit
415fae8ddcb39d33f364c81b0f199e28c65bb539 which is marked in
gnulib-tool.py.TODO.

-ifneq (,$(gl_GNULIB_ENABLED_euidaccess_CONDITION))
-ifneq (,$(GL_COND_OBJ_EUIDACCESS_CONDITION))
+ifneq (,$(gl_GNULIB_ENABLED_euidaccess))
+ifneq (,$(GL_COND_OBJ_EUIDACCESS))

Most of the other incorrect lines are because of the changes made to
generating header files which gnulib-tool.py has not caught up with.

Let me know if you see something that I missed.

Thanks,
Collin

[1] https://git.savannah.gnu.org/cgit/emacs.git/tree/admin/merge-gnulib

[-- Attachment #2: 0001-gnulib-tool.py-Follow-gnulib-tool-changes-part-27.patch --]
[-- Type: text/x-patch, Size: 21176 bytes --]

From edf3dd7dc7f0d8c2b4bd77213554a236e23dca4e Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Thu, 22 Feb 2024 20:16:18 -0800
Subject: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

Follow gnulib-tool changes
2017-03-12  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: new option --gnu-make
2017-03-13  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: minor --gnu-make fixups
2017-03-14  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: fix typo in comment output
2017-05-14  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: improve GNU Make debugging
2018-07-04  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: minor tweaks for --gnu-make

* gnulib-tool.py (main): Add --gnu-make option. Do not allow --gnu-make
in test modes, since they all require Automake.
* pygnulib/GLConfig.py: Add gnu_make argument to constructor.
(getGnuMake, setGnuMake, resetGnuMake): New methods for accessing the
gnu_make instance variable.
* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Use the "+=" operator
with GNU Make and Automake. Transform conditionals to GNU Make syntax if
--gnu-make is in use. Use a Autoconf subprocess to define values and
check the return code for errors.
(GLEmiter.tests_Makefile_am): Likewise.
* pygnulib/GLImport.py (GLImport.actioncmd): Add "--gnu-make" to the
output comment if it is in use.
(GLImport.gnulib_comp): Don't require "AM_PROG_CC_C_O" when using GNU
Make.
* pygnulib/GLInfo.py (GLInfo.usage): Update help message to reflect
addition of --gnu-make.
* pygnulib/GLModuleSystem.py (GLModuleTable.transitive_closure): Don't
add Automake snippets as unconditional dependencies when using
--gnu-make.
---
 ChangeLog                  | 34 +++++++++++++++++++
 gnulib-tool.py             | 11 ++++++
 gnulib-tool.py.TODO        | 58 --------------------------------
 pygnulib/GLConfig.py       | 28 +++++++++++++++-
 pygnulib/GLEmiter.py       | 68 ++++++++++++++++++++++++++++++++------
 pygnulib/GLImport.py       |  6 +++-
 pygnulib/GLInfo.py         |  8 +++--
 pygnulib/GLModuleSystem.py |  2 +-
 8 files changed, 140 insertions(+), 75 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2bd61f3052..41f338b0b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,37 @@
+2024-02-22  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Follow gnulib-tool changes, part 27.
+	Follow gnulib-tool changes
+	2017-03-12  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: new option --gnu-make
+	2017-03-13  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: minor --gnu-make fixups
+	2017-03-14  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: fix typo in comment output
+	2017-05-14  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: improve GNU Make debugging
+	2018-07-04  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: minor tweaks for --gnu-make
+	* gnulib-tool.py (main): Add --gnu-make option. Do not allow --gnu-make
+	in test modes, since they all require Automake.
+	* pygnulib/GLConfig.py: Add gnu_make argument to constructor.
+	(getGnuMake, setGnuMake, resetGnuMake): New methods for accessing the
+	gnu_make instance variable.
+	* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Use the "+=" operator
+	with GNU Make and Automake. Transform conditionals to GNU Make syntax if
+	--gnu-make is in use. Use a Autoconf subprocess to define values and
+	check the return code for errors.
+	(GLEmiter.tests_Makefile_am): Likewise.
+	* pygnulib/GLImport.py (GLImport.actioncmd): Add "--gnu-make" to the
+	output comment if it is in use.
+	(GLImport.gnulib_comp): Don't require "AM_PROG_CC_C_O" when using GNU
+	Make.
+	* pygnulib/GLInfo.py (GLInfo.usage): Update help message to reflect
+	addition of --gnu-make.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.transitive_closure): Don't
+	add Automake snippets as unconditional dependencies when using
+	--gnu-make.
+
 2024-02-22  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Follow gnulib-tool changes, part 26.
diff --git a/gnulib-tool.py b/gnulib-tool.py
index 876a16ca9f..599dbe0984 100755
--- a/gnulib-tool.py
+++ b/gnulib-tool.py
@@ -358,6 +358,11 @@ def main():
                         action='append',
                         choices=['2', '3orGPLv2', '3'],
                         nargs='?')
+    # gnu-make
+    parser.add_argument('--gnu-make',
+                        dest='gnu_make',
+                        default=None,
+                        action='store_true')
     # makefile-name
     parser.add_argument('--makefile-name',
                         dest='makefile_name',
@@ -602,6 +607,10 @@ def main():
     if cmdargs.pobase == None and cmdargs.podomain != None:
         message = '%s: warning: --po-domain has no effect without a --po-base option\n' % constants.APP['name']
         sys.stderr.write(message)
+    if modules != None and "tests" in mode and gnu_make:
+        message = '%s: --gnu-make not supported when including tests\n' % constants.APP['name']
+        sys.stderr.write(message)
+        sys.exit(1)
 
     # Determine specific settings.
     destdir = cmdargs.destdir
@@ -670,6 +679,7 @@ def main():
             lgpl = True
     cond_dependencies = cmdargs.cond_dependencies
     libtool = cmdargs.libtool
+    gnu_make = cmdargs.gnu_make == True
     makefile_name = cmdargs.makefile_name
     if makefile_name != None:
         makefile_name = makefile_name[0]
@@ -709,6 +719,7 @@ def main():
         excl_test_categories=excl_test_categories,
         libname=libname,
         lgpl=lgpl,
+        gnu_make=gnu_make,
         makefile_name=makefile_name,
         libtool=libtool,
         conddeps=cond_dependencies,
diff --git a/gnulib-tool.py.TODO b/gnulib-tool.py.TODO
index 2b33ae2308..aef3ca253b 100644
--- a/gnulib-tool.py.TODO
+++ b/gnulib-tool.py.TODO
@@ -903,64 +903,6 @@ Date:   Tue Mar 14 12:19:40 2017 +0100
 
 --------------------------------------------------------------------------------
 
-commit 60e8b9303d8ce312bb2322d4801ed08678f93d1e
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Wed Jul 4 20:42:07 2018 -0700
-
-    gnulib-tool: minor tweaks for --gnu-make
-
-    * gnulib-tool: Do not allow --gnu-make in test modes,
-    since they all require automake.
-    (func_emit_lib_Makefile_am): Don’t emit automake comment
-    if --gnu-make.
-
-commit 8224d65142d7b8cea2b8721a7d09c2cd60d2d312
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Mon May 15 07:41:10 2017 -0700
-
-    gnulib-tool: improve GNU Make debugging
-
-    * gnulib-tool (func_emit_lib_Makefile_am): Omit unnecessary echo.
-    Report autoconf diagnostics when it fails, in the output makefile.
-
-commit fb8289f44a58c9462434eba8eaffd58c3f417c42
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Tue Mar 14 08:39:27 2017 -0700
-
-    gnulib-tool: fix typo in comment output
-
-    * gnulib-tool (func_import): Fix typo with previous change.
-
-commit d6088547633af472625ab815452004c22fda6d58
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Mon Mar 13 15:50:44 2017 -0700
-
-    gnulib-tool: minor --gnu-make fixups
-
-    * gnulib-tool (func_emit_lib_Makefile_am):
-    Remove useless code that was a blind alley during implementation.
-    Problem reported by Thien-Thi Nguyen in:
-    http://lists.gnu.org/archive/html/bug-gnulib/2017-03/msg00029.html
-    (func_import): Note the "--gnu-make" option in the output comment.
-
-commit dfbe4c0276701e42ffaed13a1c7a79003dc8fb30
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Sun Mar 12 19:18:53 2017 -0700
-
-    gnulib-tool: new option --gnu-make
-
-    This is for applications like GNU Emacs that use GNU Make
-    features instead of Automake.
-    * doc/gnulib-tool.texi (Initial import): Mention --gnu-make.
-    * doc/gnulib.texi (Unit test modules, Build robot for gnulib):
-    Do not assume Automake.
-    * gnulib-tool (func_determine_path_separator)
-    (func_modules_transitive_closure, func_update_file)
-    (func_emit_lib_Makefile_am, func_emit_tests_Makefile_am)
-    (func_import): Add support for --gnu-make.
-
---------------------------------------------------------------------------------
-
 commit 9bdf6c8a0cdeb13c12e4b65dee9538c5468dbe1d
 Author: Bruno Haible <bruno@clisp.org>
 Date:   Sun Aug 19 14:06:50 2012 +0200
diff --git a/pygnulib/GLConfig.py b/pygnulib/GLConfig.py
index 3fb78083c6..bdebf243cc 100644
--- a/pygnulib/GLConfig.py
+++ b/pygnulib/GLConfig.py
@@ -56,7 +56,8 @@ class GLConfig(object):
                  sourcebase=None, m4base=None, pobase=None, docbase=None, testsbase=None,
                  modules=None, avoids=None, files=None,
                  incl_test_categories=None, excl_test_categories=None, libname=None,
-                 lgpl=None, makefile_name=None, libtool=None, conddeps=None, macro_prefix=None,
+                 lgpl=None, gnu_make=None, makefile_name=None, libtool=None,
+                 conddeps=None, macro_prefix=None,
                  podomain=None, witness_c_macro=None, vc_files=None, symbolic=None,
                  lsymbolic=None, configure_ac=None, ac_version=None,
                  libtests=None, single_configure=None, verbose=None, dryrun=None,
@@ -130,6 +131,10 @@ class GLConfig(object):
         self.resetLGPL()
         if lgpl != None:
             self.setLGPL(lgpl)
+        # gnu-make
+        self.resetGnuMake()
+        if gnu_make != None:
+            self.setGnuMake(gnu_make)
         # makefile_name
         self.resetMakefileName()
         if makefile_name != None:
@@ -776,6 +781,27 @@ class GLConfig(object):
         Default value is None, which means that lgpl is disabled.'''
         self.table['lgpl'] = None
 
+    # Define gnu-make methods.
+    def getGnuMake(self):
+        '''Return a boolean value describing whether the --gnu-make argument
+        was used.'''
+        return self.table['gnu_make']
+
+    def setGnuMake(self, value):
+        '''Set the --gnu-make argument as if it were invoked using the
+        command-line or disable it.'''
+        if type(value) is bool:
+            self.table['gnu_make'] = value
+        else:  # if type(value) is not bool
+            raise TypeError('value must be a bool, not %s'
+                            % type(value).__name__)
+
+    def resetGnuMake(self):
+        '''Reset the --gnu-make argument to its default. This feature must be
+        explicitly enabled by programs who utilize GNU Make features instead
+        of Autmake.'''
+        self.table['gnu_make'] = False
+
     def getModuleIndicatorPrefix(self):
         '''Return module_indicator_prefix to use inside GLEmiter class.'''
         return self.getIncludeGuardPrefix()
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index c7b1e61fc4..6de9aba68a 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -40,6 +40,7 @@ __copyright__ = constants.__copyright__
 #===============================================================================
 # Define global constants
 #===============================================================================
+UTILS = constants.UTILS
 TESTS = constants.TESTS
 joinpath = constants.joinpath
 relinverse = constants.relinverse
@@ -627,6 +628,7 @@ AC_DEFUN([%V1%_LIBSOURCES], [
         libname = self.config['libname']
         pobase = self.config['pobase']
         auxdir = self.config['auxdir']
+        gnu_make = self.config['gnu_make']
         makefile_name = self.config['makefile_name']
         libtool = self.config['libtool']
         macro_prefix = self.config['macro_prefix']
@@ -639,9 +641,9 @@ AC_DEFUN([%V1%_LIBSOURCES], [
         destfile = os.path.normpath(destfile)
         emit = ''
 
-        # When creating an includable Makefile.am snippet, augment variables with
-        # += instead of assigning them.
-        if makefile_name:
+        # When using GNU make, or when creating an includable Makefile.am snippet,
+        # augment variables with += instead of assigning them.
+        if gnu_make or makefile_name:
             assign = '+='
         else:  # if not makefile_name
             assign = '='
@@ -662,7 +664,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
         else:  # if not for_test
             edit_check_PROGRAMS = False
         emit += "## DO NOT EDIT! GENERATED AUTOMATICALLY!\n"
-        emit += "## Process this file with automake to produce Makefile.in.\n"
+        if not gnu_make:
+            emit += "## Process this file with automake to produce Makefile.in.\n"
         emit += self.copyright_notice()
         if actioncmd:
             # The maximum line length (excluding the terminating newline) of
@@ -711,16 +714,33 @@ AC_DEFUN([%V1%_LIBSOURCES], [
                                                 '$(' + module_indicator_prefix + '_GNULIB_')
                 # Skip the contents if it's entirely empty.
                 if not (amsnippet1 + amsnippet2).isspace():
-                    allsnippets += '## begin gnulib module %s\n\n' % str(module)
+                    allsnippets += '## begin gnulib module %s\n' % str(module)
+                    if gnu_make:
+                        allsnippets += 'ifeq (,$(OMIT_GNULIB_MODULE_%s))\n' % str(module)
+                        convert_to_gnu_make = True
+                    else:
+                        convert_to_gnu_make = False
+                    allsnippets += '\n'
                     if conddeps:
                         if moduletable.isConditional(module):
                             name = module.getConditionalName()
-                            allsnippets += 'if %s\n' % name
-                    allsnippets += amsnippet1
+                            if gnu_make:
+                                allsnippets += 'ifneq (,$(%s))\n' % name
+                            else:
+                                allsnippets += 'if %s\n' % name
+                    if convert_to_gnu_make:
+                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet1)
+                    else:
+                        allsnippets += amsnippet1
                     if conddeps:
                         if moduletable.isConditional(module):
                             allsnippets += 'endif\n'
-                    allsnippets += amsnippet2
+                    if convert_to_gnu_make:
+                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet2)
+                    else:
+                        allsnippets += amsnippet2
+                    if gnu_make:
+                        allsnippets += 'endif\n'
                     allsnippets += '## end   gnulib module %s\n\n' % str(module)
 
                     # Test whether there are some source files in subdirectories.
@@ -763,6 +783,24 @@ AC_DEFUN([%V1%_LIBSOURCES], [
             emit += 'DISTCLEANFILES =\n'
             emit += 'MAINTAINERCLEANFILES =\n'
 
+        if gnu_make:
+            emit += '# Start of GNU Make output.\n'
+            result = sp.run([UTILS['autoconf'], '-t', 'AC_SUBST:$1 = @$1@',
+                            joinpath(self.config['destdir'], 'configure.ac')],
+                            capture_output=True)
+            if result.returncode == 0:
+                # sort -u
+                emit += '\n'.join(sorted(list(set(x.strip() for x in
+                                                  result.stdout.decode(encoding='utf-8').splitlines()))))
+                emit += '\n'
+            else:
+                emit += '== gnulib-tool GNU Make output failed as follows ==\n'
+                emit += ['# stderr: ' + x + '\n' for x in
+                         result.stderr.decode(encoding='utf-8').splitlines()]
+            emit += '# End of GNU Make output.\n'
+        else:
+            emit += '# No GNU Make output.\n'
+
         # Execute edits that apply to the Makefile.am being generated.
         for current_edit in range(0, makefiletable.count()):
             dictionary = makefiletable[current_edit]
@@ -895,6 +933,7 @@ AC_DEFUN([%V1%_LIBSOURCES], [
         m4base = self.config['m4base']
         pobase = self.config['pobase']
         testsbase = self.config['testsbase']
+        gnu_make = self.config['gnu_make']
         makefile_name = self.config['makefile_name']
         libtool = self.config['libtool']
         macro_prefix = self.config['macro_prefix']
@@ -964,9 +1003,16 @@ AC_DEFUN([%V1%_LIBSOURCES], [
 
                 # Skip the contents if it's entirely empty.
                 if not snippet.isspace():
-                    snippet = ('## begin gnulib module %s\n\n' % str(module)
-                               + snippet
-                               + '## end   gnulib module %s\n\n' % str(module))
+                    nonwrapped_snippet = snippet
+                    snippet = '## begin gnulib module %s\n' % str(module)
+                    if gnu_make:
+                        snippet += 'ifeq (,$(OMIT_GNULIB_MODULE_%s))\n' % str(module)
+                    snippet += '\n'
+                    snippet += nonwrapped_snippet
+                    if gnu_make:
+                        snippet += 'endif\n'
+                    snippet += '## end   gnulib module %s\n' % str(module)
+                    snippet += '\n'
                     # Mention long-running tests at the end.
                     if 'longrunning-test' in module.getStatuses():
                         longrun_snippets += snippet
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 68b2e7b0bb..e01cb9f63e 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -365,6 +365,7 @@ class GLImport(object):
         conddeps = self.config.checkCondDeps()
         libname = self.config.getLibName()
         lgpl = self.config.getLGPL()
+        gnu_make = self.config.getGnuMake()
         makefile_name = self.config.getMakefileName()
         libtool = self.config.checkLibtool()
         macro_prefix = self.config.getMacroPrefix()
@@ -407,6 +408,8 @@ class GLImport(object):
                 actioncmd += ' --lgpl'
             else:  # if lgpl != True
                 actioncmd += ' --lgpl=%s' % lgpl
+        if gnu_make:
+            actioncmd += ' --gnu-make'
         if makefile_name:
             actioncmd += ' --makefile-name=%s' % makefile_name
         if conddeps:
@@ -563,6 +566,7 @@ class GLImport(object):
         testsbase = self.config['testsbase']
         lgpl = self.config['lgpl']
         libname = self.config['libname']
+        gnu_make = self.config['gnu_make']
         makefile_name = self.config['makefile_name']
         conddeps = self.config['conddeps']
         libtool = self.config['libtool']
@@ -602,7 +606,7 @@ AC_DEFUN([%s_EARLY],
                         and file.count('/') > 1):
                     uses_subdirs = True
                     break
-        if uses_subdirs:
+        if not gnu_make and uses_subdirs:
             emit += '  AC_REQUIRE([AM_PROG_CC_C_O])\n'
         for module in moduletable['final']:
             emit += '  # Code from module %s:\n' % str(module)
diff --git a/pygnulib/GLInfo.py b/pygnulib/GLInfo.py
index 69d3e4a7f8..808f11b06f 100644
--- a/pygnulib/GLInfo.py
+++ b/pygnulib/GLInfo.py
@@ -265,14 +265,16 @@ Options for --import, --add/remove-import:
                             placed (default \"tests\").
       --aux-dir=DIRECTORY   Directory relative to --dir where auxiliary build
                             tools are placed (default comes from configure.ac).
+      --gnu-make            Output for GNU Make instead of for the default
+                            Automake
       --lgpl[=2|=3orGPLv2|=3]
                             Abort if modules aren't available under the LGPL.
                             Also modify license template from GPL to LGPL.
                             The version number of the LGPL can be specified;
                             the default is currently LGPLv3.
-      --makefile-name=NAME  Name of makefile in automake syntax in the
-                            source-base and tests-base directories
-                            (default \"Makefile.am\").
+      --makefile-name=NAME  Name of makefile in the source-base and tests-base
+                            directories (default \"Makefile.am\", or
+                            \"Makefile.in\" if --gnu-make).
       --macro-prefix=PREFIX  Specify the prefix of the macros 'gl_EARLY' and
                             'gl_INIT'. Default is 'gl'.
       --po-domain=NAME      Specify the prefix of the i18n domain. Usually use
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index a97e4b310e..c6f0eb15c7 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -860,7 +860,7 @@ class GLModuleTable(object):
                         automake_snippet = \
                             module.getAutomakeSnippet_Conditional()
                         pattern = re.compile('^if', re.M)
-                        if not pattern.findall(automake_snippet):
+                        if not self.config['gnu_make'] and not pattern.findall(automake_snippet):
                             # A module whose Makefile.am snippet contains a
                             # reference to an automake conditional. If we were
                             # to use it conditionally, we would get an error
-- 
2.39.2

Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Bruno Haible]

Hello Collin,

Collin Funk wrote:
> Hello, here is a patch implementing the --gnu-make option for
> gnulib-tool.py.

Thanks! Applied. This was already a major piece of work.

> All of these
> commits were grouped together and were similar so I felt like it
> didn't make too much sense to handle them separately.

Yes, keeping them together was the right thing to do.

> I ended up testing these changes with Paul Eggert's merge-gnulib
> script used by Emacs [1]. Maybe there is a better way to test it but
> this seemed to work pretty well. First I would update the sources with
> the regular gnulib-tool, save the generated Makefile, and then run the
> script again with gnulib-tool.py.

Perfect. That's the perfect way to test it this --gnu-make option.

Just three small nits that you might want to revisit:

* gnulib-tool.py line 610:
+    if modules != None and "tests" in mode and gnu_make:
        ^^^^^^^^^^^^^^^^^^^
What is the rationale for this condition? It is not present in the original. Can
it be removed?

* pygnulib/GLEmiter.py lines 732, 739:
+                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet1)
+                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet2)

The regular expression is duplicated. The problem with duplicated code is
that it very often leads to bugs in the future: Future code maintainers
will see one copy of the piece of code, modify it, and leave the other
copy unchanged. Sometimes it's a bug because it's inconsistent; sometimes it's
a bug because the other copy lacks the feature for which the modification
was made in the first copy.

So, the lesson is: Try hard to avoid code duplication!

* pygnulib/GLConfig.py line 802:
Typo: Autmake -> Automake.


Bruno

Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Collin Funk]

Hey Bruno, thanks for the feedback.

On 2/23/24 5:08 AM, Bruno Haible wrote:
> Just three small nits that you might want to revisit:
> 
> * gnulib-tool.py line 610:
> +    if modules != None and "tests" in mode and gnu_make:
>         ^^^^^^^^^^^^^^^^^^^
> What is the rationale for this condition? It is not present in the original. Can
> it be removed?

I added this condition in an attempt to mirror changes made in commit
60e8b9303d8ce312bb2322d4801ed08678f93d1e which was marked in 
gnulib-tool.py.TODO. It seems correct to me. Here is the original
change from gnulib-tool. I am not the best when it comes to
reading/writing shell scripts so I could always be wrong. :)

* gnulib-tool Line 1493:
+  case $mode,$gnu_make in
+    *test*,true)
+      echo "gnulib-tool: --gnu-make not supported when including tests"
+      func_exit 1;;
+  esac

The "mode" variable is initialized to None and then set in the
conditionals before the one I added depending on the arguments. Since
it may still be None it must be checked before treating it as a string
to avoid an uncaught exception.

> * pygnulib/GLEmiter.py lines 732, 739:
> +                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet1)
> +                        allsnippets += re.sub(r'^if (.*)', r'ifneq (,$(\1))', amsnippet2)
> 
> The regular expression is duplicated. The problem with duplicated code is
> that it very often leads to bugs in the future: Future code maintainers
> will see one copy of the piece of code, modify it, and leave the other
> copy unchanged. Sometimes it's a bug because it's inconsistent; sometimes it's
> a bug because the other copy lacks the feature for which the modification
> was made in the first copy.
> 
> So, the lesson is: Try hard to avoid code duplication!

I agree. I will have to revisit that code a few times to handle the
other items in the TODO. I'll try to think of a good way to clean that
up. An idea that makes sense to me currently is storing the regular
expressions in the "convert_to_gnu_make" variable as a tuple/list if
"gnu_make" is set. If it is not set, the variable can be set to None.
This would essentially let it act as a boolean:

if convert_to_gnu_make != None:
   do something

But would also allow it to be used as the regular expression
arguments:

allsnippets += re.sub(convert_to_gnu_make[0],
                      convert_to_gnu_make[1], amsnippet1)

That is just an idea that I have now. I might discover something
better later down the line. In the future it would be nice to cleanup
many different parts of the code. One thing that annoys me personally
is comparing to none using "!=" instead of "is not". This is
recommended against in PEP 8, "Comparisons to singletons like None
should always be done with is or is not, never the equality
operators." [1]. I also am a big fan of using Pathlib for portable,
high-level path construction [2]. Currently we use a few functions
wrapping the os.path versions which I find less readable. Larger
changes like this are probably best left until gnulib-tool.py catches
up in features to gnulib-tool though. They would probably make it
harder to follow the git history.

> * pygnulib/GLConfig.py line 802:
> Typo: Autmake -> Automake.

Oops. I also noticed that I forgot the remove the --gnu-make option
from the missing arguments gnulib-tool.py.TODO. It seems that you
remembered for me. Thanks!

[1] https://peps.python.org/pep-0008/#programming-recommendations
[2] https://docs.python.org/3/library/pathlib.html

Collin

Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Bruno Haible]

Hi Collin,

> > * gnulib-tool.py line 610:
> > +    if modules != None and "tests" in mode and gnu_make:
> >         ^^^^^^^^^^^^^^^^^^^
> > What is the rationale for this condition? It is not present in the original. Can
> > it be removed?
> 
> I added this condition in an attempt to mirror changes made in commit
> 60e8b9303d8ce312bb2322d4801ed08678f93d1e which was marked in 
> gnulib-tool.py.TODO. It seems correct to me. Here is the original
> change from gnulib-tool. I am not the best when it comes to
> reading/writing shell scripts so I could always be wrong. :)
> 
> * gnulib-tool Line 1493:
> +  case $mode,$gnu_make in
> +    *test*,true)
> +      echo "gnulib-tool: --gnu-make not supported when including tests"
> +      func_exit 1;;
> +  esac
> 
> The "mode" variable is initialized to None and then set in the
> conditionals before the one I added depending on the arguments. Since
> it may still be None it must be checked before treating it as a string
> to avoid an uncaught exception.

I.e. you meant to write
  mode != None
not
  modules != None
?

> > So, the lesson is: Try hard to avoid code duplication!
> 
> I agree. I will have to revisit that code a few times to handle the
> other items in the TODO. I'll try to think of a good way to clean that
> up. An idea that makes sense to me currently is storing the regular
> expressions in the "convert_to_gnu_make" variable as a tuple/list if
> "gnu_make" is set. If it is not set, the variable can be set to None.
> This would essentially let it act as a boolean:
> 
> if convert_to_gnu_make != None:
>    do something
> 
> But would also allow it to be used as the regular expression
> arguments:
> 
> allsnippets += re.sub(convert_to_gnu_make[0],
>                       convert_to_gnu_make[1], amsnippet1)

Sounds good.

> One thing that annoys me personally
> is comparing to none using "!=" instead of "is not". This is
> recommended against in PEP 8, "Comparisons to singletons like None
> should always be done with is or is not, never the equality
> operators." [1]

I recall having seen this warning. Was it from python itself?
Or from pycodestyle or pylint? (Cf. the comments at gnulib-tool.py
line 29..33)

But I don't recall whether this warning was justified or bogus.

> I also am a big fan of using Pathlib for portable,
> high-level path construction [2]. Currently we use a few functions
> wrapping the os.path versions which I find less readable. Larger
> changes like this are probably best left until gnulib-tool.py catches
> up in features to gnulib-tool though. They would probably make it
> harder to follow the git history.

Yes, larger style changes like this one are probably best postponed
for the moment.

And I'm not convinced Pathlib is a win here, because
  - we are not targetting native Windows, only Unix platforms (that
    includes Cygwin),
  - it appears to have some extra learning curve besides 'import os'.

Bruno

Re: [PATCH] gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Collin Funk]

On 2/23/24 3:51 PM, Bruno Haible wrote:
> I.e. you meant to write
>   mode != None
> not
>   modules != None
> ?

Oops. I'm not sure how I missed this in your original email as well...
You are correct. It should be "mode != None". I should have a patch
ready in a bit to fix another item in the gnulib-tool.py.TODO. If you
would like I could fix that typo along with it. Or you can commit the
change yourself if you would prefer. Thanks for the catch.

> I recall having seen this warning. Was it from python itself?
> Or from pycodestyle or pylint? (Cf. the comments at gnulib-tool.py
> line 29..33)
> 
> But I don't recall whether this warning was justified or bogus.

I'd imagine that most of the code linters and style checkers would
warn about it. It seems to be error code E711 for pycodestyle which is
disabled by the command in that comment [1]. I mentioned it more out
of habit though. I tend to find the Python tools confusing so I mostly
avoid them (which is probably a bad habit). There is like 20 separate
linters so I never know which one is the most "standard" if that makes
sense...

The difference between "==" and "is" can cause actual bugs, but those
typically don't involve comparisons to None. It is hard for me to
imagine a situation where it would actually cause problems in that
case. I'm not super familiar with Python though, so maybe someone who
knows better can give their thoughts. Here is a silly example to show
the difference, but of course you would never write it without
intending the consequences. 

class Example:
    def __eq__(self, other):
        return True

def main():
    one = Example()
    print(f"one == None: {one == None}")
    print(f"one == 0:    {one == 0}")
    print(f"one is None: {one is None}")

if __name__ == "__main__":
    main()

[collin@debian gnulib]$ ./test.py 
one == None: True
one == 0:    True
one is None: False

> And I'm not convinced Pathlib is a win here, because
>   - we are not targetting native Windows, only Unix platforms (that
>     includes Cygwin),
>   - it appears to have some extra learning curve besides 'import os'.

Sure, that makes sense. There is definitely less benefit if you are not
concerned with Windows paths. I'll stick to os.path since it seems
everyone is comfortable with it.

[1] https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes

gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Collin Funk]

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

Hi Bruno,

I've attached two patches. The first one fixes an item in the
gnulib-tool.py.TODO file. Previously the "Generated by gnulib-tool"
comment at the top of the Makefile would be on one line. This splits
it into multiple lines so that it can be used with git send-email and
versions of AWK with line length limits. I again used the Emacs
merge-gnulib script and diffed the two Makefiles until they were the
same. This required reordering a few of the options and such. The
regular gnulib-tool prints a few more options that are not yet
implemented by gnulib-tool.py. I've left a comment with a FIXME tag
with them ordered properly.

On 2/23/24 3:51 PM, Bruno Haible wrote:
> I.e. you meant to write
>   mode != None
> not
>   modules != None
> ?

The second fixes this typo. Thanks for noticing it. Who knows how
long it would have taken to cause an exception otherwise... I made
sure to give you credit in the ChangeLog entry.

Thanks,
Collin

[-- Attachment #2: 0001-gnulib-tool.py-Follow-gnulib-tool-changes-part-28.patch --]
[-- Type: text/x-patch, Size: 9390 bytes --]

From 52abea086e7c33723e255202a121d555a41b652d Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Fri, 23 Feb 2024 20:52:15 -0800
Subject: [PATCH 1/2] gnulib-tool.py: Follow gnulib-tool changes, part 28.

Follow gnulib-tool change
2018-07-17  Paul Eggert  <eggert@cs.ucla.edu>
gnulib-tool: limit line length for git send-email

* pygnulib/GLImport.py (GLImport.actioncmd): Break actioncmd into
multiple lines. Reorder emitting of arguments to match gnulib-tool. Emit
"--witness-c-macro" instead of "--witness_c_macro". Emit "--po-domain"
instead of "--podomain". Document ordering of unimplemented options. Add
updated comments documenting line length limitations of git send-email
and some implementations of AWK.
* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Remove comment which
was moved to pygnulib/GLImport.py. Remove length limitation on actioncmd
since it now spans multiple lines.
---
 ChangeLog            | 16 +++++++++
 gnulib-tool.py.TODO  | 11 ------
 pygnulib/GLEmiter.py |  8 +----
 pygnulib/GLImport.py | 82 +++++++++++++++++++++++++-------------------
 4 files changed, 63 insertions(+), 54 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1900d90186..e439ce6389 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,19 @@
+2024-02-23  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Follow gnulib-tool changes, part 28.
+	Follow gnulib-tool change
+	2018-07-17  Paul Eggert  <eggert@cs.ucla.edu>
+	gnulib-tool: limit line length for git send-email
+	* pygnulib/GLImport.py (GLImport.actioncmd): Break actioncmd into
+	multiple lines. Reorder emitting of arguments to match gnulib-tool. Emit
+	"--witness-c-macro" instead of "--witness_c_macro". Emit "--po-domain"
+	instead of "--podomain". Document ordering of unimplemented options. Add
+	updated comments documenting line length limitations of git send-email
+	and some implementations of AWK.
+	* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Remove comment which
+	was moved to pygnulib/GLImport.py. Remove length limitation on actioncmd
+	since it now spans multiple lines.
+
 2024-02-23  Bruno Haible  <bruno@clisp.org>
 
 	DEPENDENCIES: Update entry about gperf.
diff --git a/gnulib-tool.py.TODO b/gnulib-tool.py.TODO
index 3313839a32..c72746941e 100644
--- a/gnulib-tool.py.TODO
+++ b/gnulib-tool.py.TODO
@@ -779,17 +779,6 @@ Date:   Mon Sep 3 21:19:16 2018 +0200
 
 --------------------------------------------------------------------------------
 
-commit a1276e5bf2286afec7b0445040be05cad858cdd1
-Author: Paul Eggert <eggert@cs.ucla.edu>
-Date:   Tue Jul 17 15:20:39 2018 -0700
-
-    gnulib-tool: limit line length for git send-email
-
-    * gnulib-tool (func_import): Break actioncmd log line
-    into multiple lines.
-
---------------------------------------------------------------------------------
-
 commit 589e96475f8f2d21a83405ab0672ce95091b80e5
 Author: Bruno Haible <bruno@clisp.org>
 Date:   Fri Dec 29 00:29:23 2017 +0100
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index 6de9aba68a..56e8156761 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -668,13 +668,7 @@ AC_DEFUN([%V1%_LIBSOURCES], [
             emit += "## Process this file with automake to produce Makefile.in.\n"
         emit += self.copyright_notice()
         if actioncmd:
-            # The maximum line length (excluding the terminating newline) of
-            # any file that is to be preprocessed by config.status is 3070.
-            # config.status uses awk, and the HP-UX 11.00 awk fails if a line
-            # has length >= 3071; similarly, the IRIX 6.5 awk fails if a line
-            # has length >= 3072.
-            if len(actioncmd) <= 3000:
-                emit += "# Reproduce by: %s\n" % actioncmd
+            emit += "# Reproduce by:\n%s\n" % actioncmd
         emit += '\n'
         uses_subdirs = False
 
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index e01cb9f63e..c69a33deb7 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -374,63 +374,73 @@ class GLImport(object):
         vc_files = self.config.checkVCFiles()
         verbose = self.config.getVerbosity()
 
-        # Create command-line invocation comment.
-        actioncmd = 'gnulib-tool --import'
-        actioncmd += ' --dir=%s' % destdir
-        for localdir in localpath:
-            actioncmd += ' --local-dir=%s' % localdir
-        actioncmd += ' --lib=%s' % libname
-        actioncmd += ' --source-base=%s' % sourcebase
-        actioncmd += ' --m4-base=%s' % m4base
+        # Command-line invocation printed in a comment in generated gnulib-cache.m4.
+        actioncmd = '# gnulib-tool --import'
+
+        # Break the action command log into multiple lines.
+        # Emacs puts some gnulib-tool log lines in its source repository, and
+        # git send-email rejects patch lines longer than 998 characters.
+        # Also, config.status uses awk, and the HP-UX 11.00 awk fails if a
+        # line has length >= 3071; similarly, the IRIX 6.5 awk fails if a
+        # line has length >= 3072.
+        if len(localpath) > 0:
+            actioncmd += ''.join([f" \\\n#  --local-dir={x}" for x in localpath])
+        actioncmd += ' \\\n#  --lib=%s' % libname
+        actioncmd += ' \\\n#  --source-base=%s' % sourcebase
+        actioncmd += ' \\\n#  --m4-base=%s' % m4base
         if pobase:
-            actioncmd += ' --po-base=%s' % pobase
-        actioncmd += ' --doc-base=%s' % docbase
-        actioncmd += ' --tests-base=%s' % testsbase
-        actioncmd += ' --aux-dir=%s' % auxdir
+            actioncmd += ' \\\n#  --po-base=%s' % pobase
+        actioncmd += ' \\\n#  --doc-base=%s' % docbase
+        actioncmd += ' \\\n#  --tests-base=%s' % testsbase
+        actioncmd += ' \\\n#  --aux-dir=%s' % auxdir
         if self.config.checkInclTestCategory(TESTS['tests']):
-            actioncmd += ' --with-tests'
+            actioncmd += ' \\\n#  --with-tests'
         if self.config.checkInclTestCategory(TESTS['obsolete']):
-            actioncmd += ' --with-obsolete'
+            actioncmd += ' \\\n#  --with-obsolete'
         if self.config.checkInclTestCategory(TESTS['c++-test']):
-            actioncmd += ' --with-c++-tests'
+            actioncmd += ' \\\n#  --with-c++-tests'
         if self.config.checkInclTestCategory(TESTS['longrunning-test']):
-            actioncmd += ' --with-longrunning-tests'
+            actioncmd += ' \\\n#  --with-longrunning-tests'
         if self.config.checkInclTestCategory(TESTS['privileged-test']):
-            actioncmd += ' --with-privileged-test'
+            actioncmd += ' \\\n#  --with-privileged-test'
         if self.config.checkInclTestCategory(TESTS['unportable-test']):
-            actioncmd += ' --with-unportable-tests'
+            actioncmd += ' \\\n#  --with-unportable-tests'
         if self.config.checkInclTestCategory(TESTS['all-test']):
-            actioncmd += ' --with-all-tests'
-        for module in avoids:
-            actioncmd += ' --avoid=%s' % module
+            actioncmd += ' \\\n#  --with-all-tests'
         if lgpl:
             if lgpl == True:
-                actioncmd += ' --lgpl'
+                actioncmd += ' \\\n#  --lgpl'
             else:  # if lgpl != True
-                actioncmd += ' --lgpl=%s' % lgpl
+                actioncmd += ' \\\n#  --lgpl=%s' % lgpl
         if gnu_make:
-            actioncmd += ' --gnu-make'
+            actioncmd += ' \\\n#  --gnu-make'
         if makefile_name:
-            actioncmd += ' --makefile-name=%s' % makefile_name
+            actioncmd += ' \\\n#  --makefile-name=%s' % makefile_name
+        # FIXME: Add the following options in this order when implemented.
+        # --tests-makefile-name
+        # --automake-subdir
+        # --automake-subdir-tests
         if conddeps:
-            actioncmd += ' --conditional-dependencies'
+            actioncmd += ' \\\n#  --conditional-dependencies'
         else:  # if not conddeps
-            actioncmd += ' --no-conditional-dependencies'
+            actioncmd += ' \\\n#  --no-conditional-dependencies'
         if libtool:
-            actioncmd += ' --libtool'
+            actioncmd += ' \\\n#  --libtool'
         else:  # if not libtool
-            actioncmd += ' --no-libtool'
-        actioncmd += ' --macro-prefix=%s' % macro_prefix
+            actioncmd += ' \\\n#  --no-libtool'
+        actioncmd += ' \\\n#  --macro-prefix=%s' % macro_prefix
         if podomain:
-            actioncmd = ' --podomain=%s' % podomain
+            actioncmd = ' \\\n#  --po-domain=%s' % podomain
         if witness_c_macro:
-            actioncmd += ' --witness_c_macro=%s' % witness_c_macro
+            actioncmd += ' \\\n#  --witness-c-macro=%s' % witness_c_macro
         if vc_files == True:
-            actioncmd += ' --vc-files'
+            actioncmd += ' \\\n#  --vc-files'
         elif vc_files == False:
-            actioncmd += ' --no-vc-files'
-        actioncmd += ' '  # Add a space
-        actioncmd += ' '.join(modules)
+            actioncmd += ' \\\n#  --no-vc-files'
+        if len(avoids) > 0:
+            actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in sorted(avoids)])
+        if len(modules) > 0:
+            actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])
         return actioncmd
 
     def relative_to_destdir(self, dir):
-- 
2.39.2


[-- Attachment #3: 0002-gnulib-tool.py-Fix-conditional-checking-the-incorrec.patch --]
[-- Type: text/x-patch, Size: 1834 bytes --]

From da6528cec124976659bac165a4b817337eccb9d2 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Fri, 23 Feb 2024 21:12:39 -0800
Subject: [PATCH 2/2] gnulib-tool.py: Fix conditional checking the incorrect
 variable.

Reported by Bruno Haible <bruno@clisp.org> in
<https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00207.html>.

* gnulib-tool.py (main): Fix incorrect conditional. Check that mode, not
modules, is not None before treating it as a string.
---
 ChangeLog      | 8 ++++++++
 gnulib-tool.py | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index e439ce6389..468020ed5b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-02-23  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Fix conditional checking the incorrect variable.
+	Reported by Bruno Haible <bruno@clisp.org> in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00207.html>.
+	* gnulib-tool.py (main): Fix incorrect conditional. Check that mode, not
+	modules, is not None before treating it as a string.
+
 2024-02-23  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Follow gnulib-tool changes, part 28.
diff --git a/gnulib-tool.py b/gnulib-tool.py
index 599dbe0984..e168e8fc91 100755
--- a/gnulib-tool.py
+++ b/gnulib-tool.py
@@ -607,7 +607,7 @@ def main():
     if cmdargs.pobase == None and cmdargs.podomain != None:
         message = '%s: warning: --po-domain has no effect without a --po-base option\n' % constants.APP['name']
         sys.stderr.write(message)
-    if modules != None and "tests" in mode and gnu_make:
+    if  mode != None and "tests" in mode and gnu_make:
         message = '%s: --gnu-make not supported when including tests\n' % constants.APP['name']
         sys.stderr.write(message)
         sys.exit(1)
-- 
2.39.2

Re: gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Bruno Haible]

Hi Collin,

> > I.e. you meant to write
> >   mode != None
> > not
> >   modules != None
> > ?
> 
> The second fixes this typo. Thanks for noticing it.

But there's another typo in the same line: The original code

  case $mode,$gnu_make in
    *test*,true)
      echo "gnulib-tool: --gnu-make not supported when including tests"
      func_exit 1;;
  esac

has the intent to match the $mode values
  create-testdir
  create-megatestdir
  test
  megatest
But these do not contain the substring "tests".

Also, please can we stick with the syntax 'foo', not "foo", for literal
strings. Outside of gnulib, both syntaxes seem to be in use. But when I
search where the literal string 'foo' occurs, I don't want to make 2
searches (for 'foo' and for "foo"), nor a regex search (for ['"]foo['"]).
Simple things should remain simple. A certain canonical way to denote
literal strings is necessary for this. (Btw, JavaScript has the same
problem.)


2024-02-24  Bruno Haible  <bruno@clisp.org>

	gnulib-tool.py: Further fix last commit.
	* gnulib-tool.py (main): Make the mode test match for 'create-testdir',
	'create-megatestdir', 'test', 'megatest'.

diff --git a/gnulib-tool.py b/gnulib-tool.py
index e168e8fc91..1df790c496 100755
--- a/gnulib-tool.py
+++ b/gnulib-tool.py
@@ -607,7 +607,7 @@ def main():
     if cmdargs.pobase == None and cmdargs.podomain != None:
         message = '%s: warning: --po-domain has no effect without a --po-base option\n' % constants.APP['name']
         sys.stderr.write(message)
-    if  mode != None and "tests" in mode and gnu_make:
+    if mode != None and 'test' in mode and gnu_make:
         message = '%s: --gnu-make not supported when including tests\n' % constants.APP['name']
         sys.stderr.write(message)
         sys.exit(1)

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Bruno Haible]

Hi Collin,

> The first one fixes an item in the
> gnulib-tool.py.TODO file. Previously the "Generated by gnulib-tool"
> comment at the top of the Makefile would be on one line.

Thanks! Applied.

Only one question on this one:

> +        if len(avoids) > 0:
> +            actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in sorted(avoids)])
> +        if len(modules) > 0:
> +            actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])

The sorted(...) instruction is not present in gnulib-tool lines 5647..5652.
Why is it needed? Can you make a quick test with a gnulib-tool invocation
that has several --avoid=... arguments and several module arguments that
are not in increasing alphabetical order?

Bruno

Re: gnulib-tool.py: Follow gnulib-tool changes, part 27.

[Dima Pasechnik]

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

Hi Bruno,

The pythonic way is

    mode is not None

rather than 

    mode != None

(the reason is None is an object)

Just in case,
Dima

On 24 February 2024 23:25:51 GMT, Bruno Haible <bruno@clisp.org> wrote:
>Hi Collin,
>
>> > I.e. you meant to write
>> >   mode != None
>> > not
>> >   modules != None
>> > ?
>> 
>> The second fixes this typo. Thanks for noticing it.
>
>But there's another typo in the same line: The original code
>
>  case $mode,$gnu_make in
>    *test*,true)
>      echo "gnulib-tool: --gnu-make not supported when including tests"
>      func_exit 1;;
>  esac
>
>has the intent to match the $mode values
>  create-testdir
>  create-megatestdir
>  test
>  megatest
>But these do not contain the substring "tests".
>
>Also, please can we stick with the syntax 'foo', not "foo", for literal
>strings. Outside of gnulib, both syntaxes seem to be in use. But when I
>search where the literal string 'foo' occurs, I don't want to make 2
>searches (for 'foo' and for "foo"), nor a regex search (for ['"]foo['"]).
>Simple things should remain simple. A certain canonical way to denote
>literal strings is necessary for this. (Btw, JavaScript has the same
>problem.)
>
>
>2024-02-24  Bruno Haible  <bruno@clisp.org>
>
>	gnulib-tool.py: Further fix last commit.
>	* gnulib-tool.py (main): Make the mode test match for 'create-testdir',
>	'create-megatestdir', 'test', 'megatest'.
>
>diff --git a/gnulib-tool.py b/gnulib-tool.py
>index e168e8fc91..1df790c496 100755
>--- a/gnulib-tool.py
>+++ b/gnulib-tool.py
>@@ -607,7 +607,7 @@ def main():
>     if cmdargs.pobase == None and cmdargs.podomain != None:
>         message = '%s: warning: --po-domain has no effect without a --po-base option\n' % constants.APP['name']
>         sys.stderr.write(message)
>-    if  mode != None and "tests" in mode and gnu_make:
>+    if mode != None and 'test' in mode and gnu_make:
>         message = '%s: --gnu-make not supported when including tests\n' % constants.APP['name']
>         sys.stderr.write(message)
>         sys.exit(1)
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 2833 bytes --]

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Collin Funk]

Hi Bruno,

Thanks for fixing the typo in the other email. I'll remember to use
'foo' instead of "foo". That is a personal habit of mine but I now
realize that it goes against all of the existing code...

On 2/24/24 3:42 PM, Bruno Haible wrote:
> The sorted(...) instruction is not present in gnulib-tool lines 5647..5652.
> Why is it needed? Can you make a quick test with a gnulib-tool invocation
> that has several --avoid=... arguments and several module arguments that
> are not in increasing alphabetical order?

Sure. I'm using the admin/merge-gnulib script from master to test
this. Here is the diff with the two sorted instructions removed.

diff --git a/gnulib.mk.in.python b/gnulib.mk.in.shell
index a86535fd700..a718c17c0e8 100644
--- a/gnulib.mk.in.python
+++ b/gnulib.mk.in.shell
@@ -1,5 +1,5 @@
 ## DO NOT EDIT! GENERATED AUTOMATICALLY!
-# Copyright (C) 2024 Free Software Foundation, Inc.
+# Copyright (C) 2002-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
@@ -173,8 +173,8 @@
 #  timer-time \
 #  timespec-add \
 #  timespec-sub \
-#  update-copyright \
 #  unlocked-io \
+#  update-copyright \
 #  utimensat \
 #  vla \
 #  warnings \

Now the diff with the two sorted instructions added again:

diff --git a/gnulib.mk.in.python-with-sort b/gnulib.mk.in.shell
index 15d15970051..a718c17c0e8 100644
--- a/gnulib.mk.in.python-with-sort
+++ b/gnulib.mk.in.shell
@@ -1,5 +1,5 @@
 ## DO NOT EDIT! GENERATED AUTOMATICALLY!
-# Copyright (C) 2024 Free Software Foundation, Inc.
+# Copyright (C) 2002-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

So that is the original reason I added the sorted() functions.
Unrelated, but gnulib-tool.py does not use a year range for copyright.
I'm not sure if it is intentional or not.

Anyways, upon further inspection not all of the gnulib-modules are
sorted in merge-gnulib. When "unlocked-io" was added to Emacs it was
placed after "update-copyright" [1]. I assume that they are sorted
somewhere before the actioncmd step in gnulib-tool. Let me experiment
with the --avoid modules and I'll reply if I notice anything.

[1] https://git.savannah.gnu.org/cgit/emacs.git/tree/admin/merge-gnulib#n50

Collin

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Collin Funk]

On 2/24/24 4:47 PM, Collin Funk wrote:
> Anyways, upon further inspection not all of the gnulib-modules are
> sorted in merge-gnulib. When "unlocked-io" was added to Emacs it was
> placed after "update-copyright" [1]. I assume that they are sorted
> somewhere before the actioncmd step in gnulib-tool. Let me experiment
> with the --avoid modules and I'll reply if I notice anything.

I think that I have confirmed this behavior by changing the order of
"GNULIB_MODULES" and "AVOIDED_MODULES" in Emacs admin/merge-gnulib
script. Changing GNULIB_MODULES so the first modules are ordered 1. dup2,
2. alignasof, 3. copy-file-range, 4. alloca-opt, ... and
AVOIDED_MODULES so the first modules are ordered 1. chmod, 2. btowc,
3. access produces the following diff:

diff --git a/lib/gnulib.mk.in b/lib/gnulib.mk.in
index 711ddcf1260..f16c018b728 100644
--- a/lib/gnulib.mk.in
+++ b/lib/gnulib.mk.in
@@ -34,9 +34,9 @@
 #  --no-libtool \
 #  --macro-prefix=gl \
 #  --no-vc-files \
-#  --avoid=access \
-#  --avoid=btowc \
 #  --avoid=chmod \
+#  --avoid=btowc \
+#  --avoid=access \
 #  --avoid=close \
 #  --avoid=crypto/af_alg \
 #  --avoid=dup \

So the --avoid modules are emitted in the order they are passed to
gnulib-tool, but the actual modules will be alphabetically sorted.
Therefore, I think the correct code would be:

if len(avoids) > 0:
    actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in avoids])
if len(modules) > 0:
    actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])

Seems sort of strange but it produces the correct output for that
test.

Collin

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Bruno Haible]

Collin Funk wrote:
> So the --avoid modules are emitted in the order they are passed to
> gnulib-tool, but the actual modules will be alphabetically sorted.
> Therefore, I think the correct code would be:
> 
> if len(avoids) > 0:
>     actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in avoids])
> if len(modules) > 0:
>     actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])

OK. For the --avoid options, things are clear now.

Regarding the modules to include:
> Here is the diff with the two sorted instructions removed.

So, it looks like the original gnulib-tool does some sorting of the
module names that gnulib-tool.py does not do yet, in some earlier
processing steps. Since this can affect other parts of the output,
it would be good to have the sorting at the same processing stage.

None of the 'sort' invocations in gnulib-tool are covered by an
entry in the gnulib-tool.py.TODO file. Therefore the most promising
approach to finding the cause of the difference is to
  - go through all 'sort' invocations in gnulib-tool,
  - find the corresponding place in pygnulib/ and see whether
    sorting happens there as well or has been forgotten.

Bruno

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Collin Funk]

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

Hi Bruno,

On 2/24/24 5:25 PM, Bruno Haible wrote:
> None of the 'sort' invocations in gnulib-tool are covered by an
> entry in the gnulib-tool.py.TODO file. Therefore the most promising
> approach to finding the cause of the difference is to
>   - go through all 'sort' invocations in gnulib-tool,
>   - find the corresponding place in pygnulib/ and see whether
>     sorting happens there as well or has been forgotten.

Thank you for the advice and for looking over all of these patches. I
think that this patch should be a solution to the issue as long as I
am understanding gnulib-tool correctly.

From what I could understand, it seems that upon seeing that the mode
is "import", the specified_modules is set with the unsorted list of
modules [1]. Then that variable is sorted before it is used [2]. Could
you confirm that I am understanding this correctly?

If so, I feel that this patch should be the correct way to handle it.
My initial ideas where to sort the modules in main() before the
GLConfig object was created, or to modify the GLConfig class's methods
which are used to set the modules in use. I think the second option is
less likely to be broken by future changes. The changes involve using
a set and sorted() to substitute `sort -u'.

[1] https://git.savannah.gnu.org/cgit/gnulib.git/tree/gnulib-tool#n5085
[2] https://git.savannah.gnu.org/cgit/gnulib.git/tree/gnulib-tool#n5268

[-- Attachment #2: 0001-gnulib-tool.py-Handle-the-sorting-of-modules-correct.patch --]
[-- Type: text/x-patch, Size: 3316 bytes --]

From 93d4e7698afc92e3b257c2a0b2050540357edc96 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sat, 24 Feb 2024 19:05:51 -0800
Subject: [PATCH] gnulib-tool.py: Handle the sorting of modules correctly.

See discussion at
<https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00219.html>

* pygnulib/GLConfig.py (GLConfig.addModule, CLConfig.setModules): Use a
sorted set when adding modules to replicate "sort -u" used by
gnulib-tool.
* pygnulib/GLImport.py (GLImport.actioncmd): Don't sort modules when
constructing actioncmd as GLConfig handles it for us.
---
 ChangeLog            | 11 +++++++++++
 pygnulib/GLConfig.py |  6 ++++--
 pygnulib/GLImport.py |  4 ++--
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 1910933eb7..a616db7e09 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2024-02-24  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Handle the sorting of modules correctly.
+	See discussion at
+	<https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00219.html>
+	* pygnulib/GLConfig.py (GLConfig.addModule, CLConfig.setModules): Use a
+	sorted set when adding modules to replicate "sort -u" used by
+	gnulib-tool.
+	* pygnulib/GLImport.py (GLImport.actioncmd): Don't sort modules when
+	constructing actioncmd as GLConfig handles it for us.
+
 2024-02-24  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Follow gnulib-tool changes, part 28.
diff --git a/pygnulib/GLConfig.py b/pygnulib/GLConfig.py
index bdebf243cc..03da6f8e3c 100644
--- a/pygnulib/GLConfig.py
+++ b/pygnulib/GLConfig.py
@@ -476,7 +476,7 @@ class GLConfig(object):
         '''Add the module to the modules list.'''
         if type(module) is str:
             if module not in self.table['modules']:
-                self.table['modules'] += [module]
+                self.table['modules'] = sorted(list(set(self.table['modules'] + [module])))
         else:  # if module has not str type
             raise TypeError('module must be a string, not %s'
                             % type(module).__name__)
@@ -499,7 +499,9 @@ class GLConfig(object):
         if type(modules) is list or type(modules) is tuple:
             old_modules = self.table['modules']
             self.table['modules'] = list()
-            for module in modules:
+            # Canonicalize the list of specified modules.
+            # sort -u
+            for module in sorted(set(modules)):
                 try:  # Try to add each module
                     self.addModule(module)
                 except TypeError as error:
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index c69a33deb7..9b08e7e4b6 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -438,9 +438,9 @@ class GLImport(object):
         elif vc_files == False:
             actioncmd += ' \\\n#  --no-vc-files'
         if len(avoids) > 0:
-            actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in sorted(avoids)])
+            actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in avoids])
         if len(modules) > 0:
-            actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])
+            actioncmd += ''.join([f" \\\n#  {x}" for x in modules])
         return actioncmd
 
     def relative_to_destdir(self, dir):
-- 
2.39.2

Re: Python != None

[Bruno Haible]

Collin Funk wrote:
> One thing that annoys me personally
> is comparing to none using "!=" instead of "is not". This is
> recommended against in PEP 8, "Comparisons to singletons like None
> should always be done with is or is not, never the equality
> operators." [1].

Dima Pasechnik wrote:
> The pythonic way is
> 
>     mode is not None
> 
> rather than 
> 
>     mode != None
> 
> (the reason is None is an object)

Summarizing the arguments for "is not None":
  1- Clarity and readability, best practice, PEP 8.
  2- Identity comparison vs. value comparison.
  3- None is a singleton.
  4- Prevents wrong results if an __eq__ method has been incorrectly coded.

Here's my take on it.

1- is clearly subjective. Clarity is context dependent. Etc.
2- Yes, https://docs.python.org/3.12/reference/expressions.html#comparisons
   clearly explains the difference between == and 'is'.
   (It's more or less like the difference between EQUAL and EQ in Lisp.)
3- None is a singleton, but it nonetheless supports both == and 'is'. So,
   this is a weak argument.
4- This is one of the weakest possible arguments.

The style warnings about "!= None" in pycodestyle and/or pylint are
relativized by this warning in Python itself:

  >>> '' != None
  True
  >>> '' is not None
  <stdin>:1: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  True

So, if Python itself warns about some code that follows PEP 8...
it means that we need to think carefully, rather than blindly apply
that PEP 8 rule.

I think a better rule, in particular in the context of gnulib-tool.py,
is to observe that each variable has a certain set of possible values
(even though this set of values is not explicitly stated in the source
code), and with this set of possible values comes a comparison operator.

So, what I mean is:

  * If a variable can only contain objects like GLModule instances or None,
    then 'is' (pointer comparison) is the right choice for this variable.

  * If a variable can only contain strings or None, then '==' (value
    comparison) is the right choice for this variable, since comparing
    strings with 'is' is unreliable:

      >>> x = "ab"
      >>> y = "ab"
      >>> x is y
      True
      
      >>> x = "abc"[:2]
      >>> y = "abx"[:2]
      >>> x is y
      False

  * If a variable can only contain a list or None, then '==' (value comparison)
    is the right choice for this variable.

    >>> ["a"] == ["a"]
    True
    >>> ["a"] is ["a"]
    False

So, depending on the variable:
   mode != None                       OK
   modules != None                    OK
   module != None                     not OK, better write:   module is not None

Bruno

Re: Python 'strings'

[Bruno Haible]

Summarizing the rationale to use 'abc' as string syntax:

* PEP 8 https://peps.python.org/pep-0008/#string-quotes
  says "In Python, single-quoted strings and double-quoted strings are
  the same. This PEP does not make a recommendation for this. Pick a rule
  and stick to it."

* When I search where the literal string 'foo' occurs, I don't want to make 2
  searches (for 'foo' and for "foo"), nor a regex search (for ['"]foo['"]).
  Simple things should remain simple. A certain canonical way to denote
  literal strings is necessary for this. (Btw, JavaScript has the same
  problem.)

* The Python interpreter prints strings with single-quotes:

  >>> "abc"
  'abc'

So, that makes the single-quotes the natural choice.

Bruno

Re: Python 'strings'

[Collin Funk]

Hi Bruno,

On 2/25/24 4:02 AM, Bruno Haible wrote:
> * The Python interpreter prints strings with single-quotes:
> 
>   >>> "abc"
>   'abc'
> 
> So, that makes the single-quotes the natural choice.

Interesting find. I don't have super strong feelings on the matter. I
just think that it is best that code is consistent. I'm more used to
"abc", but since nearly all of the exist code uses 'abc' I agree that
this choice is better.

Thanks,
Collin

Re: Python != None

[Collin Funk]

Hi Bruno,

On 2/25/24 3:57 AM, Bruno Haible wrote:
> The style warnings about "!= None" in pycodestyle and/or pylint are
> relativized by this warning in Python itself:
> 
>   >>> '' != None
>   True
>   >>> '' is not None
>   <stdin>:1: SyntaxWarning: "is not" with a literal. Did you mean "!="?
>   True

Interesting. I'm not too picky about the convention that we use. Your
convention seems fine with me. It would be nice to use Eglot with
Emacs as it is helpful for finding bugs, but currently it is spammed
with warnings about comparison. This makes it nearly impossible to
find actual warnings when the exist. I need to do some research on how
to best fix that on a per-project basis. I would hope that it just
involves a simple configuration file that takes the options we have at
the top of gnulib-tool.py.

Collin

Re: Python != None

[Collin Funk]

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

On 2/25/24 11:29 AM, Collin Funk wrote:
> I would hope that it just
> involves a simple configuration file that takes the options we have at
> the top of gnulib-tool.py.

Adding the two attached files to the root directory of gnulib should
adjust the warnings to the agreed upon coding style (as far as
pycodestyle and pylint are concerned). It works with Emacs and Eglot
using pylsp. I assume other editors should work similarly.

Not sure if they are worth adding to the repository but I figured I
would share them just incase anyone else wants to work on stuff in the
future.

More information about these files here:
https://pycodestyle.pycqa.org/en/latest/intro.html#configuration
https://pylint.readthedocs.io/en/stable/user_guide/configuration/index.html#

Collin

[-- Attachment #2: setup.cfg --]
[-- Type: text/plain, Size: 156 bytes --]

# setup.cfg

[pycodestyle]
ignore = E265,W503,E241,E711,E712,E201,E202,E221
max-line-length = 136
statistics = True

# Local Variables:
# mode: conf
# End:

[-- Attachment #3: pylintrc --]
[-- Type: text/plain, Size: 163 bytes --]

# pylintrc

[MESSAGES CONTROL]
disable=C0103,C0114,C0121,C0209,C0301,C0302,R0902,R0912,R0913,R0914,R0915,R1705,R1702,R1720

# Local Variables:
# mode: conf
# End:

Re: Python != None

[Dima Pasechnik]

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

On Sun, Feb 25, 2024 at 11:29:51AM -0800, Collin Funk wrote:
> On 2/25/24 3:57 AM, Bruno Haible wrote:
> > The style warnings about "!= None" in pycodestyle and/or pylint are
> > relativized by this warning in Python itself:
> > 
> >   >>> '' != None
> >   True
> >   >>> '' is not None
> >   <stdin>:1: SyntaxWarning: "is not" with a literal. Did you mean "!="?
> >   True
> 
> Interesting. I'm not too picky about the convention that we use. 

Just in case: this warning comes from the types of '' and None being different.

>>> 1.0 is 1
<stdin>:1: SyntaxWarning: "is" with a literal. Did you mean "=="?
False

One can compare objects of different types for equality:
>>> type(1.0)
<class 'float'>
>>> type(1)
<class 'int'>
>>> 1.0 == 1
True

(it's the same with "is not" vs. "!=")

Best,
Dima

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

Re: pycodestyle configuration

[Bruno Haible]

Collin Funk wrote:
> Adding the two attached files to the root directory of gnulib should
> adjust the warnings to the agreed upon coding style (as far as
> pycodestyle and pylint are concerned). It works with Emacs and Eglot
> using pylsp. I assume other editors should work similarly.
> 
> Not sure if they are worth adding to the repository but I figured I
> would share them just incase anyone else wants to work on stuff in the
> future.
> 
> More information about these files here:
> https://pycodestyle.pycqa.org/en/latest/intro.html#configuration
> https://pylint.readthedocs.io/en/stable/user_guide/configuration/index.html#

It's a good idea to have these in the git repository, rather than to
have each developer install them privately.

For pylint, according to
https://pylint.pycqa.org/en/stable/user_guide/usage/run.html#command-line-options
it's OK to have the file .pylintrc in the gnulib root directory.
If this is true (i.e. if it works with the file name '.pylintrc' instead of
'pylintrc' in your environment), feel free to submit this file as a patch.
It's OK with me because the file name is descriptive enough and because it's
hidden.

For pycodestyle, a file named '.pycodestyle' would be OK with me as well,
if that works. If not, then how about
  - submitting a feature request to the pycodestyle developers, so that
    '.pycodestyle' is accepted in addition to of 'pycodestyle' or 'setup.cfg'?
  - or, alternatively, can we move the gnulib-tool.py into the pygnulib/
    directory, leaving only a small redirector gnulib-tool.py at the top
    level? Then we could have the 'pycodestyle' or 'setup.cfg' in the
    pygnulib/ directory.

Bruno

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Bruno Haible]

Hi Collin,

> From what I could understand, it seems that upon seeing that the mode
> is "import", the specified_modules is set with the unsorted list of
> modules [1]. Then that variable is sorted before it is used [2]. Could
> you confirm that I am understanding this correctly?

Yes, I confirm. So, in the Python code, the corresponding place to do
the canonicalization (sorting and removing duplicates) would be in
GLImport.py line 258.

> If so, I feel that this patch should be the correct way to handle it.
> My initial ideas where to sort the modules in main() before the
> GLConfig object was created, or to modify the GLConfig class's methods
> which are used to set the modules in use.

Well, GLConfig applies to all modes (not just 'import', but also
'create-testdir' etc.). Since on the bash side, you found that the
sorting is specifically in the func_import(), the right place to do it
is in GLImport.py, not GLConfig.py.

The second part of your patch (removal of sorting from GLImport.actioncmd)
is good, but I guess you will want to have it combined with another patch
to GLImport.py.

Bruno

Re: pycodestyle configuration

[Collin Funk]

Hi Bruno,

On 2/26/24 12:38 PM, Bruno Haible wrote:
> For pycodestyle, a file named '.pycodestyle' would be OK with me as well,
> if that works. If not, then how about
>   - submitting a feature request to the pycodestyle developers, so that
>     '.pycodestyle' is accepted in addition to of 'pycodestyle' or 'setup.cfg'?
>   - or, alternatively, can we move the gnulib-tool.py into the pygnulib/
>     directory, leaving only a small redirector gnulib-tool.py at the top
>     level? Then we could have the 'pycodestyle' or 'setup.cfg' in the
>     pygnulib/ directory.

I think that option 2 would be the best idea. It seems like all the
different Python tools could never agree on a way to configure things.
From the pycodestyle documentation, per project configuration must be
placed in setup.cfg or tox.ini [1]. It seems that setup.cfg is a file
from setuptools (Python packaging tool) that has mostly superseded by
pyproject.toml [2]. Most tools seem to support this pyproject.toml
format, but it seems that the pycodestyle people are not fans [3]. The
tox.ini file is used to configure tox which is a test automation tool
that allows you to run packages with different virtual environments
[4]. Seems useful, but a strange place to store configurations.

It is my first time learning all this stuff so sorry if it is hard to
follow. It is for me as well...

I think that it would be best to just shove all the Python mess in
that subdirectory since it is a work-in-progress and is safe to be
ignored by gnulib users. As long as some gnulib-tool.py file is left
in the root directory, it should be easy enough to modify the
bootstrap script for testing.

[1] https://pycodestyle.pycqa.org/en/latest/intro.html#configuration
[2] https://packaging.python.org/en/latest/guides/writing-pyproject-toml/
[3] https://github.com/PyCQA/pycodestyle/issues/813#issuecomment-953736162
[4] https://tox.wiki/en/4.13.0/

Collin

Re: pycodestyle configuration

[Bruno Haible]

Hi Collin,

> I think that it would be best to just shove all the Python mess in
> that subdirectory since it is a work-in-progress and is safe to be
> ignored by gnulib users.

I tend to agree. Even after it is finished, it will still be useful
to have all Python sources in one directory: it makes it easy to do
a "grep something *.py".

Done:

2024-02-26  Bruno Haible  <bruno@clisp.org>

	gnulib-tool.py: Reorganize code.
	* pygnulib/main.py: New file, moved here from gnulib-tool.py.
	* pygnulib/constants.py: Change the way APP['name'] and DIRS['root'] are
	computed.
	* gnulib-tool.py: New file, based on gnulib-tool.

(The patch is best viewed using gitk.)

You can now submit the two configuration files in the pygnulib/ directory.
That's the proper place, not the root directory of gnulib.

Bruno

Re: pycodestyle configuration

[Collin Funk]

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

Hi Bruno,

On 2/26/24 2:54 PM, Bruno Haible wrote:
> I tend to agree. Even after it is finished, it will still be useful
> to have all Python sources in one directory: it makes it easy to do
> a "grep something *.py".

True. I've already run into this a few times. :)

> Done:
> 
> 2024-02-26  Bruno Haible  <bruno@clisp.org>
> 
> 	gnulib-tool.py: Reorganize code.
> 	* pygnulib/main.py: New file, moved here from gnulib-tool.py.
> 	* pygnulib/constants.py: Change the way APP['name'] and DIRS['root'] are
> 	computed.
> 	* gnulib-tool.py: New file, based on gnulib-tool.
> 
> (The patch is best viewed using gitk.)

I took a brief look at the patch and everything looks good. I've run
the merge-emacs script and confirmed that it works just as before.

> You can now submit the two configuration files in the pygnulib/ directory.
> That's the proper place, not the root directory of gnulib.

Done. I've confirmed that pylint recognizes .pylintrc so I've used
that file name instead. One thing to note is that you must run pylint
from the pygnulib/ subdirectory. Pycodestyle doesn't have this
restriction. For example:

     # Works
     [collin@debian pygnulib]$ pylint main.py
     # Doesn't work, doesn't pick up configuration file.
     [collin@debian pygnulib]$ cd ../
     [collin@debian gnulib]$ pylint pygnulib/main.py

This isn't a problem for me but I figured I'd mention it in case
someone gets scared by a ton of warnings that we don't care about.

The second patch fixes an undefined variable access that was uncovered
by the typos of mine that you fixed.

   [collin@debian pygnulib]$ gnulib-tool.py --create-testdir --dir test dummy
   Traceback (most recent call last):
     File "/home/collin/.local/src/gnulib/pygnulib/main.py", line 1180, in <module>
       main()
     File "/home/collin/.local/src/gnulib/pygnulib/main.py", line 608, in main
       if mode != None and 'test' in mode and gnu_make:
                                           ^^^^^^^^
    UnboundLocalError: cannot access local variable 'gnu_make' where it is not associated with a value

That was my fault, sorry about that. That file should probably be
cleaned up at a later date. For some reason all of the fields in the
object returned by argparse are used before being assigned to local
variables and then used again.

Thanks,
Collin

[-- Attachment #2: 0001-gnulib-tool.py-Add-configuration-files-for-Python-to.patch --]
[-- Type: text/x-patch, Size: 1697 bytes --]

From 900ded124da7b8f5c267dd58b423f119ac1eda2a Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Mon, 26 Feb 2024 16:02:34 -0800
Subject: [PATCH 1/2] gnulib-tool.py: Add configuration files for Python tools.

* pygnulib/.pylintrc: New file, used by pylintrc.
* pygnulib/setup.cfg: New file, currently only used for pycodestyle
options.
---
 ChangeLog          |  7 +++++++
 pygnulib/.pylintrc |  8 ++++++++
 pygnulib/setup.cfg | 10 ++++++++++
 3 files changed, 25 insertions(+)
 create mode 100644 pygnulib/.pylintrc
 create mode 100644 pygnulib/setup.cfg

diff --git a/ChangeLog b/ChangeLog
index af84986424..5b40d684ce 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-02-26  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Add configuration files for Python tools.
+	* pygnulib/.pylintrc: New file, used by pylintrc.
+	* pygnulib/setup.cfg: New file, currently only used for pycodestyle
+	options.
+
 2024-02-26  Bruno Haible  <bruno@clisp.org>
 
 	gnulib-tool.py: Reorganize code.
diff --git a/pygnulib/.pylintrc b/pygnulib/.pylintrc
new file mode 100644
index 0000000000..7c4c1a338c
--- /dev/null
+++ b/pygnulib/.pylintrc
@@ -0,0 +1,8 @@
+# .pylintrc
+
+[MESSAGES CONTROL]
+disable=C0103,C0114,C0121,C0209,C0301,C0302,R0902,R0912,R0913,R0914,R0915,R1705,R1702,R1720
+
+# Local Variables:
+# mode: conf
+# End:
diff --git a/pygnulib/setup.cfg b/pygnulib/setup.cfg
new file mode 100644
index 0000000000..5d4a17171a
--- /dev/null
+++ b/pygnulib/setup.cfg
@@ -0,0 +1,10 @@
+# setup.cfg
+
+[pycodestyle]
+ignore = E265,W503,E241,E711,E712,E201,E202,E221
+max-line-length = 136
+statistics = True
+
+# Local Variables:
+# mode: conf
+# End:
-- 
2.39.2


[-- Attachment #3: 0002-gnulib-tool.py-Fix-undefined-variable-access.patch --]
[-- Type: text/x-patch, Size: 1455 bytes --]

From c995d3334c9822ca9496025c0de674c0c041d183 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Mon, 26 Feb 2024 16:13:46 -0800
Subject: [PATCH 2/2] gnulib-tool.py: Fix undefined variable access.

* pygnulib/main.py (main): Don't use gnu_make before it is defined.
---
 ChangeLog        | 5 +++++
 pygnulib/main.py | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 5b40d684ce..035eda5c25 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2024-02-26  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Fix undefined variable access.
+	* pygnulib/main.py (main): Don't use gnu_make before it is defined.
+
 2024-02-26  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Add configuration files for Python tools.
diff --git a/pygnulib/main.py b/pygnulib/main.py
index 3d615ddbac..cfa59c6562 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -605,7 +605,7 @@ def main():
     if cmdargs.pobase == None and cmdargs.podomain != None:
         message = '%s: warning: --po-domain has no effect without a --po-base option\n' % constants.APP['name']
         sys.stderr.write(message)
-    if mode != None and 'test' in mode and gnu_make:
+    if mode != None and 'test' in mode and cmdargs.gnu_make:
         message = '%s: --gnu-make not supported when including tests\n' % constants.APP['name']
         sys.stderr.write(message)
         sys.exit(1)
-- 
2.39.2

Re: pycodestyle configuration

[Bruno Haible]

Hi Collin,

> > You can now submit the two configuration files in the pygnulib/ directory.
> > That's the proper place, not the root directory of gnulib.
> 
> Done.

Thanks; I applied both patches.

But now we have the desired configurations of pycodestyle and pylint
in two places: in comments and in the config files. With the promise that
they will get out of sync in the future. To avoid this, let me
update the comments...

> I've confirmed that pylint recognizes .pylintrc so I've used
> that file name instead. One thing to note is that you must run pylint
> from the pygnulib/ subdirectory. Pycodestyle doesn't have this
> restriction. For example:
> 
>      # Works
>      [collin@debian pygnulib]$ pylint main.py
>      # Doesn't work, doesn't pick up configuration file.
>      [collin@debian pygnulib]$ cd ../
>      [collin@debian gnulib]$ pylint pygnulib/main.py

Interesting. Also worth documenting. Done through the patch below.

> That file should probably be
> cleaned up at a later date. For some reason all of the fields in the
> object returned by argparse are used before being assigned to local
> variables and then used again.

I don't see a problem here, or anything to "clean up". It is on purpose
that these fields are stored in different objects:
  - in the cmdargs, to satisfy the requirements of the argparser,
  - in a GLConfig object, for the tool's logic.
For example, the way argparser works, 'mode_list', 'mode_find' etc.
need to be different booleans in cmdargs; however in GLConfig, we don't
want to obey a dictate from argparser; instead it should be a single
field 'mode', with several possible values.

Bruno


2024-02-26  Bruno Haible  <bruno@clisp.org>

	gnulib-tool.py: Add more comments.
	* pygnulib/main.py: Add comments regarding code style. Mention the
	pycodestyle and pylint configurations.

diff --git a/pygnulib/main.py b/pygnulib/main.py
index cfa59c6562..443300c439 100644
--- a/pygnulib/main.py
+++ b/pygnulib/main.py
@@ -24,11 +24,28 @@
 # - Place line breaks to help reading the code:
 #   Avoid line breaks inside expressions, if they can be avoided.
 #   Do use line breaks to separate the parts of [ ... for ... ] iterations.
+# - String literals: Use 'single-quotes'.
+#   Rationale: <https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00226.html>
+# - Comparison operators:
+#   Use == and != for variables which can contain only strings, numbers, lists,
+#   and None.
+#   Use 'is' and 'is not' for variables which can contain only class instances
+#   (e.g. GLModule instances) and None.
+#   Rationale: <https://lists.gnu.org/archive/html/bug-gnulib/2024-02/msg00225.html>
+
 # You can use this command to check the style:
-#   $ pycodestyle --max-line-length=136 --ignore=E265,W503,E241,E711,E712,E201,E202,E221 pygnulib/*.py
+#   $ pycodestyle *.py
+# The pycodestyle configuration is found in the file setup.cfg.
+# Documentation:
+# <https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes>
 
 # You can use this command to check for mistakes:
-#   $ pylint --disable=C0103,C0114,C0121,C0209,C0301,C0302,R0902,R0912,R0913,R0914,R0915,R1705,R1702,R1720 pygnulib/*.py
+#   $ pylint *.py
+# The pylint configuration is found in the file .pylintrc.
+# Note: This configuration is only effective when you invoke pylint from this
+# directory.
+# Documentation:
+# <https://pylint.readthedocs.io/en/latest/user_guide/messages/messages_overview.html>
 
 
 #===============================================================================

Re: pycodestyle configuration

[Collin Funk]

Hi Bruno,

On 2/26/24 6:38 PM, Bruno Haible wrote:
> But now we have the desired configurations of pycodestyle and pylint
> in two places: in comments and in the config files. With the promise that
> they will get out of sync in the future. To avoid this, let me
> update the comments...

True. I thought about adding comments to the configuration files, but
your idea is probably better. Less chance for statements to disagree
with each other. Better to just tailor the files to those comments.

> I don't see a problem here, or anything to "clean up". It is on purpose
> that these fields are stored in different objects:
>   - in the cmdargs, to satisfy the requirements of the argparser,
>   - in a GLConfig object, for the tool's logic.
> For example, the way argparser works, 'mode_list', 'mode_find' etc.
> need to be different booleans in cmdargs; however in GLConfig, we don't
> want to obey a dictate from argparser; instead it should be a single
> field 'mode', with several possible values.

I see, it makes more sense with that explanation. Thanks. I saw your
email about the sorting of modules and GLImport. I see what you mean
and I'll work on that later.

It seems that gnulib-tool.py likes to mess with copyright headers on
source files copied from lib/. I'll probably look into that first
since it makes comparing the result of gnulib-tool and gnulib-tool.py
take more work. It seems unrelated to the licensing items in
gnulib-tool.py.TODO but those seem trivial so I might as well do them
too.

Thanks,
Collin

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Collin Funk]

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

On 2/26/24 12:51 PM, Bruno Haible wrote:
> Well, GLConfig applies to all modes (not just 'import', but also
> 'create-testdir' etc.). Since on the bash side, you found that the
> sorting is specifically in the func_import(), the right place to do it
> is in GLImport.py, not GLConfig.py.

I think that this patch should be correct. The sorting of modules is
done only for mode == 'import' in GLImport.__init__. Looking at the
path from main, this would make the sorting happen right before
performing the transitive closure, like gnulib-tool.

Also this comment from gnulib-tool:
    # In 'import' mode, the new set of specified modules overrides the cached
    # set of modules. Ignore the cached settings.

So we only care about self.config.getModules().

I've also removed the sorted() calls in actioncmd. The sorting in
__init__ works correctly so gnulib-tool and gnulib-tool.py output the
same actioncmd (at least with Emac's merge-gnulib). Let me know if I
missed anything.

Collin

[-- Attachment #2: 0001-gnulib-tool.py-Make-module-sorting-more-similar-to-g.patch --]
[-- Type: text/x-patch, Size: 2279 bytes --]

From 3a7cbd5c95b2d24341865bf101fe8b1a93f83737 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Wed, 28 Feb 2024 03:33:15 -0800
Subject: [PATCH] gnulib-tool.py: Make module sorting more similar to
 gnulib-tool.

* pygnulib/GLImport.py (GLImport.__init__): Sort modules when mode is
'import'.
(GLImport.actioncmd): Don't sort modules while creating actioncmd. Use
preferred quoting style.
---
 ChangeLog            | 8 ++++++++
 pygnulib/GLImport.py | 8 +++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 330727e02e..8cfe498940 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-02-28  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Make module sorting more similar to gnulib-tool.
+	* pygnulib/GLImport.py (GLImport.__init__): Sort modules when mode is
+	'import'.
+	(GLImport.actioncmd): Don't sort modules while creating actioncmd. Use
+	preferred quoting style.
+
 2024-02-28  Bruno Haible  <bruno@clisp.org>
 
 	gnulib-tool: Make --version output independent of git's configuration.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 0f0f463add..fca2333ec5 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -221,7 +221,9 @@ class GLImport(object):
                               for localdir in self.cache['localpath'] ]
                 self.config.setLocalPath(localpath)
 
-        if self.mode != MODES['import']:
+        if self.mode == MODES['import']:
+            self.config.setModules(sorted(self.config.getModules()))
+        else:
             if self.cache['m4base'] and (self.config['m4base'] != self.cache['m4base']):
                 raise GLError(5, m4base)
 
@@ -438,9 +440,9 @@ class GLImport(object):
         elif vc_files == False:
             actioncmd += ' \\\n#  --no-vc-files'
         if len(avoids) > 0:
-            actioncmd += ''.join([f" \\\n#  --avoid={x}" for x in sorted(avoids)])
+            actioncmd += ''.join([f' \\\n#  --avoid={x}' for x in avoids])
         if len(modules) > 0:
-            actioncmd += ''.join([f" \\\n#  {x}" for x in sorted(modules)])
+            actioncmd += ''.join([f' \\\n#  {x}' for x in modules])
         return actioncmd
 
     def relative_to_destdir(self, dir):
-- 
2.39.2

Re: gnulib-tool.py: Follow gnulib-tool changes, part 28.

[Bruno Haible]

Collin Funk wrote:
> I've also removed the sorted() calls in actioncmd. The sorting in
> __init__ works correctly so gnulib-tool and gnulib-tool.py output the
> same actioncmd (at least with Emacs' merge-gnulib).

Thanks for following up on this. The patch looks good. Applied.

Bruno