configure.ac: Fix --with* options
diff mbox series

Message ID 20191113104149.42407-1-lkml@jv-coder.de
State Accepted
Delegated to: Petr Vorel
Headers show
Series
  • configure.ac: Fix --with* options
Related show

Commit Message

Joerg Vehlow Nov. 13, 2019, 10:41 a.m. UTC
From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

The usage of AC_ARG_WITH was wrong. This resulted in unexpected configuration.
E.g --without-bash set with_bash=yes and --with-nume set with_numa=no.
This is fixed by using "$withval" in the action-if-given. Also all AC_ARG_WITH
are unified now (all use alos action-if-not-given)

The "default=" help text did not make sense for same options.
e.g. for --with expect was "default=yes", but it defaults to no.
The "default=" strings are dropped, because defaultness is indicated by
either "--with-<option>" or "--without-<option>" as done by other projects,
that use autoconf.

Defining AC_ARG_WITH within an if to express dependencies does not work.
./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
even if with_bash=no or with_python=no. The check is removed completely.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 configure.ac | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

Comments

Petr Vorel Nov. 13, 2019, 4:02 p.m. UTC | #1
Hi Joerg,

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

> The usage of AC_ARG_WITH was wrong. This resulted in unexpected configuration.
> E.g --without-bash set with_bash=yes and --with-nume set with_numa=no.
> This is fixed by using "$withval" in the action-if-given. Also all AC_ARG_WITH
> are unified now (all use alos action-if-not-given)

> The "default=" help text did not make sense for same options.
> e.g. for --with expect was "default=yes", but it defaults to no.
> The "default=" strings are dropped, because defaultness is indicated by
> either "--with-<option>" or "--without-<option>" as done by other projects,
> that use autoconf.

> Defining AC_ARG_WITH within an if to express dependencies does not work.
> ./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
> even if with_bash=no or with_python=no. The check is removed completely.

> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
LGTM, but I'll have more deep look tomorrow.
FYI: https://github.com/linux-test-project/ltp/issues/508

Kind regards,
Petr
Li Wang Nov. 14, 2019, 3:24 a.m. UTC | #2
On Wed, Nov 13, 2019 at 6:42 PM Joerg Vehlow <lkml@jv-coder.de> wrote:

> From: Joerg Vehlow <joerg.vehlow@aox-tech.de>
>
> The usage of AC_ARG_WITH was wrong. This resulted in unexpected
> configuration.
> E.g --without-bash set with_bash=yes and --with-nume set with_numa=no.
> This is fixed by using "$withval" in the action-if-given. Also all
> AC_ARG_WITH
> are unified now (all use alos action-if-not-given)
>
> The "default=" help text did not make sense for same options.
> e.g. for --with expect was "default=yes", but it defaults to no.
> The "default=" strings are dropped, because defaultness is indicated by
> either "--with-<option>" or "--without-<option>" as done by other projects,
> that use autoconf.
>
> Defining AC_ARG_WITH within an if to express dependencies does not work.
> ./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
> even if with_bash=no or with_python=no. The check is removed completely.
>

I think this patch makes sense. It follows the AC_ARG_WITH official usage,
and make use of the shell variable 'withval' is also a wise choice.

Just a few queries below:


>
> Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
> ---
>  configure.ac | 46 ++++++++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 62c5a0bb4..4b7c6d57c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -108,8 +108,9 @@ AC_CHECK_FUNCS([ \
>  # Expect
>  AC_ARG_WITH([bash],
>    [AC_HELP_STRING([--with-bash],
> -    [have the Bourne Again SHell interpreter (default=no)])],
> -  [with_bash=yes],
> +    [have the Bourne Again Shell interpreter])],
> +  [with_bash=$withval],
> +  [with_bash=no]
>  )
>  if test "x$with_bash" = xyes; then
>      AC_SUBST([WITH_BASH],["yes"])
> @@ -119,8 +120,8 @@ fi
>
>  AC_ARG_WITH([expect],
>    [AC_HELP_STRING([--with-expect],
> -    [have the Tcl/expect library (default=yes)])],
> -  [with_expect=yes],
> +    [have the Tcl/expect library])],
> +  [with_expect=$withval],
>    [with_expect=no]
>

