[PATCH 0/5] Golang build fixes

[PATCH 0/5] Golang build fixes

[George Dunlap]

This is a series of patches that improve build for the golang xenlight
bindings.  The most important patch is #3, which will update the
generated golang bindings from the tools/libxl directory when
libxl_types.idl is updated, even if the person building doesn't have
the golang packages enabled.

George Dunlap (5):
  golang: Add a minimum go version to go.mod
  golang: Add a variable for the libxl source directory
  libxl: Generate golang bindings in libxl Makefile
  golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding
  gitignore: Ignore golang package directory

 .gitignore                     |  1 +
 tools/golang/xenlight/Makefile | 12 +++++++++---
 tools/golang/xenlight/go.mod   |  2 ++
 tools/libxl/Makefile           | 12 +++++++++++-
 4 files changed, 23 insertions(+), 4 deletions(-)

--
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
CC: Wei Liu <wl@xen.org>

[PATCH 1/5] golang: Add a minimum go version to go.mod

[George Dunlap]

`go build` wants to add the current go version to go.mod as the
minimum every time we run `make` in the directory.  Add 1.11 (the
earliest Go version that supports modules) there to make it happy.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/go.mod | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/golang/xenlight/go.mod b/tools/golang/xenlight/go.mod
index 926474d929..7dfbd758d1 100644
--- a/tools/golang/xenlight/go.mod
+++ b/tools/golang/xenlight/go.mod
@@ -1 +1,3 @@
 module xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight
+
+go 1.11
-- 
2.25.1

[PATCH 2/5] golang: Add a variable for the libxl source directory

[George Dunlap]

...rather than duplicating the path in several places.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
index 37ed1358c4..cd0a62505f 100644
--- a/tools/golang/xenlight/Makefile
+++ b/tools/golang/xenlight/Makefile
@@ -9,6 +9,8 @@ GOXL_INSTALL_DIR = $(GOCODE_DIR)$(GOXL_PKG_DIR)
 
 GO ?= go
 
+LIBXL_SRC_DIR = ../../libxl
+
 .PHONY: all
 all: build
 
@@ -21,8 +23,8 @@ $(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: xenlight.go types.gen.go helpers.
 	$(INSTALL_DATA) types.gen.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
 	$(INSTALL_DATA) helpers.gen.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
 
-%.gen.go: gengotypes.py $(XEN_ROOT)/tools/libxl/libxl_types.idl $(XEN_ROOT)/tools/libxl/idl.py
-	XEN_ROOT=$(XEN_ROOT) $(PYTHON) gengotypes.py ../../libxl/libxl_types.idl
+%.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
 
 # Go will do its own dependency checking, and not actuall go through
 # with the build if none of the input files have changed.
-- 
2.25.1

[PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[George Dunlap]

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>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/Makefile |  6 +++++-
 tools/libxl/Makefile           | 12 +++++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
index cd0a62505f..751f916276 100644
--- a/tools/golang/xenlight/Makefile
+++ b/tools/golang/xenlight/Makefile
@@ -17,12 +17,16 @@ 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)
 
+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..2a06a7ebb8 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,16 @@ _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)
 
+.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

[PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/golang/xenlight/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
index 751f916276..6ab36c0aa9 100644
--- a/tools/golang/xenlight/Makefile
+++ b/tools/golang/xenlight/Makefile
@@ -19,7 +19,7 @@ package: $(XEN_GOPATH)$(GOXL_PKG_DIR)
 
 GOXL_GEN_FILES = types.gen.go helpers.gen.go
 
-$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: xenlight.go $(GOXL_GEN_FILES)
+$(XEN_GOPATH)$(GOXL_PKG_DIR): 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)
-- 
2.25.1

[PATCH 5/5] gitignore: Ignore golang package directory

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Konrad Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 7418ce9829..caaaa15b49 100644
--- a/.gitignore
+++ b/.gitignore
@@ -406,6 +406,7 @@ tools/python/xen/lowlevel/xl/_pyxl_types.h
 tools/xenstore/xenstore-watch
 tools/xl/_paths.h
 tools/xl/xl
