[PATCH for-4.14] tools: check go compiler version if present

[PATCH for-4.14] tools: check go compiler version if present

[Nick Rosbrook]

Currently, no minimum go compiler version is required by the configure
scripts. However, the go bindings actually will not build with some
older versions of go. Add a check for a minimum go version of 1.11.1 in
accordance with tools/golang/xenlight/go.mod.

Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 m4/golang.m4       |  4 ++++
 tools/configure    | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/configure.ac |  7 +++++++
 3 files changed, 60 insertions(+)

diff --git a/m4/golang.m4 b/m4/golang.m4
index 0b4bd54ce0..9cfd7e00a5 100644
--- a/m4/golang.m4
+++ b/m4/golang.m4
@@ -1,4 +1,8 @@
 AC_DEFUN([AC_PROG_GO], [
     dnl Check for the go compiler
     AC_CHECK_TOOL([GO],[go],[no])
+
+    if test "$GO" != "no"; then
+        GOVERSION=`$GO version | cut -d " " -f 3 | sed "s/go//"`
+    fi
 ])
diff --git a/tools/configure b/tools/configure
index b3668ec98d..f3f19c1a90 100755
--- a/tools/configure
+++ b/tools/configure
@@ -6845,6 +6845,10 @@ else
 fi
 
 
+    if test "$GO" != "no"; then
+        GOVERSION=`$GO version | cut -d " " -f 3 | sed "s/go//"`
+    fi
+
     if test "x$GO" = "xno"; then :
 
         if test "x$enable_golang" =  "xyes"; then :
@@ -6854,6 +6858,51 @@ fi
 fi
         golang="n"
 
+else
+
+
+
+
+  # Used to indicate true or false condition
+  ax_compare_version=false
+
+  # Convert the two version strings to be compared into a format that
+  # allows a simple string comparison.  The end result is that a version
+  # string of the form 1.12.5-r617 will be converted to the form
+  # 0001001200050617.  In other words, each number is zero padded to four
+  # digits, and non digits are removed.
+
+  ax_compare_version_A=`echo "$GOVERSION" | sed -e 's/\([0-9]*\)/Z\1Z/g' \
+                     -e 's/Z\([0-9]\)Z/Z0\1Z/g' \
+                     -e 's/Z\([0-9][0-9]\)Z/Z0\1Z/g' \
+                     -e 's/Z\([0-9][0-9][0-9]\)Z/Z0\1Z/g' \
+                     -e 's/[^0-9]//g'`
+
+
+  ax_compare_version_B=`echo "1.11.1" | sed -e 's/\([0-9]*\)/Z\1Z/g' \
+                     -e 's/Z\([0-9]\)Z/Z0\1Z/g' \
+                     -e 's/Z\([0-9][0-9]\)Z/Z0\1Z/g' \
+                     -e 's/Z\([0-9][0-9][0-9]\)Z/Z0\1Z/g' \
+                     -e 's/[^0-9]//g'`
+
+
+    ax_compare_version=`echo "x$ax_compare_version_A
+x$ax_compare_version_B" | sed 's/^ *//' | sort -r | sed "s/x${ax_compare_version_A}/false/;s/x${ax_compare_version_B}/true/;1q"`
+
+
+
+    if test "$ax_compare_version" = "true" ; then
+
+            if test "x$enable_golang" = "xyes"; then :
+
+                as_fn_error $? "\"Your version of go: $GOVERSION is not supported\"" "$LINENO" 5
+
+fi
+            golang="n"
+
+      fi
+
+
 fi
 
 fi
diff --git a/tools/configure.ac b/tools/configure.ac
index a9af0a21c6..9d126b7a14 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -338,6 +338,13 @@ AS_IF([test "x$golang" = "xy"], [
             AC_MSG_ERROR([Go tools enabled, but missing go compiler])
         ])
         golang="n"
+    ], [
+        AX_COMPARE_VERSION([$GOVERSION], [lt], [1.11.1], [
+            AS_IF([test "x$enable_golang" = "xyes"], [
+                AC_MSG_ERROR(["Your version of go: $GOVERSION is not supported"])
+            ])
+            golang="n"
+        ])
     ])
 ])
 
-- 
2.17.1

Re: [PATCH for-4.14] tools: check go compiler version if present

[Ian Jackson]

Nick Rosbrook writes ("[PATCH for-4.14] tools: check go compiler version if present"):
> Currently, no minimum go compiler version is required by the configure
> scripts. However, the go bindings actually will not build with some
> older versions of go. Add a check for a minimum go version of 1.11.1 in
> accordance with tools/golang/xenlight/go.mod.

> diff --git a/tools/configure.ac b/tools/configure.ac
> index a9af0a21c6..9d126b7a14 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -338,6 +338,13 @@ AS_IF([test "x$golang" = "xy"], [
>              AC_MSG_ERROR([Go tools enabled, but missing go compiler])
>          ])
>          golang="n"
> +    ], [
> +        AX_COMPARE_VERSION([$GOVERSION], [lt], [1.11.1], [
> +            AS_IF([test "x$enable_golang" = "xyes"], [
> +                AC_MSG_ERROR(["Your version of go: $GOVERSION is not supported"])
> +            ])
> +            golang="n"
> +        ])
>      ])
>  ])

