Genivia / ugrep-indexer

A monotonic indexer to speed up grepping by >10x (ugrep-indexer is now part of ugrep 6.0)
https://ugrep.com
BSD 3-Clause "New" or "Revised" License
67 stars 1 forks source link

configure.ac: Incorrect use of `AC_ARG_ENABLE` #10

Closed barsnick closed 9 months ago

barsnick commented 9 months ago

As mentioned in https://github.com/Genivia/ugrep-indexer/issues/8#issuecomment-1878981697, there currently (v0.9.5-1) isn't any difference in the behavior of

Expected:

$ ./configure
[...]
checking for --disable-7zip... no

Expected:

$ ./configure --disable-7zip
[...]
checking for --disable-7zip... yes

Expected:

$ ./configure --enable-7zip=no
[...]
checking for --disable-7zip... yes

Not expected:

$ ./configure --enable-7zip
[...]
checking for --disable-7zip... yes

This is due to the incorrect use of autoconf's macro AC_ARG_ENABLE: Choosing Package Options AC_ARG_ENABLE detects "enable" and "disable", and creates an $enableval. The default behavior (when no such option is given) is determined by the fourth argument.

7zip detection uses this logic:

AC_ARG_ENABLE(7zip,
  [AS_HELP_STRING([--disable-7zip],
                  [to disable 7zip and no longer index .7z files])],
  [with_no_7zip="yes"],
  [with_no_7zip="no"])

If no such option is given, disabling of 7zip support is off, as intended. But if any option is given - whether explicitly to enable or disable - disabling of 7zip is on. Even if --enable-7zip is given.

The third argument needs to evaluate $enableval, which has a different logic than the intended disabling. Therefore, the logic needs to be inverted as well:

AC_ARG_ENABLE(7zip,
  [AS_HELP_STRING([--disable-7zip],
                  [to disable 7zip and no longer index .7z files])],
  [with_7zip="$enableval"],
  [with_7zip="yes"])

This leads to this block, which then behaves as expected:

AC_ARG_ENABLE(7zip,
  [AS_HELP_STRING([--disable-7zip],
                  [to disable 7zip and no longer index .7z files])],
  [with_7zip="$enableval"],
  [with_7zip="yes"])
AC_MSG_CHECKING(for --disable-7zip)
if test "x$with_7zip" = "xyes"; then
  AC_MSG_RESULT(no)
  CPPFLAGS="$CPPFLAGS -I../lzma/C"
  LDFLAGS="$LDFLAGS -L../lzma/C"
  LIBS="-lviiz $LIBS"
else
  EXTRA_CFLAGS="-DWITH_NO_7ZIP ${EXTRA_CFLAGS}"
  AC_MSG_RESULT(yes)
fi

or this patch:

diff --git a/configure.ac b/configure.ac
index 48f35ae..7871c08 100644
--- a/configure.ac
+++ b/configure.ac
@@ -50,10 +50,10 @@ AX_CHECK_BROTLILIB([], [echo "optional brotli library not found: install if you
 AC_ARG_ENABLE(7zip,
   [AS_HELP_STRING([--disable-7zip],
                   [to disable 7zip and no longer index .7z files])],
-  [with_no_7zip="yes"],
-  [with_no_7zip="no"])
+  [with_7zip="$enableval"],
+  [with_7zip="yes"])
 AC_MSG_CHECKING(for --disable-7zip)
-if test "x$with_no_7zip" = "xno"; then
+if test "x$with_7zip" = "xyes"; then
   AC_MSG_RESULT(no)
   CPPFLAGS="$CPPFLAGS -I../lzma/C"
   LDFLAGS="$LDFLAGS -L../lzma/C"
genivia-inc commented 9 months ago

Ah, yes. Slain by double negated logic...

barsnick commented 9 months ago

This patch probably also applies 1:1 to ugrep, when I look at its configure.ac.

genivia-inc commented 9 months ago

Committed a change to both projects to fix this.

genivia-inc commented 9 months ago

Fixed in update 0.9.5-2.