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

[Bogdan]

Gavin Smith <gavinsmith0123@gmail.com>, Wed Mar 22 2023 20:06:49 
GMT+0100 (Central European Standard Time)
> 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.


 I believe. Probably not many wish to have such a high level of 
compatibility with both the new and old Texinfo, and so most people 
simply use command-line switches. But it's so good that the 
environment variable now exists!


>  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.


 Yes. As a side effect, this would cause test failures in Automake if 
just the environment variable and only this particular environment 
variable was set, because some tests check for left-over files in the 
build 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 *was* thinking about this, too. On the other hand, if the user 
gives "--build-dir=.", it *would* imply --tidy, even though this is 
the default. The sole presence of the switch changes the behaviour.

 I'm thinking also about situations like

        export TEXI2DVI_BUILD_DIRECTORY=$some_runtime_var
and
        texi2dvi --build-dir=$some_runtime_var ...

 where 'some_runtime_var' can be any directory, or '.' as a fallback, 
in some user script.
 Even if it ends up in the '.' fallback, it still means the user 
explicitly chose to use the command-line switch (which implies --tidy) 
or the environment variable (which doesn't imply --tidy right now).
 In other words, for consistency and no-surprises, it should be:
        --build-dir given => --tidy
        TEXI2DVI_BUILD_DIRECTORY given => --tidy
whatever the values.


> 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.


 I understand and I'm not claiming that my patch is perfect.


> 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.


 Certainly makes sense, and is probably the way it was meant to be 
(in other words, clean some external directories). Shouldn't hurt 
Automake's tests, I think.

 Remember also about the help message:

"
      --build-dir=DIR  specify where the tidy compilation is performed;
                         implies --tidy;
                         defaults to TEXI2DVI_BUILD_DIRECTORY 
[$build_dir]

"

For some reason, it was made so that passing --build-dir implies 
--tidy... But, maybe just for the non-default values, as I wrote above 
("external directories").


> 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?


 From a user's point of view - looks fine. Generally, I have nothing 
against your change.
 Since I began playing with fixing Automake I'm just worried if its 
tests would need to pass --tidy now, which wasn't present in the older 
versions, so what do we do now to have a command line that works with 
all versions (maybe except really ancient ones)?
But it seems we're always passing a directory which isn't ".", so 
hopefully it will work.


> >   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.


 No bugtracker? Not good from a bug reporter's point of view :). I 
thought all FSF projects use the same bugtracker, it would seem 
logical. You probably have a good reason for not to have one, but 
don't you sometimes get 10 reports about the same problem? :)



> 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.)


 Ouch, really bad from my side. I believe I read the same manual, 
because I usually write things like

        "x$var" != "x"

 but I missed the part you're quoting. Maybe I didn't have a need for 
"-a" earlier, so I skipped that or erased it from my memory :).
 Thanks for pointing that out!


--
Regards - Bogdan ('bogdro') D.                 (GNU/Linux & FreeDOS)
X86 assembly (DOS, GNU/Linux):    http://bogdro.evai.pl/index-en.php
Soft(EN): http://bogdro.evai.pl/soft  http://bogdro.evai.pl/soft4asm
www.Xiph.org  www.TorProject.org  www.LibreOffice.org  www.GnuPG.org