The generated golang bindings (types.gen.go and helpers.gen.go) are
left checked in so that they can be fetched from xenbits using the
golang tooling.  This means that they must be updated whenever
libxl_types.idl (or other dependencies) are updated.  However, the
golang bindings are only built optionally; we can't assume that anyone
updating libxl_types.idl will also descend into the tools/golang tree
to re-generate the bindings.

Fix this by re-generating the golang bindings from the libxl Makefile
when the IDL dependencies are updated, so that anyone who updates
libxl_types.idl will also end up updating the golang generated files
as well.

 - Make a variable for the generated files, and a target in
   xenlight/Makefile which will only re-generate the files.

 - Add a target in libxl/Makefile to call external idl generation
   targets (currently only golang).

For ease of testing, also add a specific target in libxl/Makefile just
to check and update files generated from the IDL.

This does mean that there are two potential paths for generating the
files during a parallel build; but that shouldn't be an issue, since
tools/golang/xenlight should never be built until after tools/libxl
has completed building anyway.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2:
- Add comments explaining parallel make safety

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/Makefile | 11 ++++++++++-
 tools/libxl/Makefile           | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
index cd0a62505f..8ab4cb5665 100644
--- a/tools/golang/xenlight/Makefile
+++ b/tools/golang/xenlight/Makefile
@@ -17,12 +17,21 @@ all: build
 .PHONY: package
 package: $(XEN_GOPATH)$(GOXL_PKG_DIR)
 
-$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: xenlight.go types.gen.go helpers.gen.go
+GOXL_GEN_FILES = types.gen.go helpers.gen.go
+
+$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: xenlight.go $(GOXL_GEN_FILES)
 	$(INSTALL_DIR) $(XEN_GOPATH)$(GOXL_PKG_DIR)
 	$(INSTALL_DATA) xenlight.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
 	$(INSTALL_DATA) types.gen.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
 	$(INSTALL_DATA) helpers.gen.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
 
+# NOTE: This target is called from libxl/Makefile:all.  Since that
+# target must finish before golang/Makefile is called, this is
+# currently safe.  It must not be called from anywhere else in the
+# Makefile system without careful thought about races with
+# xenlight/Makefile:all
+idl-gen: $(GOXL_GEN_FILES)
+
 %.gen.go: gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl $(LIBXL_SRC_DIR)/idl.py
 	XEN_ROOT=$(XEN_ROOT) $(PYTHON) gengotypes.py $(LIBXL_SRC_DIR)/libxl_types.idl
 
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 69fcf21577..947eb6036e 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -218,7 +218,7 @@ testidl.c: libxl_types.idl gentest.py libxl.h $(AUTOINCS)
 .PHONY: all
 all: $(CLIENTS) $(TEST_PROGS) $(PKG_CONFIG) $(PKG_CONFIG_LOCAL) \
 		libxenlight.so libxenlight.a libxlutil.so libxlutil.a \
-	$(AUTOSRCS) $(AUTOINCS)
+	$(AUTOSRCS) $(AUTOINCS) idl-external
 
 $(LIBXL_OBJS) $(LIBXLU_OBJS) $(SAVE_HELPER_OBJS) \
 		$(LIBXL_TEST_OBJS) $(TEST_PROG_OBJS): \
@@ -274,6 +274,21 @@ _libxl_type%.h _libxl_type%_json.h _libxl_type%_private.h _libxl_type%.c: libxl_
 	$(call move-if-changed,__libxl_type$(stem)_json.h,_libxl_type$(stem)_json.h)
 	$(call move-if-changed,__libxl_type$(stem).c,_libxl_type$(stem).c)
 
