On 10/7/19 4:13 PM, Nick Rosbrook wrote: > From: Nick Rosbrook <rosbro...@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 <rosbro...@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.dun...@citrix.com> > Cc: Ian Jackson <ian.jack...@eu.citrix.com> > Cc: Wei Liu <w...@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