Re: [PATCH] Texinfo: inconsistent behavior: cmd line vs. env

[Gavin Smith]

On Tue, Mar 21, 2023 at 02:13:40PM +0100, Bogdan wrote:
> Hello.
> 
>  As suggested by Gavin, I'm reporting a problem (or at least a "surprising
> inconsistency") in the texi2dvi script. This is related to Automake
> bug#29188 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29188).
> 
>  Since texinfo-4.9, one can set the build directory with an environment
> variable "TEXI2DVI_BUILD_DIRECTORY". Unfortunately, texinfo has a
> problem/inconsistency there. When setting "--build-dir=" on the command
> line, the build mode is set to "tidy" (which cleans some log files, etc.).
> Fine. The problem is that if you set the build directory using an
> environment variable, the mode is NOT set to "tidy", leaving the logs files
> (and failing tests in Automake). Luckily, you can also set the build mode
> from an environment variable, "TEXI2DVI_BUILD_MODE".

I guess that TEXI2DVI_BUILD_DIRECTORY must be rarely used because this
hasn't been reported before.  The existing behaviour doesn't seem useful,
and I'm in favour of simplifying the possible configurations, so it
seems like a good idea to make setting TEXI2DVI_BUILD_DIRECTORY equivalent
to using --build-dir, as you suggest.

To summarize, if TEXI2DVI_BUILD_DIRECTORY was set, but --tidy (or --clean)
wasn't given, then the build directory would be created, but all the
auxiliary files would be dumped in the current directory.

There is still the minor question of what should happen if
TEXI2DVI_BUILD_DIRECTORY is set to '.' (the default).  Since this is the
default value, it doesn't seem right that explicitly setting
TEXI2DVI_BUILD_DIRECTORY=. in the environment should change the
behaviour of texi2dvi.

I like the simplicity that the environment variable is used to set
the internal 'build_dir' variable exactly once, and is never used again.

Maybe it seems like I'm being picky here, but texi2dvi is at the borderline
of being difficult to make sense of, and any extra complications in terms
of possible configurations could push it over the edge.

Hence, I propose the following patch:

diff --git a/util/texi2dvi b/util/texi2dvi
index 28c53ebe44..29b0abd453 100755
--- a/util/texi2dvi
+++ b/util/texi2dvi
@@ -1694,7 +1694,7 @@ while test x"$1" != x"$arg_sep"; do
     -~ ) verbose "Option -~ is obsolete: texi2dvi ignores it.";;
     -b | --batch) ;; # Obsolete
          --build)      shift; build_mode=$1;;
-         --build-dir)  shift; build_dir=$1; build_mode=tidy;;
+         --build-dir)  shift; build_dir=$1;;
     -c | --clean) build_mode=clean;;
     -D | --debug) debug=true;;
     -e | -E | --expand) expand=true;;
@@ -1743,6 +1743,10 @@ done
 # Pop the token
 shift
 
+if test x"$build_dir" != x"." && test x"$build_mode" = x"local" ; then
+  build_mode=tidy
+fi
+
 # $tidy:  compile in a t2d directory.
 # $clean: remove all the aux files.
 case $build_mode in

It still sets build_mode to tidy based on the build directory, but in only
one place in the code, not in two.

My patch may have the problem that it makes --build-dir=. do nothing,
while this may have been a useful option.  It had the same effect
as --tidy on its own, so maybe people would just use --tidy instead
of --build-dir=.

Does this seem ok?

>  P.S. I didn't manage to find any bugtracker for Texinfo, so I don't know if
> I'm not duplicating some other report or patch.

There isn't one.

Note for this:

> +if test -n "$TEXI2DVI_BUILD_DIRECTORY" -a \
> +  -z "$TEXI2DVI_BUILD_MODE" ; then
> +  build_mode=tidy;
> +fi

The autoconf manual warns agains using the -a option to test:

     The ‘-a’, ‘-o’, ‘(’, and ‘)’ operands are not present in all
     implementations, and have been marked obsolete by Posix 2008.  This
     is because there are inherent ambiguities in using them.  For
     example, ‘test "$1" -a "$2"’ looks like a binary operator to check
     whether two strings are both non-empty, but if ‘$1’ is the literal
     ‘!’, then some implementations of ‘test’ treat it as a negation of
     the unary operator ‘-a’.

     Thus, portable uses of ‘test’ should never have more than four
     arguments, and scripts should use shell constructs like ‘&&’ and
     ‘||’ instead.

(I only know this because I looked it up -- I don't remember how you
are supposed to write these things.)