I don't understand this code.  Why are you checking $enable_golang in
your new code whereas the old code checks $golang ?  I actually read
the generated code trying to see where $golang is set and AFAICT it is
only ever set to n ?

This is all very weird.  Surely golang should be enabled by default
when the proper compiler is present, and disabled by default
otherwise.  I think this effect will be quite hard to achieve with
AX_ARG_DEFAULT_ENABLE.  Probably you should be using AC_ARG_ENABLE
and doing the defaulting yourself so that you can use a computed
default etc.

The docs are not clear but reading the code, AC_ARG_ENABLE sets the
variable enable_foo to "no" if --disable-foo is given, to "" if
--enable-foo is given, or to the value given to
--enable-foo=VALUE.

Ian.

Re: [PATCH for-4.14] tools: check go compiler version if present

[Nick Rosbrook]

On Fri, Jun 12, 2020 at 05:40:21PM +0100, Ian Jackson wrote:
> Nick Rosbrook writes ("[PATCH for-4.14] tools: check go compiler version if present"):
> > Currently, no minimum go compiler version is required by the configure
> > scripts. However, the go bindings actually will not build with some
> > older versions of go. Add a check for a minimum go version of 1.11.1 in
> > accordance with tools/golang/xenlight/go.mod.
> 
> > diff --git a/tools/configure.ac b/tools/configure.ac
> > index a9af0a21c6..9d126b7a14 100644
> > --- a/tools/configure.ac
> > +++ b/tools/configure.ac
> > @@ -338,6 +338,13 @@ AS_IF([test "x$golang" = "xy"], [
> >              AC_MSG_ERROR([Go tools enabled, but missing go compiler])
> >          ])
> >          golang="n"
> > +    ], [
> > +        AX_COMPARE_VERSION([$GOVERSION], [lt], [1.11.1], [
> > +            AS_IF([test "x$enable_golang" = "xyes"], [
> > +                AC_MSG_ERROR(["Your version of go: $GOVERSION is not supported"])
> > +            ])
> > +            golang="n"
> > +        ])
> >      ])
> >  ])
> 
> I don't understand this code.  Why are you checking $enable_golang in
> your new code whereas the old code checks $golang ?  I actually read
> the generated code trying to see where $golang is set and AFAICT it is
> only ever set to n ?

For some background, in an attempt to be consistent with existing code
here, I basically copied the logic for enabling the ocaml tools. 

The logic is setup in a way that (unless --disable-golang is set) if a
suitable version of the go compiler is found, then golang is enabled by
default. If, however, a suitable go compiler is not found (either go
is not present at all, or it is too old), then golang is disabled. This
part happens silently unless --enable-golang is _explicitly_ set by the
user, in which case an error is thrown by ./configure. This is why
$enable_golang is checked.

> 
> This is all very weird.  Surely golang should be enabled by default
> when the proper compiler is present, and disabled by default
> otherwise.  I think this effect will be quite hard to achieve with
> AX_ARG_DEFAULT_ENABLE.  Probably you should be using AC_ARG_ENABLE
> and doing the defaulting yourself so that you can use a computed
> default etc.

I believe the behavior you described here is the same as I described
above (and is acheived in this patch). But, I'm happy to re-work the
implementation if necessary so that the code is more clear.

Thanks,
-NR

Re: [PATCH for-4.14] tools: check go compiler version if present

[George Dunlap]

> On Jun 12, 2020, at 3:31 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
> Currently, no minimum go compiler version is required by the configure
> scripts. However, the go bindings actually will not build with some
> older versions of go. Add a check for a minimum go version of 1.11.1 in
> accordance with tools/golang/xenlight/go.mod.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

There’s a good chance I won’t have time to review the code for this at all this week; but regarding the intention of having 1.11.1 as a minimum version:

Acked-by: George Dunlap <george.dunlap@citrix.com>

Re: [PATCH for-4.14] tools: check go compiler version if present

[Ian Jackson]

Nick Rosbrook writes ("Re: [PATCH for-4.14] tools: check go compiler version if present"):
> On Fri, Jun 12, 2020 at 05:40:21PM +0100, Ian Jackson wrote:
> > Nick Rosbrook writes ("[PATCH for-4.14] tools: check go compiler version if present"):
> > > Currently, no minimum go compiler version is required by the configure
> > > scripts. However, the go bindings actually will not build with some
> > > older versions of go. Add a check for a minimum go version of 1.11.1 in
> > > accordance with tools/golang/xenlight/go.mod.
...
> > I don't understand this code.  Why are you checking $enable_golang in
> > your new code whereas the old code checks $golang ?  I actually read
> > the generated code trying to see where $golang is set and AFAICT it is
> > only ever set to n ?
> 
> For some background, in an attempt to be consistent with existing code
> here, I basically copied the logic for enabling the ocaml tools. 
> 
> The logic is setup in a way that (unless --disable-golang is set) if a
> suitable version of the go compiler is found, then golang is enabled by
> default. If, however, a suitable go compiler is not found (either go
> is not present at all, or it is too old), then golang is disabled. This
> part happens silently unless --enable-golang is _explicitly_ set by the
> user, in which case an error is thrown by ./configure. This is why
> $enable_golang is checked.

