[24/24] golang/xenlight: add make target for generated files
diff mbox series

Message ID 26d6deae1803591361f7568645bc59b1535d6b88.1570456846.git.rosbrookn@ainfosec.com
State Superseded
Headers show
Series
  • generated Go libxl bindings using IDL
Related show

Commit Message

Nick Rosbrook Oct. 7, 2019, 3:13 p.m. UTC
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>
---
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(-)

Comments

George Dunlap Oct. 24, 2019, 2:26 p.m. UTC | #1
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
Nick Rosbrook Oct. 24, 2019, 6:49 p.m. UTC | #2
> 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
George Dunlap Nov. 14, 2019, 5:19 p.m. UTC | #3
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

Patch
diff mbox series

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: