Re: [PATCH RFC] tools/xenlight: Create xenlight Makefile

[George Dunlap <dunlapg@umich.edu>]

On Tue, Nov 29, 2016 at 4:18 AM, Ronald Rojas <ronladred@gmail.com> wrote:
> Created basic Makfele for the Golang xenlight
> project so that the project is built and
> installed. A template template is alo added.

Thanks Ronald!  Not a bad first submission, but a lot of things that
need tightening up.

First, the normal style of most commit messages is more direct;
usually in the form "Do X" (like a recipe), or in the form "We do Y".

So to translate the comment above, I'd probably say something like this:

"Create a basic Makefile to build and install libxenlight Golang
bindings.  Also add a template."  (Or, as an example of the second
form, "We also add a template so that we have something to build.")

But in the final patch, we'll probably be introducing all the libxl
golang bindings in one go (not first the makefile, then the bindings,
&c), so it probably makes sense to have your mock-up start with that
assumption, then explain that it's still a work in progress.

The best way to do this is to put comments for the reviewers under a
line with three dashes ("---") in the commit message.

So something like this:

8<--------

tools: Introduce xenlight golang bindings

Create tools/golang/xenlight and hook it into the main tools Makefile.

Signed-off-by: John Smith <jsmith@google.com>

---

Eventually this patch will contain the actual bindings package; for
now just include a stub package.

To do:
- Make a real package
- Have configure detect golang bindings properly

-------------->8

>  tools/Makefile                    | 15 +++++++++++++++
>  tools/golang/xenlight/Makefile    | 29 +++++++++++++++++++++++++++++
>  tools/golang/xenlight/xenlight.go |  7 +++++++
>  3 files changed, 51 insertions(+)
>  create mode 100644 tools/golang/xenlight/Makefile
>  create mode 100644 tools/golang/xenlight/xenlight.go
>
> diff --git a/tools/Makefile b/tools/Makefile
> index 71515b4..b89f06b 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -11,6 +11,7 @@ SUBDIRS-y += misc
>  SUBDIRS-y += examples
>  SUBDIRS-y += hotplug
>  SUBDIRS-y += xentrace
> +SUBDIRS-y += golang/xenlight

As Wei said, you should follow the python model, and put another
Makefile in tools/golang.

And you should add a comment here saying that you plan to use
autotools to do this eventually:

# FIXME: Have this controlled by a configure variable

>  SUBDIRS-$(CONFIG_XCUTILS) += xcutils
>  SUBDIRS-$(CONFIG_X86) += firmware
>  SUBDIRS-y += console
> @@ -311,6 +312,20 @@ subdir-install-debugger/gdbsx: .phony
>  subdir-all-debugger/gdbsx: .phony
>         $(MAKE) -C debugger/gdbsx all
>
> +subdir-all-golang/xenlight: .phony
> +       $(MAKE) -C golang/xenlight all
> +
> +subdir-clean-golang/xenlight: .phony
> +       $(MAKE) -C golang/xenlight clean
> +
> +subdir-install-golang/xenlight: .phony
> +       $(MAKE) -C golang/xenlight install
> +
> +subdir-build-golang/xenlight: .phony
> +       $(MAKE) -C golang/xenlight build
> +
> +subdir-distclean-golang/xenlight: .phony
> +       $(MAKE) -C golang/xenlight distclean
>
>  subdir-clean-debugger/kdd subdir-distclean-debugger/kdd: .phony
>         $(MAKE) -C debugger/kdd clean
> diff --git a/tools/golang/xenlight/Makefile b/tools/golang/xenlight/Makefile
> new file mode 100644
> index 0000000..c723c1d
> --- /dev/null
> +++ b/tools/golang/xenlight/Makefile
> @@ -0,0 +1,29 @@
> +XEN_ROOT=$(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +BINARY = xenlightgo
> +GO = go
> +
> +.PHONY: all
> +all: build
> +
> +.PHONY: build
> +build: xenlight
> +
> +.PHONY: install
> +install: build
> +       [ -f $(BINARY) ] || $(INSTALL_PROG) xenlight.go $(DESTDIR)$(bindir)

Is there a reason you decided to make this a binary (and to install it
as a binary), rather than making a stub package?

Thanks,
 -George

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