Hi Collin,
> Hopefully this patch should be better.
Thanks, much better. I applied it, together with this follow-up:
2024-03-10 Bruno Haible <br...@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
