Hi Collin,

> > One point needs attention, still: In gnulib-tool.sh the code looks at
> > configure.ac and, if that does not exist, at configure.in:
> > ...
> > Whereas in main.py configure.in is not looked at:
> > ...

Thanks for getting back to this item.

But the patch does way too much, and does some things incorrectly.

* In GLConfig.py the comment
  '''Specify path of autoconf file relative to destdir.'''
  is no longer correct. Now, configure_ac is specified relative to the
  current directory.

* In main.py there is code added for modes other than
  'import', 'add-import', 'remove-import', 'update':

+    else:
+        if isfile('configure.ac'):
+            configure_ac = 'configure.ac'
+        elif isfile('configure.in'):
+            configure_ac = 'configure.in'

  But when the mode is 'create-testdir' or 'create-megatestdir',
  it is wrong to even look for a configure.ac file in the current directory.

* In main.py:

+        autoconf_minversion = pattern.search(configure_ac_data)
+        if autoconf_minversion:
+            autoconf_minversion = float(autoconf_minversion.group(1))

  The original code looked out for multiple AC_PREREQ invocations and
  took the maximum. For example:
     AC_PREREQ([2.64])
     AC_PREREQ([2.69])
     AC_PREREQ([2.66])
  must be treated as if it were
     AC_PREREQ([2.69])
  The new code looks at the first AC_PREREQ invocation only.

* The --automake-subdir/--automake-subdir-tests handling is specific
  to the 'import', 'add-import', 'remove-import', 'update' modes. It
  therefore belongs in GLImport.py. It does not belong in main.py.

* Even the determination of auxdir and libtool from the contents of
  configure.ac is misplaced in main.py. It should remain in GLImport.py.
  It's not a problem if configure.ac gets read 2 or 3 times; it's a small
  file.

The only problem that should be fixed is the determination of the file
name of configure.ac/in, since that is currently duplicated code between
main.py and GLImport.py.

> I'm not sure if you want to add a test case which uses configure.in.
> Maybe just checking on clisp or something would be enough?

A new test case is not needed. Test cases are needed, in general, for
functionality that is
  - essential to many users, or
  - implemented in a complicated way, or
  - likely to be broken through future maintenance.
The configure.in filename handling does not match either of these three
criteria.

Bruno




Reply via email to