| Message ID | 20191113104149.42407-1-lkml@jv-coder.de |
|---|---|
| State | Accepted |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | configure.ac: Fix --with* options | expand |
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
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 > >
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
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.
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
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>
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
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 > >
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.
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
Hi Jörg, Li, merged (the original version with bash and python kept). Kind regards, Petr
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.