From the original intention, it likely to set yes as the default, so maybe
the [action-if-not-given] should as  [with_expect=yes]?


>  )
>  if test "x$with_expect" = xyes; then
> @@ -132,16 +133,16 @@ fi
>  # Numa
>  AC_ARG_WITH([numa],
>    AC_HELP_STRING([--without-numa],
> -    [without numa support (default=no)]),
> -  [with_numa=no],
> +    [without numa support]),
> +  [with_numa=$withval],
>    [with_numa=yes]
>  )
>
>  # Perl
>  AC_ARG_WITH([perl],
>    [AC_HELP_STRING([--with-perl],
> -    [have a perl interpreter (default=yes)])],
> -  [with_perl=yes],
> +    [have a perl interpreter])],
> +  [with_perl=$withval],
>    [with_perl=no]
>

[with_perl=yes] ?


>  )
>  if test "x$with_perl" = xyes; then
> @@ -153,8 +154,8 @@ fi
>  # Python
>  AC_ARG_WITH([python],
>    [AC_HELP_STRING([--with-python],
> -    [have a python interpreter (default=yes)])],
> -  [with_python=yes],
> +    [have a python interpreter])],
> +  [with_python=$withval],
>    [with_python=no]
>

[with_python=yes] ?


>  )
>  if test "x$with_python" = xyes; then
> @@ -166,8 +167,8 @@ fi
>  # TI RPC
>  AC_ARG_WITH([tirpc],
>    AC_HELP_STRING([--without-tirpc],
> -    [without libtirpc support (default=no)]),
> -  [with_tirpc=no],
> +    [without libtirpc support]),
> +  [with_tirpc=$withval],
>    [with_tirpc=yes]
>  )
>  # END tools knobs
> @@ -176,8 +177,9 @@ AC_ARG_WITH([tirpc],
>
>  AC_ARG_WITH([open-posix-testsuite],
>    [AC_HELP_STRING([--with-open-posix-testsuite],
> -    [compile and install the open posix testsuite (default=no)])],
> -  [with_open_posix_testsuite=$withval]
> +    [compile and install the open posix testsuite])],
> +  [with_open_posix_testsuite=$withval],
> +  [with_open_posix_testsuite=no]
>  )
>  if test "x$with_open_posix_testsuite" = xyes; then
>      AC_SUBST([WITH_OPEN_POSIX_TESTSUITE],["yes"])
> @@ -185,14 +187,14 @@ else
>      AC_SUBST([WITH_OPEN_POSIX_TESTSUITE],["no"])
>  fi
>
> -# testcases/realtime requires bash and python.
> -if test "x$with_bash" = xyes && test "x$with_python" = xyes; then
> -    AC_ARG_WITH([realtime-testsuite],
> -      [AC_HELP_STRING([--with-realtime-testsuite],
> -        [compile and install the realtime testsuite (default=no)])],
> -      [with_realtime_testsuite=yes]
> -    )
> -fi
> +# TODO: testcases/realtime requires bash and python.
>

Why remove the judgment of bash/python here?


> +AC_ARG_WITH([realtime-testsuite],
> +  [AC_HELP_STRING([--with-realtime-testsuite],
> +    [compile and install the realtime testsuite])],
> +  [with_realtime_testsuite=$withval],
> +  [with_realtime_testsuite=no]
> +)
> +
>  if test "x$with_realtime_testsuite" = xyes; then
>      AC_SUBST([WITH_REALTIME_TESTSUITE],["yes"])
>      # Run configure on testcases/realtime as well.
> --
> 2.20.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
Joerg Vehlow Nov. 14, 2019, 6:31 a.m. UTC | #3
Hi Li,

> I think this patch makes sense. It follows the AC_ARG_WITH official 
> usage, and make use of the shell variable 'withval' is also a wise choice.
>
> Just a few queries below:
>
>
>
>      AC_ARG_WITH([expect],
>        [AC_HELP_STRING([--with-expect],
>     -    [have the Tcl/expect library (default=yes)])],
>     -  [with_expect=yes],
>     +    [have the Tcl/expect library])],
>     +  [with_expect=$withval],
>      [with_expect=no]
>
>
> From the original intention, it likely to set yes as the default, so 
> maybe the [action-if-not-given] should as  [with_expect=yes]?
Maybe, but I did not want to change the current behavior here. This 
should be done in another patch.
>
>
>     -# testcases/realtime requires bash and python.
>     -if test "x$with_bash" = xyes && test "x$with_python" = xyes; then
>     -    AC_ARG_WITH([realtime-testsuite],
>     -      [AC_HELP_STRING([--with-realtime-testsuite],
>     -        [compile and install the realtime testsuite (default=no)])],
>     -      [with_realtime_testsuite=yes]
>     -    )
>     -fi
>     +# TODO: testcases/realtime requires bash and python.
>
>
> Why remove the judgment of bash/python here?
It simply does not work as intended. See last part of my patch description:
Defining AC_ARG_WITH within an if to express dependencies does not work.
./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
even if with_bash=no or with_python=no. The check is removed completely.

I though removing it is better than leaving something that does nothing 
anyway
>
Jörg
Li Wang Nov. 14, 2019, 7:59 a.m. UTC | #4
Hi Joerg,

On Thu, Nov 14, 2019 at 2:31 PM Joerg Vehlow <lkml@jv-coder.de> wrote:

> ...
> > From the original intention, it likely to set yes as the default, so
> > maybe the [action-if-not-given] should as  [with_expect=yes]?
> Maybe, but I did not want to change the current behavior here. This
> should be done in another patch.
>

Ok, sure.


> >
> >
> >     -# testcases/realtime requires bash and python.
> >     -if test "x$with_bash" = xyes && test "x$with_python" = xyes; then
> >     -    AC_ARG_WITH([realtime-testsuite],
> >     -      [AC_HELP_STRING([--with-realtime-testsuite],
> >     -        [compile and install the realtime testsuite (default=no)])],
> >     -      [with_realtime_testsuite=yes]
> >     -    )
> >     -fi
> >     +# TODO: testcases/realtime requires bash and python.
> >
> >
> > Why remove the judgment of bash/python here?
> It simply does not work as intended. See last part of my patch description:
>

Ah, sorry for missing this part.


> Defining AC_ARG_WITH within an if to express dependencies does not work.
> ./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
> even if with_bash=no or with_python=no. The check is removed completely.
>

Or, maybe we can make use of AS_IF here? and I noticed there are many
places that use "if test ..." in the configure.ac file. That looks tangly...


>
> I though removing it is better than leaving something that does nothing
> anyway
>

Indeed.
Joerg Vehlow Nov. 14, 2019, 8:09 a.m. UTC | #5
Hi,
>
>     Defining AC_ARG_WITH within an if to express dependencies does not
>     work.
>     ./configure --with-realtime-testsuite set with_realtime_testsuite=yes,
>     even if with_bash=no or with_python=no. The check is removed
>     completely.
>
>
> Or, maybe we can make use of AS_IF here? and I noticed there are many 
> places that use "if test ..." in the configure.ac 
> <http://configure.ac> file. That looks tangly...
I left a TODO in there so it can be fixed later, maybe together with 
other errors. I think this does not block merging this patch first?
The python requirement for realtime-testsuite is outdated/wrong anyway, 
if I remember correctly. A colleague of mine recently
investigated the python requirement and we came to the conclusion, that 
it is not needed for running the test.
I think it was used only for manual test result evaluation.

So there seems to be more work regarding dependencies between configure 
options.

Jörg
Li Wang Nov. 14, 2019, 8:36 a.m. UTC | #6
On Thu, Nov 14, 2019 at 4:09 PM Joerg Vehlow <lkml@jv-coder.de> wrote:

> Hi,
> >
> >     Defining AC_ARG_WITH within an if to express dependencies does not
> >     work.
> >     ./configure --with-realtime-testsuite set
> with_realtime_testsuite=yes,
> >     even if with_bash=no or with_python=no. The check is removed
> >     completely.
> >
> >
> > Or, maybe we can make use of AS_IF here? and I noticed there are many
> > places that use "if test ..." in the configure.ac
> > <http://configure.ac> file. That looks tangly...
> I left a TODO in there so it can be fixed later, maybe together with
> other errors. I think this does not block merging this patch first?
>

