Hi Collin,

The patch is mostly good; I've applied it.

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

    # Prefer configure.ac to configure.in.
    if test -f "$destdir"/configure.ac; then
      configure_ac="$destdir/configure.ac"
    else
      if test -f "$destdir"/configure.in; then
        configure_ac="$destdir/configure.in"
      else
        func_fatal_error "cannot find $destdir/configure.ac - make sure you run 
gnulib-tool from within your package's directory"
      fi
    fi

Whereas in main.py configure.in is not looked at:

        # Analyze configure.ac.
        with open(joinpath(destdir, 'configure.ac'), 'r', encoding='utf-8') as 
file:
            configure_ac_data = file.read()

> In gnulib-tool.sh AC_CONFIG_AUX_DIR, A[CM]_PROG_LIBTOOL,
> AC_CONFIG_MACRO_DIR, and AC_CONFIG_MACRO_DIRS are checked under:
> 
> import | add-import | remove-import | update ) ;;
> 
> In the Python version currently we check for AC_CONFIG_AUX_DIR and
> A[CM]_PROG_LIBTOOL in GLImport.__init__(). This patch adds the checks
> for AC_CONFIG_MACRO_DIR and AC_CONFIG_MACRO_DIRS in main() under:
> 
> elif mode in ['import', 'add-import', 'remove-import', 'update']:
> 
> Can the AC_CONFIG_AUX_DIR and A[CM]_PROG_LIBTOOL being checked in
> every new GLImport cause any issues?

The only "issue" that I can see with it is that in the case of multiple
gnulib dirs

        # Apply func_import to all gnulib directories.

the Python code will read the configure.{ac,in} file several times,
whereas the shell code reads it only once — which is more efficient.

It would be good to move this block of code (GLImport.py lines 90..118)
to main.py. This means that the GLImport constructor takes more arguments.
But I don't see a use-case of creating a GLImport object other than from
main.py.

You can do this move and the fix of the problem mentioned above in one
patch.

> I've made one style change:
> 
> -        current_edit = int()
> +        current_edit = 0
> 
> I've just changed it now since it was confusing to me and there are no
> other occurrences of this convention.

Good.

Bruno




Reply via email to