+# NOTE: This is safe to do at the moment because idl-external and
+# idl-gen are only called from libxl/Makefile:all, which must return
+# before golang/Makefile is callid.  idl-external and idl-gen must
+# never be called from another part of the make system without careful thought
+# about races with tools/golang/xenlight/Makefile:all
+.PHONY: idl-external
+idl-external:
+	$(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen
+
+LIBXL_IDLGEN_FILES = _libxl_types.h _libxl_types_json.h _libxl_types_private.h _libxl_types.c \
+	_libxl_types_internal.h _libxl_types_internal_json.h _libxl_types_internal_private.h _libxl_types_internal.c
+
+
+idl-gen: $(LIBXL_GEN_FILES) idl-external
+
 libxenlight.so: libxenlight.so.$(MAJOR)
 	$(SYMLINK_SHLIB) $< $@
 
-- 
2.25.1

> On May 26, 2020, at 11:16 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> 
> The generated golang bindings (types.gen.go and helpers.gen.go) are
> left checked in so that they can be fetched from xenbits using the
> golang tooling.  This means that they must be updated whenever
> libxl_types.idl (or other dependencies) are updated.  However, the
> golang bindings are only built optionally; we can't assume that anyone
> updating libxl_types.idl will also descend into the tools/golang tree
> to re-generate the bindings.
> 
> Fix this by re-generating the golang bindings from the libxl Makefile
> when the IDL dependencies are updated, so that anyone who updates
> libxl_types.idl will also end up updating the golang generated files
> as well.
> 
> - Make a variable for the generated files, and a target in
>   xenlight/Makefile which will only re-generate the files.
> 
> - Add a target in libxl/Makefile to call external idl generation
>   targets (currently only golang).

So as written this turns out not to work correctly:  `gengotypes.py` spits out syntactically valid but unformatted Go code, and then runs `go fmt` on it to get the right style (including tabs, &c).  But the error code of `go fmt` isn’t checked; so on a system without go installed, if the build system decides it needs to re-run `gengotypes.py` for whatever reason, the build succeeds but the tree ends up “dirtied” with an unformatted version fo the generated text.

The simplest short-term way to fix this would be to remove the `go fmt` call from `gengotypes.py`.  It’s actually relatively unusual for generated code to look pretty (or even be looked at).  We could also consider adding in some “manual” formatting in gengotypes.py, like indentation, so that it doesn’t look too terrible.

Nick, do you have time to work on a patch like that?

 -George

> The simplest short-term way to fix this would be to remove the `go fmt` call from `gengotypes.py`.  It’s actually relatively unusual for generated code to look pretty (or even be looked at).  We could also consider adding in some “manual” formatting in gengotypes.py, like indentation, so that it doesn’t look too terrible.
> 
> Nick, do you have time to work on a patch like that?

Yes, I have time to work on a quick patch for this. I'll see what it
would take to add a bit of basic manual formatting, but of course the
original purpose of using gofmt was to avoid re-creating formatting
logic. I'll likely just remove the call to go fmt.

Out of curiosity, would it be totally out of the question to require
having gofmt installed (not for 4.14, but in the future)? I ask because
I haven't seen it discussed one way or the other.

Thanks,
-NR

> On Jun 4, 2020, at 7:29 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
>> The simplest short-term way to fix this would be to remove the `go fmt` call from `gengotypes.py`.  It’s actually relatively unusual for generated code to look pretty (or even be looked at).  We could also consider adding in some “manual” formatting in gengotypes.py, like indentation, so that it doesn’t look too terrible.
>> 
>> Nick, do you have time to work on a patch like that?
> 
> Yes, I have time to work on a quick patch for this. I'll see what it
> would take to add a bit of basic manual formatting, but of course the
> original purpose of using gofmt was to avoid re-creating formatting
> logic. I'll likely just remove the call to go fmt.
> 
> Out of curiosity, would it be totally out of the question to require
> having gofmt installed (not for 4.14, but in the future)? I ask because
> I haven't seen it discussed one way or the other.

I think I’d like to try to avoid it, unless / until we have a “core component” written in golang.  For one, if we added it as a core dependency, we’d need to be  backwards compatible to the oldest version of golang of distros on which we want to build; that would include  Debian oldstable, Ubuntu LTS, probably CentOS 7 at least, possibly CentOS 6, and so on.  If any of those didn’t have golang available, then we’d basically have to decide between dropping support for those distros, and making golang optional.

I don’t at the moment have a good feel for what other people in the community feel about this, but generally speaking being fanatically backwards compatible is an investment in the long-term ecosystem; keeping Xen *as a whole* building on older distros is an example of that.  (FWIW I don’t think we have an official rubric, but my starting point is that we should try to build on all still-supported major distros; that would include CentOS 6 until Q4 of 2020, if my quick Google search is correct.)

One advantage of making golang optional is that we don’t have to be quite so backwards compatible — up until we declare the feature “fully supported”, we can move the minimum required version forward at will if we want to rely on new features.

 -George

> > Out of curiosity, would it be totally out of the question to require
> > having gofmt installed (not for 4.14, but in the future)? I ask because
> > I haven't seen it discussed one way or the other.
> 
> I think I’d like to try to avoid it, unless / until we have a “core component” written in golang.  For one, if we added it as a core dependency, we’d need to be  backwards compatible to the oldest version of golang of distros on which we want to build; that would include  Debian oldstable, Ubuntu LTS, probably CentOS 7 at least, possibly CentOS 6, and so on.  If any of those didn’t have golang available, then we’d basically have to decide between dropping support for those distros, and making golang optional.
> 
> I don’t at the moment have a good feel for what other people in the community feel about this, but generally speaking being fanatically backwards compatible is an investment in the long-term ecosystem; keeping Xen *as a whole* building on older distros is an example of that.  (FWIW I don’t think we have an official rubric, but my starting point is that we should try to build on all still-supported major distros; that would include CentOS 6 until Q4 of 2020, if my quick Google search is correct.)
> 
> One advantage of making golang optional is that we don’t have to be quite so backwards compatible — up until we declare the feature “fully supported”, we can move the minimum required version forward at will if we want to rely on new features.

That all makes sense, thanks for explaining.

-NR

George Dunlap writes ("Re: [PATCH v2 1/5] libxl: Generate golang bindings in libxl Makefile"):
> So as written this turns out not to work correctly:  `gengotypes.py` spits out syntactically valid but unformatted Go code, and then runs `go fmt` on it to get the right style (including tabs, &c).  But the error code of `go fmt` isn’t checked; so on a system without go installed, if the build system decides it needs to re-run `gengotypes.py` for whatever reason, the build succeeds but the tree ends up “dirtied” with an unformatted version fo the generated text.

And `go fmt' overwrites its input file ?

The lost error is due to using `os.system' which does not raise an
exception.  The python 3 docs for `os.system' say
  | The subprocess module provides more powerful facilities for
  | spawning new processes and retrieving their results; using that
  | module is preferable to using this function. See the Replacing
  | Older Functions with the subprocess Module section in the
  | subprocess documentation for some helpful recipes.
And the recipe suggests
  | sts = os.system("mycmd" + " myarg")
  | # becomes
  | sts = call("mycmd" + " myarg", shell=True)
  | Notes:
  | * Calling the program through the shell is usually not required.
    
This is not particularly helpful advice because subprocess.call, like
os.system, does not raise an exception when things go wrong.  But it
does have a "more realistic example" immediately afterwards which does
actually check the error code.

You wanted subprocess.check_call.  IDK which Python versions have
subprocess.check_call.

Ian.

> On Jun 8, 2020, at 12:16 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("Re: [PATCH v2 1/5] libxl: Generate golang bindings in libxl Makefile"):
>> So as written this turns out not to work correctly:  `gengotypes.py` spits out syntactically valid but unformatted Go code, and then runs `go fmt` on it to get the right style (including tabs, &c).  But the error code of `go fmt` isn’t checked; so on a system without go installed, if the build system decides it needs to re-run `gengotypes.py` for whatever reason, the build succeeds but the tree ends up “dirtied” with an unformatted version fo the generated text.
> 
> And `go fmt' overwrites its input file ?

Yes.

> The lost error is due to using `os.system' which does not raise an
> exception.  The python 3 docs for `os.system' say
>  | The subprocess module provides more powerful facilities for
>  | spawning new processes and retrieving their results; using that
>  | module is preferable to using this function. See the Replacing
>  | Older Functions with the subprocess Module section in the
>  | subprocess documentation for some helpful recipes.
> And the recipe suggests
>  | sts = os.system("mycmd" + " myarg")
>  | # becomes
>  | sts = call("mycmd" + " myarg", shell=True)
>  | Notes:
>  | * Calling the program through the shell is usually not required.
> 
> This is not particularly helpful advice because subprocess.call, like
> os.system, does not raise an exception when things go wrong.  But it
> does have a "more realistic example" immediately afterwards which does
> actually check the error code.
> 
> You wanted subprocess.check_call.  IDK which Python versions have
> subprocess.check_call.

Since the golang code generation recipe is now called from libxl/Makefile unconditionally, the effect of “fixing” the `go fmt` call in gengotypes.py to fail if `go fmt` is not available will be to make golang a required dependency for building the tools.  I think it’s rather late in the day to be discussing that for 4.14.

Nick has already submitted a patch which simply removes the `go fmt` call, leaving the generated code looking very “generated”.  That will do for this release.

 -George