Well, I just saw adding new #TODO here and hoping to solve this together, a
simple way I was thinking is:

+AS_IF([test "x$with_bash" != xyes || test "x$with_python" != xyes],
+  [with_realtime_testsuite=no]
+)

Of course, It's fine to fix that in a separate patch!


> The python requirement for realtime-testsuite is outdated/wrong anyway,
> if I remember correctly. A colleague of mine recently
> investigated the python requirement and we came to the conclusion, that
> it is not needed for running the test.
> I think it was used only for manual test result evaluation.


> So there seems to be more work regarding dependencies between configure
> options.
>

Yes, thanks for investigating this. Looking forward to more patches :).

For this one:
    Reviewed-by: Li Wang <liwang@redhat.com>
Petr Vorel Nov. 18, 2019, 6:16 a.m. UTC | #7
Hi,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Well, I just saw adding new #TODO here and hoping to solve this together, a
> simple way I was thinking is:

> +AS_IF([test "x$with_bash" != xyes || test "x$with_python" != xyes],
> +  [with_realtime_testsuite=no]
> +)

> Of course, It's fine to fix that in a separate patch!

I'm for merging this. I just wonder if it's not better to remove $with_bash and
$with_python (as part of this patch), if they're not working.

BTW better, than fixing bash/shell would be to actually rewrote tests which are
using python and bash to C or posix shell.
https://github.com/linux-test-project/ltp/issues/547

Kind regards,
Petr
Li Wang Nov. 18, 2019, 6:38 a.m. UTC | #8
On Mon, Nov 18, 2019 at 2:16 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi,
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> > Well, I just saw adding new #TODO here and hoping to solve this
> together, a
> > simple way I was thinking is:
>
> > +AS_IF([test "x$with_bash" != xyes || test "x$with_python" != xyes],
> > +  [with_realtime_testsuite=no]
> > +)
>
> > Of course, It's fine to fix that in a separate patch!
>
> I'm for merging this. I just wonder if it's not better to remove
> $with_bash and
> $with_python (as part of this patch), if they're not working.
>

Me too. I think we could fix that in one patch. But as Joerg mentioned they
will be working on the #TODO. So maybe it doesn't matter to do right now or
later.

>
> BTW better, than fixing bash/shell would be to actually rewrote tests
> which are
> using python and bash to C or posix shell.
> https://github.com/linux-test-project/ltp/issues/547
>
> Kind regards,
> Petr
>
>
Joerg Vehlow Nov. 18, 2019, 6:40 a.m. UTC | #9
Hi Petr,
> I'm for merging this. I just wonder if it's not better to remove $with_bash and
> $with_python (as part of this patch), if they're not working.
I think you misunderstood. I did not say, that Bash and Python switches 
were not working,
just that the dependency of bash and python for realtime tests did not 
work -> it was not a dependency.
The realtime testsuite could be enabled without bash and python enabled.
Petr Vorel Nov. 18, 2019, 8:59 a.m. UTC | #10
Hi Joerg,

> Hi Petr,
> > I'm for merging this. I just wonder if it's not better to remove $with_bash and
> > $with_python (as part of this patch), if they're not working.
> I think you misunderstood. I did not say, that Bash and Python switches were
> not working,
> just that the dependency of bash and python for realtime tests did not work
> -> it was not a dependency.
> The realtime testsuite could be enabled without bash and python enabled.
I understood :). It's just that this whole Bash and Python switches are meant to
be just for realtime testsuite (therefore completely useless, if they're not
working for the only use). As we generally don't want to depend on either
python nor bash, this switches will be removed sooner or later.
And our autoconf needs more love, it's obscure.
https://github.com/linux-test-project/ltp/issues/508

But ok, it's a separate problem, I'm for merging it as it is.

Kind regards,
Petr
Petr Vorel Nov. 18, 2019, 8:41 p.m. UTC | #11
Hi Jörg, Li,

