[PATCH for-4.15] tools/configure: add bison as mandatory

[PATCH for-4.15] tools/configure: add bison as mandatory

[Roger Pau Monne]

Bison is now mandatory when the pvshim build is enabled in order to
generate the Kconfig.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Please re-run autogen.sh after applying.

Fallout from this patch can lead to broken configure script being
generated or bison not detected correctly, but those will be cached
quite quickly by the automated testing.
---
 tools/configure.ac | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/configure.ac b/tools/configure.ac
index 5b328700e0..f4e3fccdb0 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -308,7 +308,6 @@ AC_ARG_VAR([AWK], [Path to awk tool])
 AC_PROG_CC
 AC_PROG_MAKE_SET
 AC_PROG_INSTALL
-AC_PATH_PROG([BISON], [bison])
 AC_PATH_PROG([FLEX], [flex])
 AX_PATH_PROG_OR_FAIL([PERL], [perl])
 AX_PATH_PROG_OR_FAIL([AWK], [awk])
@@ -516,5 +515,10 @@ AC_ARG_ENABLE([pvshim],
     esac
 ])
 AC_SUBST(pvshim)
+AS_IF([test "x$pvshim" = "xy"], [
+    AX_PATH_PROG_OR_FAIL([BISON], [bison])
+], [
+    AC_PATH_PROG([BISON], [bison])
+])
 
 AC_OUTPUT()
-- 
2.29.2

Re: [PATCH for-4.15] tools/configure: add bison as mandatory

[Andrew Cooper]

On 05/02/2021 11:53, Roger Pau Monne wrote:
> Bison is now mandatory when the pvshim build is enabled in order to
> generate the Kconfig.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Please re-run autogen.sh after applying.
>
> Fallout from this patch can lead to broken configure script being
> generated or bison not detected correctly, but those will be cached
> quite quickly by the automated testing.

I think this can be simpler.  Both flex and bison are mandatory libxlutil.

i.e. they should both simply to convert to _OR_FAIL variants in place.

~Andrew

> ---
>  tools/configure.ac | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 5b328700e0..f4e3fccdb0 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -308,7 +308,6 @@ AC_ARG_VAR([AWK], [Path to awk tool])
>  AC_PROG_CC
>  AC_PROG_MAKE_SET
>  AC_PROG_INSTALL
> -AC_PATH_PROG([BISON], [bison])
>  AC_PATH_PROG([FLEX], [flex])
>  AX_PATH_PROG_OR_FAIL([PERL], [perl])
>  AX_PATH_PROG_OR_FAIL([AWK], [awk])
> @@ -516,5 +515,10 @@ AC_ARG_ENABLE([pvshim],
>      esac
>  ])
>  AC_SUBST(pvshim)
> +AS_IF([test "x$pvshim" = "xy"], [
> +    AX_PATH_PROG_OR_FAIL([BISON], [bison])
> +], [
> +    AC_PATH_PROG([BISON], [bison])
> +])
>  
>  AC_OUTPUT()

Re: [PATCH for-4.15] tools/configure: add bison as mandatory

[Roger Pau Monné]

On Fri, Feb 05, 2021 at 01:34:20PM +0000, Andrew Cooper wrote:
> On 05/02/2021 11:53, Roger Pau Monne wrote:
> > Bison is now mandatory when the pvshim build is enabled in order to
> > generate the Kconfig.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Please re-run autogen.sh after applying.
> >
> > Fallout from this patch can lead to broken configure script being
> > generated or bison not detected correctly, but those will be cached
> > quite quickly by the automated testing.
> 
> I think this can be simpler.  Both flex and bison are mandatory libxlutil.

No, we ship the output .c and .h files so that the user only needs to
have bison/flex if it wants to modify the .l or .y files AFAICT?

Thanks, Roger.

Re: [PATCH for-4.15] tools/configure: add bison as mandatory

[Andrew Cooper]

