| Message ID | 26d6deae1803591361f7568645bc59b1535d6b88.1570456846.git.rosbrookn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | generated Go libxl bindings using IDL | expand |
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
> 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. Ah yeah that makes sense, I'll correct this in v2. > 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. +1 for a naming convention that says "this file is generated." But, the only special cases that I'm aware of for go file name suffixes are "test", and valid GOOS and GOARCH values. It's conventional to use underscores for compounded file names, and unnecessary to avoid them. To reference gRPC again, their protobuf compiler writes file names like 'package.pb.go', where pb is short for protobuf. So, I think something like '<name>_generated.go', or '<name>.idl.go' could work. > Kind of random, but would it make sense to `rm -rf` the whole directory > here instead? Yeah probably :) -NR
On 10/24/19 7:49 PM, Nick Rosbrook wrote: >> 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. > > Ah yeah that makes sense, I'll correct this in v2. > >> 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. > > +1 for a naming convention that says "this file is generated." But, > the only special > cases that I'm aware of for go file name suffixes are "test", and > valid GOOS and GOARCH > values. It's conventional to use underscores for compounded file > names, and unnecessary > to avoid them. > > To reference gRPC again, their protobuf compiler writes file names > like 'package.pb.go', where > pb is short for protobuf. So, I think something like > '<name>_generated.go', or '<name>.idl.go' > could work. Both of those are OK. I might go with "gen_*.go" to be a bit shorter, or ".idlgen.go" to make it clear that this is /generated from/ and idl, and not an idl itself. But I don't have strong opinions; any of those four options would be fine with me. -George
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) + +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) .PHONY: clean clean: