Re: [Xen-devel] [PATCH 24/24] golang/xenlight: add make target for generated files

[George Dunlap <george.dunlap@citrix.com>]

On 10/7/19 4:13 PM, Nick Rosbrook wrote:
> From: Nick Rosbrook <rosbrookn@ainfosec.com>
> 
> Remove the PKGSOURCES variable since adding xenlight_types.go
> and xenlight_helpers.go to this list breaks the rest of the
> Makefile.
> 
> Add xenlight_%.go target for generated files, and use full
> file names within install, uninstall and $(XEN_GOPATH)$(GOXL_PKG_DIR)
> rule.
> 
> Signed-off-by: Nick Rosbrook <rosbrookn@ainfosec.com>

Hey Nick!  Thanks for breaking down the series this way -- this is much
easier to review.

One standard practice when making a series is to try to avoid any
regressions, including build regressions, in the middle of the series.
This is particularly helpful to aid in bisections, but in this case it
makes it easier to observe the action of the `gengotypes.py` script (and
how it's meant to be called).

So I would basically make this part of patch 2, except remove references
to xenlight_helpers.go until the patch where that file is generated.

One other comment...

> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> 
>  tools/golang/xenlight/Makefile | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
> index 0987305224..821a5d48fa 100644
> --- a/tools/golang/xenlight/Makefile
> +++ b/tools/golang/xenlight/Makefile
> @@ -7,20 +7,22 @@ GOCODE_DIR ?= $(prefix)/share/gocode/
>  GOXL_PKG_DIR = /src/$(XEN_GOCODE_URL)/xenlight/
>  GOXL_INSTALL_DIR = $(GOCODE_DIR)$(GOXL_PKG_DIR)
>  
> -# PKGSOURCES: Files which comprise the distributed source package
> -PKGSOURCES = xenlight.go
> -
>  GO ?= go
>  
>  .PHONY: all
>  all: build
>  
>  .PHONY: package
> -package: $(XEN_GOPATH)$(GOXL_PKG_DIR)$(PKGSOURCES)
> +package: $(XEN_GOPATH)$(GOXL_PKG_DIR)
>  
> -$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/$(PKGSOURCES): $(PKGSOURCES)
> +$(XEN_GOPATH)/src/$(XEN_GOCODE_URL)/xenlight/: xenlight_%.go
>  	$(INSTALL_DIR) $(XEN_GOPATH)$(GOXL_PKG_DIR)
> -	$(INSTALL_DATA) $(PKGSOURCES) $(XEN_GOPATH)$(GOXL_PKG_DIR)
> +	$(INSTALL_DATA) xenlight.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
> +	$(INSTALL_DATA) xenlight_types.go $(XEN_GOPATH)$(GOXL_PKG_DIR)
> +	$(INSTALL_DATA) xenlight_helpers.go $(XEN_GOPATH)$(GOXL_PKG_DIR)

It might be nice to have a naming convention for the generated files
that clues people in to the fact that they're generated (other than the
comment at the top of course).  In libxl, this is done by giving them a
leading underscore (e.g., _libxl_type.h); but the go compiler will
helpfully ignore such files. :-)

The go compiler will also do special things sometimes with things after
a `_`; e.g., "${foo}_test.go" will only be compiled for `go test`,
"${foo}_linux.go" will only be compiled on Linux, and so on.  I'm pretty
sure these names will be safe, but it might be slightly more
future-proof to avoid using an underscore in the names.

What about something like "gentypes.go" or "idltypes.go"?

Just a suggestion.

> +
> +xenlight_%.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
>  
>  # Go will do its own dependency checking, and not actuall go through
>  # with the build if none of the input files have changed.
> @@ -36,10 +38,14 @@ build: package
>  .PHONY: install
>  install: build
>  	$(INSTALL_DIR) $(DESTDIR)$(GOXL_INSTALL_DIR)
> -	$(INSTALL_DATA) $(XEN_GOPATH)$(GOXL_PKG_DIR)$(PKGSOURCES) $(DESTDIR)$(GOXL_INSTALL_DIR)
> +	$(install_data) $(xen_gopath)$(goxl_pkg_dir)xenlight.go $(destdir)$(goxl_install_dir)
> +	$(install_data) $(xen_gopath)$(goxl_pkg_dir)xenlight_types.go $(destdir)$(goxl_install_dir)
> +	$(install_data) $(xen_gopath)$(goxl_pkg_dir)xenlight_helpers.go $(destdir)$(goxl_install_dir)
>  
>  .PHONY: uninstall
> -	rm -f $(addprefix $(DESTDIR)$(GOXL_INSTALL_DIR)/, $(PKGSOURCES))
> +	rm -f $(addprefix $(DESTDIR)$(GOXL_INSTALL_DIR)/, xenlight.go)
> +	rm -f $(addprefix $(DESTDIR)$(GOXL_INSTALL_DIR)/, xenlight_types.go)
> +	rm -f $(addprefix $(DESTDIR)$(GOXL_INSTALL_DIR)/, xenlight_helpers.go)

Kind of random, but would it make sense to `rm -rf` the whole directory
here instead?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel