m4/golang.m4 | 4 ++++ tools/configure | 49 ++++++++++++++++++++++++++++++++++++++++++++++ tools/configure.ac | 7 +++++++ 3 files changed, 60 insertions(+)
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
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.
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
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.
> 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
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.
> -----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.
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.
> 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>
© 2016 - 2024 Red Hat, Inc.