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

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

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