+tools/golang/src
 
 docs/txt/misc/*.txt
 docs/txt/man/*.txt
-- 
2.25.1

Re: [PATCH 1/5] golang: Add a minimum go version to go.mod

[Nick Rosbrook]

> `go build` wants to add the current go version to go.mod as the
> minimum every time we run `make` in the directory.  Add 1.11 (the
> earliest Go version that supports modules) there to make it happy.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Re: [PATCH 2/5] golang: Add a variable for the libxl source directory

[Nick Rosbrook]

> ...rather than duplicating the path in several places.
>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Re: [PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[Nick Rosbrook]

> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Re: [PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[Nick Rosbrook]

> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Oh, I just noticed your commit message calls the variable
"XEN_PKG_DIR", but it's actually named "GOXL_PKG_DIR."

Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[Nick Rosbrook]

> 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>

For the golang side:

Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Re: [PATCH 5/5] gitignore: Ignore golang package directory

[Nick Rosbrook]

> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Re: [PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[George Dunlap]

> On May 23, 2020, at 5:48 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Oh, I just noticed your commit message calls the variable
> "XEN_PKG_DIR", but it's actually named "GOXL_PKG_DIR."

Oh, weird.  I presume the R-b stands if I fix the title?

 -George

Re: [PATCH 2/5] golang: Add a variable for the libxl source directory

[Ian Jackson]

George Dunlap writes ("[PATCH 2/5] golang: Add a variable for the libxl source directory"):
> ...rather than duplicating the path in several places.

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

Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[Ian Jackson]

George Dunlap writes ("[PATCH 3/5] libxl: Generate golang bindings in libxl Makefile"):
> +.PHONY: idl-external
> +idl-external:
> +	$(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen

Unfortunately this kind of thing is forbidden.  At least, without a
rigorous proof that this isn't a concurrency hazard.

The problem is that with parallel make, the concurrency correctness
principles are as follows:
(1) different targets use nonoverlapping temporary and output files
    (makefile authors' responsibiliy)
(2) one invocation of make won't make the same target twice at the
    same time (fundamental principle of operation for make)
(3) the same makefile (or different makefiles with overlapping
    targets) may not be entered multiple times in parallel
    (build system authors' responsibility; preclucdes most use of
    make -C to sibling directories rather than to children)

A correctness proof to make an exception would involve demonstrating
that the tools/golang directories never touch this file when invoked
as part of a recursive build.  NB, consider the clean targets too.

Alternatively, move the generated golang files to tools/libxl maybe,
and perhaps leave symlinks behind.

Or convert the whole (of tools/, maybe) to nonrecursive make using eg
subdirmk :-).  https://diziet.dreamwidth.org/5763.html

Sorry,
Ian.

[PATCH 5/5] gitignore: Ignore golang package directory

[Ian Jackson]

George Dunlap writes ("[PATCH 5/5] gitignore: Ignore golang package directory"):
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

I have to say that finding a directory src/ in gitignore is very
startling.

This directory src/ contains only output files ?

Is there not a risk that humans will try to edit it ?

Ian.

Re: [PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[Ian Jackson]

George Dunlap writes ("[PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding"):
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

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

Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[George Dunlap]

> On May 26, 2020, at 2:53 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 3/5] libxl: Generate golang bindings in libxl Makefile"):
>> +.PHONY: idl-external
>> +idl-external:
>> +	$(MAKE) -C $(XEN_ROOT)/tools/golang/xenlight idl-gen
> 
> Unfortunately this kind of thing is forbidden.  At least, without a
> rigorous proof that this isn't a concurrency hazard.
> 
> The problem is that with parallel make, the concurrency correctness
> principles are as follows:
> (1) different targets use nonoverlapping temporary and output files
>    (makefile authors' responsibiliy)
> (2) one invocation of make won't make the same target twice at the
>    same time (fundamental principle of operation for make)
> (3) the same makefile (or different makefiles with overlapping
>    targets) may not be entered multiple times in parallel
>    (build system authors' responsibility; preclucdes most use of
>    make -C to sibling directories rather than to children)
> 
> A correctness proof to make an exception would involve demonstrating
> that the tools/golang directories never touch this file when invoked
> as part of a recursive build.  NB, consider the clean targets too.

tools/golang/xenlight/Makefile:*.gen.go target will be triggered by xenlight/Makefile:idl-gen and xenlight/Makefile:build.

xenlight/Makefile:build is called from tools/golang/Makfile:subdirs-all, which is called from tools/Makefile:subdirs-all.

xenlight/Makefile:idl-gen is called from tools/libxl/Makefile:idl-external, which is called from tools/libxl/Makefile:all, which is called from tools/Makefile:subdirs-all.

tools/Makefile:subdirs-all is implemented as a non-parallel for loop executing over SUBDIRS-y; tools/golang comes after tools/libxl in that list, and so tools/golang:all will never be called until after tools/libxl:all has completed.  This invariant — that tools/golang/Makefile:all must not be called until tools/libxl/Makefile:all has completed must be kept regardless of this patch, since xenlight/Makefile:build depends on other output from tools/libxl/Makefile:all.

So as long as nothing else calls tools/libxl:all or tools/libxl:idl-external, this should be safe.  We could add a comments near xenlight/Makefile:idl-gen saying it must only be called from libxl/Makefile:idl-external; and to libxl/Makefile:idl-external saying it must not be called recursively from another Makefile.

> Alternatively, move the generated golang files to tools/libxl maybe,
> and perhaps leave symlinks behind.

Would that result in the files being accessible to the golang build tools at https://xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight ?  If not, it defeats the purpose of having the files checked into the tree.

> Or convert the whole (of tools/, maybe) to nonrecursive make using eg
> subdirmk :-).  https://diziet.dreamwidth.org/5763.html

This isn’t really a practical suggestion: I don’t have time to refactor the entire libxl Makefile tree, and certainly don’t have time by Friday.

 -George

Re: [PATCH 4/5] golang/xenlight: Use XEN_PKG_DIR variable rather than open-coding

[Nick Rosbrook]

On Tue, May 26, 2020 at 5:31 AM George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
> > On May 23, 2020, at 5:48 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> >
> >>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> >> Reviewed-by: Nick Rosbrook <rosbrookn@ainfosec.com>
> >
> > Oh, I just noticed your commit message calls the variable
> > "XEN_PKG_DIR", but it's actually named "GOXL_PKG_DIR."
>
> Oh, weird.  I presume the R-b stands if I fix the title?

Yes, of course.

-NR

Re: [PATCH 5/5] gitignore: Ignore golang package directory

[George Dunlap]

> On May 26, 2020, at 2:54 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("[PATCH 5/5] gitignore: Ignore golang package directory"):
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> I have to say that finding a directory src/ in gitignore is very
> startling.
> 
> This directory src/ contains only output files ?

With golang, you don’t really distribute package binaries; you only distribute source files.

However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files.

Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME.  We can set GOPATH but we can’t change the “src” component of that.  So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and 

With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH.

If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead.  (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.)

Nick, any opinions?

> Is there not a risk that humans will try to edit it ?

I suppose someone might.  If we decide we want to support older versions of go, we probably want to figure something out there.  Options:

1. Copy the files to a temp directory instead.  This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much).

2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”.

3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead.

 -George

[PATCH 5/5] gitignore: Ignore golang package directory

[Nick Rosbrook]

> With golang, you don’t really distribute package binaries; you only distribute source files.
> 
> However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files.
> 
> Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME.  We can set GOPATH but we can’t change the “src” component of that.  So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and 
> 
> With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH.
> 
> If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead.  (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.)
> 
> Nick, any opinions?

I can't think of a reason we need to support anything older than go
1.11, so I think it would be fine to get rid of remnants of the GOPATH
build.

-NR

Re: [PATCH 5/5] gitignore: Ignore golang package directory

[George Dunlap]

> On May 26, 2020, at 5:21 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
> 
>> With golang, you don’t really distribute package binaries; you only distribute source files.
>> 
>> However, we don’t want to wait until someone tries to clone the package to see if we’ve broken the build; so the current makefile does a “build test” of the package files.
>> 
>> Before golang’s “modules” feature, the only way to do this was to have the code to build inside $GOPATH/src/$PACKAGENAME.  We can set GOPATH but we can’t change the “src” component of that.  So we used to set GOPATH to $XENROOT/tools/golang, put the files in $XENROOT/tools/golang/src/$PACKAGENAME, and 
>> 
>> With the “modules” feature, code can be built anywhere; the build at the moment doesn’t require GOPATH.
>> 
>> If we’re willing to limit ourselves to using versions of golang which support modules by default (1.12+), then we can probably get rid of this bit instead.  (And if we do want to support older versions, we should really add some code in the configure script to determine whether to build with modules or GOPATH.)
>> 
>> Nick, any opinions?
> 
> I can't think of a reason we need to support anything older than go
> 1.11, so I think it would be fine to get rid of remnants of the GOPATH
> build.

OK, I’ll send a patch to remove the “fake GOPATH build” support.

 -George

Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 3/5] libxl: Generate golang bindings in libxl Makefile"):
> tools/Makefile:subdirs-all is implemented as a non-parallel for loop executing over SUBDIRS-y; tools/golang comes after tools/libxl in that list, and so tools/golang:all will never be called until after tools/libxl:all has completed.  This invariant — that tools/golang/Makefile:all must not be called until tools/libxl/Makefile:all has completed must be kept regardless of this patch, since xenlight/Makefile:build depends on other output from tools/libxl/Makefile:all.

I had not spotted this aspect of the situation.  But the toplevel
Makefile is parallel.  I think this means that make -C works between
different directories in tools/.

Provided no-one says `make all install' (which is a thing that people
expect to work but which is already badly broken).

> So as long as nothing else calls tools/libxl:all or tools/libxl:idl-external, this should be safe.  We could add a comments near xenlight/Makefile:idl-gen saying it must only be called from libxl/Makefile:idl-external; and to libxl/Makefile:idl-external saying it must not be called recursively from another Makefile.

So, err, I'm sold on the original patch, I think.

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

I'll answer your other comments anyway:

> > Alternatively, move the generated golang files to tools/libxl maybe,
> > and perhaps leave symlinks behind.
> 
> Would that result in the files being accessible to the golang build tools at https://xenbits.xenproject.org/git-http/xen.git/tools/golang/xenlight ?  If not, it defeats the purpose of having the files checked into the tree.

Yes.  git can convey symlinks.  (I'm assuming that the golang build
tools fetch the git objects and do git checkout, rather than
trying to download individual raw files from gitweb...)

> > Or convert the whole (of tools/, maybe) to nonrecursive make using eg
> > subdirmk :-).  https://diziet.dreamwidth.org/5763.html
> 
> This isn’t really a practical suggestion: I don’t have time to refactor the entire libxl Makefile tree, and certainly don’t have time by Friday.

Yes, it wasn't a serious suggestion.  Sorry that that apparently
wasn't clear.

Regards,
Ian.

Re: [PATCH 5/5] gitignore: Ignore golang package directory

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 5/5] gitignore: Ignore golang package directory"):
> [explanation]

Sounds quite tangled...

> Nick, any opinions?
...
> > Is there not a risk that humans will try to edit it ?

Anyway ISTM that you have definitely considered this so

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

assuming that and Nick convince yourselves you've addressed this
possible issue.

> I suppose someone might.  If we decide we want to support older versions of go, we probably want to figure something out there.  Options:
> 
> 1. Copy the files to a temp directory instead.  This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much).

I don't think that helps much.

> 2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”.
> 
> 3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead.

Do they not have a header comment saying DO NOT EDIT ?

3 is pretty ugly.  I'll leave it up to you whether to bother with 2.

Thanks,
Ian.

Re: [PATCH 5/5] gitignore: Ignore golang package directory

[George Dunlap]

> On May 26, 2020, at 5:59 PM, Ian Jackson <ian.jackson@citrix.com> wrote:
> 
> George Dunlap writes ("Re: [PATCH 5/5] gitignore: Ignore golang package directory"):
>> [explanation]
> 
> Sounds quite tangled...
> 
>> Nick, any opinions?
> ...
>>> Is there not a risk that humans will try to edit it ?
> 
> Anyway ISTM that you have definitely considered this so
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> assuming that and Nick convince yourselves you've addressed this
> possible issue.
> 
>> I suppose someone might.  If we decide we want to support older versions of go, we probably want to figure something out there.  Options:
>> 
>> 1. Copy the files to a temp directory instead.  This is complicated because we have to find a good temp directory, and we’d have to copy them every time, slowing down the incremental build (though not that much).
> 
> I don't think that helps much.
> 
>> 2. Put a file in the generated directory like “GENERATED_DO_NOT_EDIT”.
>> 
>> 3. Put them in tools/golang/GENERATED_DO_NOT_EDIT/src instead.
> 
> Do they not have a header comment saying DO NOT EDIT ?

The generated files do, but this copies all the files, including the non-generated ones.

Anyway, it turns out is has nothing to do with go modules per se, but more to do with my quixotic attempt to make it possible to build from stuff installed locally in $PREFIX, rather than having to clone something over the internet.  The current version of the “build test” doesn’t actually use this GOPATH stuff, and works even on versions of golang that don’t have module support.

I’ve got a patch that removes this whole fake-GOPATH thing; I’ll send that along in lieu of patches 4 and 5.

Thanks,
 -George