On 05/02/2021 13:39, Roger Pau Monné wrote:
> On Fri, Feb 05, 2021 at 01:34:20PM +0000, Andrew Cooper wrote:
>> On 05/02/2021 11:53, Roger Pau Monne wrote:
>>> Bison is now mandatory when the pvshim build is enabled in order to
>>> generate the Kconfig.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Please re-run autogen.sh after applying.
>>>
>>> Fallout from this patch can lead to broken configure script being
>>> generated or bison not detected correctly, but those will be cached
>>> quite quickly by the automated testing.
>> I think this can be simpler.  Both flex and bison are mandatory libxlutil.
> No, we ship the output .c and .h files so that the user only needs to
> have bison/flex if it wants to modify the .l or .y files AFAICT?

I know that theory, but it is broken in practice because of `git
checkout` timestamps.

Also, the Makefile explicitly enforces the checks, so they are mandatory
in despite an attempt to ship the preprocessed form.

~Andrew

Re: [PATCH for-4.15] tools/configure: add bison as mandatory

[Roger Pau Monné]

On Fri, Feb 05, 2021 at 01:50:27PM +0000, Andrew Cooper wrote:
> On 05/02/2021 13:39, Roger Pau Monné wrote:
> > On Fri, Feb 05, 2021 at 01:34:20PM +0000, Andrew Cooper wrote:
> >> On 05/02/2021 11:53, Roger Pau Monne wrote:
> >>> Bison is now mandatory when the pvshim build is enabled in order to
> >>> generate the Kconfig.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Please re-run autogen.sh after applying.
> >>>
> >>> Fallout from this patch can lead to broken configure script being
> >>> generated or bison not detected correctly, but those will be cached
> >>> quite quickly by the automated testing.
> >> I think this can be simpler.  Both flex and bison are mandatory libxlutil.
> > No, we ship the output .c and .h files so that the user only needs to
> > have bison/flex if it wants to modify the .l or .y files AFAICT?
> 
> I know that theory, but it is broken in practice because of `git
> checkout` timestamps.
> 
> Also, the Makefile explicitly enforces the checks, so they are mandatory
> in despite an attempt to ship the preprocessed form.

I seem to be able to `make -C tools/libs/util/` just fine without
having bison installed. If we do require bison/flex then we certainly
need to remove the output *.c/*.h files from tools/libs/util/.

I'm not specially thrilled either way, but I think the proposed patch
is safer given the point of the release we are at.

Thanks, Roger.

Re: [PATCH for-4.15] tools/configure: add bison as mandatory

[Jan Beulich]

On 05.02.2021 14:50, Andrew Cooper wrote:
> On 05/02/2021 13:39, Roger Pau Monné wrote:
>> On Fri, Feb 05, 2021 at 01:34:20PM +0000, Andrew Cooper wrote:
>>> On 05/02/2021 11:53, Roger Pau Monne wrote:
>>>> Bison is now mandatory when the pvshim build is enabled in order to
>>>> generate the Kconfig.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>> Please re-run autogen.sh after applying.
>>>>
>>>> Fallout from this patch can lead to broken configure script being
>>>> generated or bison not detected correctly, but those will be cached
>>>> quite quickly by the automated testing.
>>> I think this can be simpler.  Both flex and bison are mandatory libxlutil.
>> No, we ship the output .c and .h files so that the user only needs to
>> have bison/flex if it wants to modify the .l or .y files AFAICT?
> 
> I know that theory, but it is broken in practice because of `git
> checkout` timestamps.
> 
> Also, the Makefile explicitly enforces the checks, so they are mandatory
> in despite an attempt to ship the preprocessed form.

I don't see the Makefile enforcing anything. Upon seeing "XYZ is
needed to rebuild some libxl parsers and scanners, please install
it and rerun configure" you then have the choice of doing so or,
if you know you didn't fiddle with the sources, playing with the
time stamps.

Jan

[PATCH for-4.15] tools/configure: add bison as mandatory

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-4.15] tools/configure: add bison as mandatory"):
> Bison is now mandatory when the pvshim build is enabled in order to
> generate the Kconfig.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Please re-run autogen.sh after applying.
> 
> Fallout from this patch can lead to broken configure script being
> generated or bison not detected correctly, but those will be cached
> quite quickly by the automated testing.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>

I've read the rest of the thread and I prefer Roger's version of this
patch.

Thanks,
Ian.