Thanks.  Well, I have to say I still don't understand the code.

But as you note, the behaviour you describe is the one I would want.
And the confusingness doesn't seem to have been your fault.  It would
probably be worse to have two different arrangements.  Let's leave it
as it is for now.

> > This is all very weird.  Surely golang should be enabled by default
> > when the proper compiler is present, and disabled by default
> > otherwise.  I think this effect will be quite hard to achieve with
> > AX_ARG_DEFAULT_ENABLE.  Probably you should be using AC_ARG_ENABLE
> > and doing the defaulting yourself so that you can use a computed
> > default etc.
> 
> I believe the behavior you described here is the same as I described
> above (and is acheived in this patch). But, I'm happy to re-work the
> implementation if necessary so that the code is more clear.

Ideally at some point maybe we would make this clearer but not now.

Have you tested the situations you describe ?  That might be a better
way of checking that it's right than the code inspection which is
obviously failing for me now...

I definitely think we want to fix this for 4.14.  So thanks for your
continued help!

Ian.

Re: [PATCH for-4.14] tools: check go compiler version if present

[Nick Rosbrook]

> Ideally at some point maybe we would make this clearer but not now.

Okay, sounds good.

> Have you tested the situations you describe ?  That might be a better
> way of checking that it's right than the code inspection which is
> obviously failing for me now...

Yes, I have tested the following situations:

  (1) ./configure without go installed => go is not enabled
  (2) ./configure with go version < 1.11.1 => go is not enabled
  (3) ./configure with go version > 1.11.1 => go is enabled
  (4) ./configure --enable-golang without go installed => error
  (5) ./configure --enable-golang with go < 1.11.1 => error
  (6) ./configure --disable-golang in any case => go is not enabled
> 
> I definitely think we want to fix this for 4.14.  So thanks for your
> continued help!

You're welcome. Thank you again for the review.

-NR

Re: [PATCH for-4.14] tools: check go compiler version if present

[Ian Jackson]

Nick Rosbrook writes ("Re: [PATCH for-4.14] tools: check go compiler version if present"):
> > Ideally at some point maybe we would make this clearer but not now.
> 
> Okay, sounds good.
> 
> > Have you tested the situations you describe ?  That might be a better
> > way of checking that it's right than the code inspection which is
> > obviously failing for me now...
> 
> Yes, I have tested the following situations:
> 
>   (1) ./configure without go installed => go is not enabled
>   (2) ./configure with go version < 1.11.1 => go is not enabled
>   (3) ./configure with go version > 1.11.1 => go is enabled
>   (4) ./configure --enable-golang without go installed => error
>   (5) ./configure --enable-golang with go < 1.11.1 => error
>   (6) ./configure --disable-golang in any case => go is not enabled

Thorough!

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Paul, this should go into 4.14.  Can I have a release ack plase ?

Thanks,
Ian.

RE: [PATCH for-4.14] tools: check go compiler version if present

[Paul Durrant]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 15 June 2020 15:42
> To: Nick Rosbrook <rosbrookn@gmail.com>
> Cc: xen-devel@lists.xenproject.org; paul@xen.org; George Dunlap <George.Dunlap@citrix.com>; Nick
> Rosbrook <rosbrookn@ainfosec.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH for-4.14] tools: check go compiler version if present
> 
> Nick Rosbrook writes ("Re: [PATCH for-4.14] tools: check go compiler version if present"):
> > > Ideally at some point maybe we would make this clearer but not now.
> >
> > Okay, sounds good.
> >
> > > Have you tested the situations you describe ?  That might be a better
> > > way of checking that it's right than the code inspection which is
> > > obviously failing for me now...
> >
> > Yes, I have tested the following situations:
> >
> >   (1) ./configure without go installed => go is not enabled
> >   (2) ./configure with go version < 1.11.1 => go is not enabled
> >   (3) ./configure with go version > 1.11.1 => go is enabled
> >   (4) ./configure --enable-golang without go installed => error
> >   (5) ./configure --enable-golang with go < 1.11.1 => error
> >   (6) ./configure --disable-golang in any case => go is not enabled
> 
> Thorough!
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Paul, this should go into 4.14.  Can I have a release ack plase ?

You may...

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> Thanks,
> Ian.

RE: [PATCH for-4.14] tools: check go compiler version if present

[Ian Jackson]

Paul Durrant writes ("RE: [PATCH for-4.14] tools: check go compiler version if present"):
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Paul, this should go into 4.14.  Can I have a release ack plase ?
> 
> You may...
> 
> Release-acked-by: Paul Durrant <paul@xen.org>

Thanks, pushed.

Ian.