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

[Bruno Haible]

Hi Collin,

> Hopefully this patch should be better.

Thanks, much better. I applied it, together with this follow-up:


2024-03-10  Bruno Haible  <bruno@clisp.org>

        gnulib-tool.py: Tweak last commit.
        * pygnulib/GLEmiter.py (GLEmiter.initmacro_end): Avoid an implicit str
        to bool conversion.
        * pygnulib/GLImport.py (GLImport.__init__): Add a comment. Don't allow
        a '|' in place of whitespace. Don't emit redundant gl_source_base
        assignments.

diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index 9a889fde5a..a904aee8ad 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -566,7 +566,7 @@ USE_MSGCTXT = no\n"""
         # arguments. The check is performed only when autoconf is run from the
         # directory where the configure.ac resides; if it is run from a 
different
         # directory, the check is skipped.
-        if automake_subdir and not gentests and sourcebase and sourcebase != 
'.':
+        if automake_subdir and not gentests and sourcebase != '' and 
sourcebase != '.':
             subdir = f'{sourcebase}/'
         else:
             subdir = ''
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 8864bfd7c0..64a0bc9b13 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -259,6 +259,7 @@ class GLImport(object):
                     self.config.update_key(config, key)
             self.config.setModules(modules)
 
+        # Determine whether --automake-subdir is supported.
         if self.config['automake_subdir']:
             found_subdir_objects = False
             if self.config['destdir']:
@@ -266,7 +267,7 @@ class GLImport(object):
             else:
                 base = '.'
             if isfile(joinpath(base, 'Makefile.am')):
-                pattern = re.compile(r'^AUTOMAKE_OPTIONS[\t| ]*=(.*)$', 
re.MULTILINE)
+                pattern = re.compile(r'^AUTOMAKE_OPTIONS[\t ]*=(.*)$', 
re.MULTILINE)
                 with open(joinpath(base, 'Makefile.am'), encoding='utf-8') as 
file:
                     data = file.read()
                 automake_options = pattern.findall(data)
@@ -702,7 +703,6 @@ AC_DEFUN([%s_INIT],
         emit += '  gl_m4_base=\'%s\'\n' % m4base
         emit += self.emitter.initmacro_start(macro_prefix, False)
         emit += self.emitter.shellvars_init(False, sourcebase)
-        emit += '  gl_source_base=\'%s\'\n' % sourcebase
         if witness_c_macro:
             emit += '  m4_pushdef([gl_MODULE_INDICATOR_CONDITION], [%s])\n' % 
witness_c_macro
         # Emit main autoconf snippets.
@@ -716,7 +716,6 @@ AC_DEFUN([%s_INIT],
         emit += '  gltests_ltlibdeps=\n'
         emit += self.emitter.initmacro_start('%stests' % macro_prefix, 
gentests)
         emit += self.emitter.shellvars_init(True, testsbase)
-        emit += '  gl_source_base=\'%s\'\n' % testsbase
         # Define a tests witness macro that depends on the package.
         # PACKAGE is defined by AM_INIT_AUTOMAKE, PACKAGE_TARNAME is defined by
         # AC_INIT.


> > 1) This form of conditional expression
> > 
> >        base = self.config['destdir'] if self.config['destdir'] else '.'
> > 
> >    mentions first a value, then a condition, then another value.
> >    It would be good to mention the condition first, like in C, Lisp, Java, 
> > etc.
> >    - even if that means that this needs 4 lines of code instead of 1 line.
> 
> Sure, thanks for updating the coding style. I've changed it to a more
> normal:
> 
>         if self.config['destdir']:
>             base = self.config['destdir']
>         else:
>             base = '.'

Thanks. Yes, that's what I meant.

> > 3) Method GLConfig.setAutomakeSubdir claims that
> >    "automake_subdir must be a string"
> >    but it should be bool.
> 
> I think I see what you mean. I updated the type check but forgot to
> update the comment and message below it. Should be fixed now.

Yup, that looks good now.

> > 4) In GLEmiter.initmacro_end, you translated the condition
> > 
> >    if $automake_subdir && ! "$2" && test -n "$sourcebase" && test 
> > "$sourcebase" != '.'; then
> > 
> >    to
> > 
> >    if automake_subdir and not gentests and sourcebase != '.':
> > 
> >    What about the case sourcebase == '' ? We don't want subdir to be '/' in 
> > this case.
> >    (Or can this not happen?)
> 
> Oops, for some reason my brain must have ignored the
> 'test -n "$sourcebase"'. I am not 100% sure of all the paths that
> sourcebase can enter this function with. In any case, I think it is
> best to add the check. After a call to GLConfig.resetSourceBase() it
> may be '', and that is probably likely to occur.

Good. Thanks for adding it.

> I'm not the best with shell and always have to look up the different
> test flags. I believe that this condition should work according to my
> interpretation of 'test -n' [1]:
> 
>     if automake_subdir and not gentests and sourcebase and sourcebase != '.':
> 
> In this case the condition will fail if sourcebase is a string with a
> length of zero. Comparing to '' feels strange to me, but let me know if
> you prefer it.

I prefer comparing with '' explicitly. It's clearer about the intent.
It avoids an implicit conversion from str to bool.

Regarding the regex change: In a group '(...)' one needs to use '|' to
indicate different alternatives. Whereas in a character set, alternation
is already implicit:   '[xyz]' is the same as '([x]|[y]|[z])' or '(x|y|z)'.


Bruno