merged (the original version with bash and python kept).

Kind regards,
Petr

Patch
diff mbox series

diff --git a/configure.ac b/configure.ac
index 62c5a0bb4..4b7c6d57c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -108,8 +108,9 @@  AC_CHECK_FUNCS([ \
 # Expect
 AC_ARG_WITH([bash],
   [AC_HELP_STRING([--with-bash],
-    [have the Bourne Again SHell interpreter (default=no)])],
-  [with_bash=yes],
+    [have the Bourne Again Shell interpreter])],
+  [with_bash=$withval],
+  [with_bash=no]
 )
 if test "x$with_bash" = xyes; then
     AC_SUBST([WITH_BASH],["yes"])
@@ -119,8 +120,8 @@  fi
 
 AC_ARG_WITH([expect],
   [AC_HELP_STRING([--with-expect],
-    [have the Tcl/expect library (default=yes)])],
-  [with_expect=yes],
+    [have the Tcl/expect library])],
+  [with_expect=$withval],
   [with_expect=no]
 )
 if test "x$with_expect" = xyes; then
@@ -132,16 +133,16 @@  fi
 # Numa
 AC_ARG_WITH([numa],
   AC_HELP_STRING([--without-numa],
-    [without numa support (default=no)]),
-  [with_numa=no],
+    [without numa support]),
+  [with_numa=$withval],
   [with_numa=yes]
 )
 
 # Perl
 AC_ARG_WITH([perl],
   [AC_HELP_STRING([--with-perl],
-    [have a perl interpreter (default=yes)])],
-  [with_perl=yes],
+    [have a perl interpreter])],
+  [with_perl=$withval],
   [with_perl=no]
 )
 if test "x$with_perl" = xyes; then
@@ -153,8 +154,8 @@  fi
 # Python
 AC_ARG_WITH([python],
   [AC_HELP_STRING([--with-python],
-    [have a python interpreter (default=yes)])],
-  [with_python=yes],
+    [have a python interpreter])],
+  [with_python=$withval],
   [with_python=no]
 )
 if test "x$with_python" = xyes; then
@@ -166,8 +167,8 @@  fi
 # TI RPC
 AC_ARG_WITH([tirpc],
   AC_HELP_STRING([--without-tirpc],
-    [without libtirpc support (default=no)]),
-  [with_tirpc=no],
+    [without libtirpc support]),
+  [with_tirpc=$withval],
   [with_tirpc=yes]
 )
 # END tools knobs
@@ -176,8 +177,9 @@  AC_ARG_WITH([tirpc],
 
 AC_ARG_WITH([open-posix-testsuite],
   [AC_HELP_STRING([--with-open-posix-testsuite],
-    [compile and install the open posix testsuite (default=no)])],
-  [with_open_posix_testsuite=$withval]
+    [compile and install the open posix testsuite])],
+  [with_open_posix_testsuite=$withval],
+  [with_open_posix_testsuite=no]
 )
 if test "x$with_open_posix_testsuite" = xyes; then
     AC_SUBST([WITH_OPEN_POSIX_TESTSUITE],["yes"])
@@ -185,14 +187,14 @@  else
     AC_SUBST([WITH_OPEN_POSIX_TESTSUITE],["no"])
 fi
 
-# testcases/realtime requires bash and python.
-if test "x$with_bash" = xyes && test "x$with_python" = xyes; then
-    AC_ARG_WITH([realtime-testsuite],
-      [AC_HELP_STRING([--with-realtime-testsuite],
-        [compile and install the realtime testsuite (default=no)])],
-      [with_realtime_testsuite=yes]
-    )
-fi
+# TODO: testcases/realtime requires bash and python.
+AC_ARG_WITH([realtime-testsuite],
+  [AC_HELP_STRING([--with-realtime-testsuite],
+    [compile and install the realtime testsuite])],
+  [with_realtime_testsuite=$withval],
+  [with_realtime_testsuite=no]
+)
+
 if test "x$with_realtime_testsuite" = xyes; then
     AC_SUBST([WITH_REALTIME_TESTSUITE],["yes"])
     # Run configure on testcases/realtime as well.