[PATCH v2 0/5] Golang build fixes / improvements

[PATCH v2 0/5] Golang build fixes / improvements

[George Dunlap]

This is a series of patches that improve build for the golang xenlight
bindings.  The key patches are patch is #1 and #4.  Patch 1 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.  Patch 2 adds golang packages to the
docker images which have suitable golang versions, so that the bindings
can be tested in the CI loop.

Changes in v2:
- Document requirements to make sure the parallel build is race-free
- Replace v1 patches 4-5 with a patch which will just remove the
  GOPATH-related build testing
- Introduce improvements to automation

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

George Dunlap (5):
  libxl: Generate golang bindings in libxl Makefile
  golang/xenlight: Get rid of GOPATH-based build artefacts
  automation/archlinux: Add 32-bit glibc headers
  automation: Add golang packages to various dockerfiles
  automation/containerize: Add a shortcut for Debian unstable

 automation/build/archlinux/current.dockerfile |  2 ++
 automation/build/debian/unstable.dockerfile   |  1 +
 automation/build/fedora/29.dockerfile         |  1 +
 automation/scripts/containerize               |  2 +-
 tools/Rules.mk                                |  1 -
 tools/golang/Makefile                         | 10 --------
 tools/golang/xenlight/Makefile                | 24 +++++++++----------
 tools/libxl/Makefile                          | 17 ++++++++++++-
 8 files changed, 32 insertions(+), 26 deletions(-)

--
2.25.1

[PATCH v2 1/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>
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

[PATCH v2 2/5] golang/xenlight: Get rid of GOPATH-based build artefacts

[George Dunlap]

The original build setup used a "fake GOPATH" in tools/golang to test
the mechanism of building from go package files installed on a
filesystem.  With the move to modules, this isn't necessary, and leads
to potentially confusing directories being created.  (I.e., it might
not be obvious that files under tools/golang/src shouldn't be edited.)

Get rid of the code that creates this (now unused) intermediate
directory.  Add direct dependencies from 'build' onto the source
files.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- New

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Nick Rosbrook <rosbrookn@ainfosec.com>
---
 tools/Rules.mk                 |  1 -
 tools/golang/Makefile          | 10 ----------
 tools/golang/xenlight/Makefile | 19 ++++---------------
 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 59c72e7a88..76acaef988 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -35,7 +35,6 @@ XENSTORE_XENSTORED ?= y
 debug ?= y
 debug_symbols ?= $(debug)
 
-XEN_GOPATH        = $(XEN_ROOT)/tools/golang
 XEN_GOCODE_URL    = golang.xenproject.org
 
 ifeq ($(debug_symbols),y)
diff --git a/tools/golang/Makefile b/tools/golang/Makefile
index aba11ebc39..b022e2c5a3 100644
--- a/tools/golang/Makefile
+++ b/tools/golang/Makefile
@@ -1,16 +1,6 @@
 XEN_ROOT=$(CURDIR)/../..
 include $(XEN_ROOT)/tools/Rules.mk
 
-# In order to link against a package in Go, the package must live in a
-# directory tree in the way that Go expects.  To make this possible,
-# there must be a directory such that we can set GOPATH=${dir}, and
-# the package will be under $GOPATH/src/${full-package-path}.
-
-# So we set XEN_GOPATH to $XEN_ROOT/tools/golang.  The xenlight
-# "package build" directory ($PWD/xenlight) will create the "package
-# source" directory in the proper place.  Go programs can use this
-# package by setting GOPATH=$(XEN_GOPATH).
-
 SUBDIRS-y = xenlight
 
 .PHONY: build all
diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
index 8ab4cb5665..f8d8047524 100644
--- a/tools/golang/xenlight/Makefile
+++ b/tools/golang/xenlight/Makefile
@@ -14,17 +14,8 @@ LIBXL_SRC_DIR = ../../libxl
 .PHONY: all
 all: build
 
-.PHONY: package
-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)
-	$(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
@@ -43,23 +34,21 @@ idl-gen: $(GOXL_GEN_FILES)
 # in the LDFLAGS; and thus we need to add -L$(XEN_XENLIGHT) here
 # so that it can find the actual library.
 .PHONY: build
-build: package
+build: xenlight.go $(GOXL_GEN_FILES)
 	CGO_CFLAGS="$(CFLAGS_libxenlight) $(CFLAGS_libxentoollog)" CGO_LDFLAGS="$(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) -L$(XEN_XENLIGHT) -L$(XEN_LIBXENTOOLLOG)" $(GO) build -x
 
 .PHONY: install
 install: build
 	$(INSTALL_DIR) $(DESTDIR)$(GOXL_INSTALL_DIR)
-	$(INSTALL_DATA) $(XEN_GOPATH)$(GOXL_PKG_DIR)xenlight.go $(DESTDIR)$(GOXL_INSTALL_DIR)
-	$(INSTALL_DATA) $(XEN_GOPATH)$(GOXL_PKG_DIR)types.gen.go $(DESTDIR)$(GOXL_INSTALL_DIR)
-	$(INSTALL_DATA) $(XEN_GOPATH)$(GOXL_PKG_DIR)helpers.gen.go $(DESTDIR)$(GOXL_INSTALL_DIR)
+	$(INSTALL_DATA) xenlight.go $(DESTDIR)$(GOXL_INSTALL_DIR)
+	$(INSTALL_DATA) types.gen.go $(DESTDIR)$(GOXL_INSTALL_DIR)
+	$(INSTALL_DATA) helpers.gen.go $(DESTDIR)$(GOXL_INSTALL_DIR)
 
 .PHONY: uninstall
 	rm -rf $(DESTDIR)$(GOXL_INSTALL_DIR)
 
 .PHONY: clean
 clean:
-	$(RM) -r $(XEN_GOPATH)$(GOXL_PKG_DIR)
-	$(RM) $(XEN_GOPATH)/pkg/*/$(XEN_GOCODE_URL)/xenlight.a
 
 .PHONY: distclean
 distclean: clean
-- 
2.25.1

[PATCH v2 3/5] automation/archlinux: Add 32-bit glibc headers

[George Dunlap]

This fixes the following build error in hvmloader:

usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-32.h: No such file or directory

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- New

CC: Doug Goldstein <cardoe@cardoe.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 automation/build/archlinux/current.dockerfile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/automation/build/archlinux/current.dockerfile b/automation/build/archlinux/current.dockerfile
index 9af5d66afc..5095de65b8 100644
--- a/automation/build/archlinux/current.dockerfile
+++ b/automation/build/archlinux/current.dockerfile
@@ -19,6 +19,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm --noprogressbar --needed \
         iasl \
         inetutils \
         iproute \
+        lib32-glibc \
         libaio \
         libcacard \
         libgl \
-- 
2.25.1

[PATCH v2 4/5] automation: Add golang packages to various dockerfiles

[George Dunlap]

Specifically, Fedora 29, Archlinux, and Debian unstable.  This will
cause the CI loop to detect golang build failures.

CentOS 6 and 7 don't have golang packages, and the packages in
stretch, jessie, xenial, and trusty are too old.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- New

CC: Wei Liu <wl@xen.org>
CC: Doug Goldstein <cardoe@cardoe.com>
---
 automation/build/archlinux/current.dockerfile | 1 +
 automation/build/debian/unstable.dockerfile   | 1 +
 automation/build/fedora/29.dockerfile         | 1 +
 3 files changed, 3 insertions(+)

diff --git a/automation/build/archlinux/current.dockerfile b/automation/build/archlinux/current.dockerfile
index 5095de65b8..d8fbebaf79 100644
--- a/automation/build/archlinux/current.dockerfile
+++ b/automation/build/archlinux/current.dockerfile
@@ -16,6 +16,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm --noprogressbar --needed \
         ghostscript \
         git \
         gnutls \
+        go \
         iasl \
         inetutils \
         iproute \
diff --git a/automation/build/debian/unstable.dockerfile b/automation/build/debian/unstable.dockerfile
index d0aa5ad2bb..aeb4f3448b 100644
--- a/automation/build/debian/unstable.dockerfile
+++ b/automation/build/debian/unstable.dockerfile
@@ -45,6 +45,7 @@ RUN apt-get update && \
         nasm \
         gnupg \
         apt-transport-https \
+        golang \
         && \
         apt-get autoremove -y && \
         apt-get clean && \
diff --git a/automation/build/fedora/29.dockerfile b/automation/build/fedora/29.dockerfile
index 5be4a9e229..6a4e5b0413 100644
--- a/automation/build/fedora/29.dockerfile
+++ b/automation/build/fedora/29.dockerfile
@@ -40,5 +40,6 @@ RUN dnf -y install \
         nasm \
         ocaml \
         ocaml-findlib \
+        golang \
     && dnf clean all && \
     rm -rf /var/cache/dnf
-- 
2.25.1

[PATCH v2 5/5] automation/containerize: Add a shortcut for Debian unstable

[George Dunlap]

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- New

CC: Doug Goldstein <cardoe@cardoe.com>
CC: Wei Liu <wl@xen.org>
---
 automation/scripts/containerize | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/automation/scripts/containerize b/automation/scripts/containerize
index fbc4bc22d6..b71edd736c 100755
--- a/automation/scripts/containerize
+++ b/automation/scripts/containerize
@@ -22,6 +22,7 @@ case "_${CONTAINER}" in
     _fedora) CONTAINER="${BASE}/fedora:29";;
     _jessie) CONTAINER="${BASE}/debian:jessie" ;;
     _stretch|_) CONTAINER="${BASE}/debian:stretch" ;;
+    _unstable|_) CONTAINER="${BASE}/debian:unstable" ;;
     _trusty) CONTAINER="${BASE}/ubuntu:trusty" ;;
     _xenial) CONTAINER="${BASE}/ubuntu:xenial" ;;
 esac
@@ -91,4 +92,3 @@ exec docker run \
     -${termint}i --rm -- \
     ${CONTAINER} \
     ${cmd}
-
-- 
2.25.1

[PATCH v2 2/5] golang/xenlight: Get rid of GOPATH-based build artefacts

[Nick Rosbrook]

> diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
> index 8ab4cb5665..f8d8047524 100644
> --- a/tools/golang/xenlight/Makefile
> +++ b/tools/golang/xenlight/Makefile
> @@ -14,17 +14,8 @@ LIBXL_SRC_DIR = ../../libxl
>  .PHONY: all
>  all: build
>  
> -.PHONY: package
> -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)
> -	$(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)
> -

I think the GOXL_PKG_DIR variable can be removed all together too. With
these changes it's only used to initialize GOXL_INSTALL_DIR.

-NR

[PATCH v2 2/5] golang/xenlight: Get rid of GOPATH-based build artefacts

[Nick Rosbrook]

On Tue, May 26, 2020 at 11:16:09PM +0100, George Dunlap wrote:
> The original build setup used a "fake GOPATH" in tools/golang to test
> the mechanism of building from go package files installed on a
> filesystem.  With the move to modules, this isn't necessary, and leads
> to potentially confusing directories being created.  (I.e., it might
> not be obvious that files under tools/golang/src shouldn't be edited.)
> 
> Get rid of the code that creates this (now unused) intermediate
> directory.  Add direct dependencies from 'build' onto the source
> files.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

If you want to just make that change on check-in,

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

Re: [PATCH v2 3/5] automation/archlinux: Add 32-bit glibc headers

[Anthony PERARD]

On Tue, May 26, 2020 at 11:16:10PM +0100, George Dunlap wrote:
> This fixes the following build error in hvmloader:
> 
> usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-32.h: No such file or directory
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
>  automation/build/archlinux/current.dockerfile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/automation/build/archlinux/current.dockerfile b/automation/build/archlinux/current.dockerfile
> index 9af5d66afc..5095de65b8 100644
> --- a/automation/build/archlinux/current.dockerfile
> +++ b/automation/build/archlinux/current.dockerfile
> @@ -19,6 +19,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm --noprogressbar --needed \
>          iasl \
>          inetutils \
>          iproute \
> +        lib32-glibc \
>          libaio \
>          libcacard \
>          libgl \

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

Re: [PATCH v2 3/5] automation/archlinux: Add 32-bit glibc headers

[Wei Liu]

On Wed, May 27, 2020 at 11:43:16AM +0100, Anthony PERARD wrote:
> On Tue, May 26, 2020 at 11:16:10PM +0100, George Dunlap wrote:
> > This fixes the following build error in hvmloader:
> > 
> > usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-32.h: No such file or directory
> > 
> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> > ---
> >  automation/build/archlinux/current.dockerfile | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/automation/build/archlinux/current.dockerfile b/automation/build/archlinux/current.dockerfile
> > index 9af5d66afc..5095de65b8 100644
> > --- a/automation/build/archlinux/current.dockerfile
> > +++ b/automation/build/archlinux/current.dockerfile
> > @@ -19,6 +19,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm --noprogressbar --needed \
> >          iasl \
> >          inetutils \
> >          iproute \
> > +        lib32-glibc \
> >          libaio \
> >          libcacard \
> >          libgl \
> 
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

All automation patches:

Acked-by: Wei Liu <wl@xen.org>

Anthony, can you generate and push the new images? Thanks.

Wei.

> 
> -- 
> Anthony PERARD

Re: [PATCH v2 3/5] automation/archlinux: Add 32-bit glibc headers

[George Dunlap]

> On May 27, 2020, at 12:29 PM, Wei Liu <wl@xen.org> wrote:
> 
> On Wed, May 27, 2020 at 11:43:16AM +0100, Anthony PERARD wrote:
>> On Tue, May 26, 2020 at 11:16:10PM +0100, George Dunlap wrote:
>>> This fixes the following build error in hvmloader:
>>> 
>>> usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-32.h: No such file or directory
>>> 
>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>> ---
>>> automation/build/archlinux/current.dockerfile | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/automation/build/archlinux/current.dockerfile b/automation/build/archlinux/current.dockerfile
>>> index 9af5d66afc..5095de65b8 100644
>>> --- a/automation/build/archlinux/current.dockerfile
>>> +++ b/automation/build/archlinux/current.dockerfile
>>> @@ -19,6 +19,7 @@ RUN pacman -S --refresh --sysupgrade --noconfirm --noprogressbar --needed \
>>>         iasl \
>>>         inetutils \
>>>         iproute \
>>> +        lib32-glibc \
>>>         libaio \
>>>         libcacard \
>>>         libgl \
>> 
>> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> All automation patches:
> 
> Acked-by: Wei Liu <wl@xen.org>
> 
> Anthony, can you generate and push the new images? Thanks.

These are checked in now.

BTW it looks like there may be some other compilation issues updating the archlinux image.  I tested the minimum configuration required to get the golang bindings to build; but a full build fails with other errors I haven’t had time to diagnose yet.

 -George

Re: [PATCH v2 3/5] automation/archlinux: Add 32-bit glibc headers

[Anthony PERARD]

On Thu, May 28, 2020 at 12:32:02PM +0100, George Dunlap wrote:
> On May 27, 2020, at 12:29 PM, Wei Liu <wl@xen.org> wrote:
> > All automation patches:
> > 
> > Acked-by: Wei Liu <wl@xen.org>
> > 
> > Anthony, can you generate and push the new images? Thanks.
> 
> These are checked in now.
> 
> BTW it looks like there may be some other compilation issues updating the archlinux image.  I tested the minimum configuration required to get the golang bindings to build; but a full build fails with other errors I haven’t had time to diagnose yet.

The only issue seems to be that ipxe doesn't build. A simple -Wno-error
would make it works.

-- 
Anthony PERARD

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

[George Dunlap]

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

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

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

[Nick Rosbrook]

> 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

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

[George Dunlap]

> 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

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

[Ian Jackson]

George Dunlap writes ("Re: [PATCH v2 1/5] libxl: Generate golang bindings in libxl Makefile"):
> \x10So 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.

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

[George Dunlap]

> 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"):
>> \x10So 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

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

[Nick Rosbrook]

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