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

[Ronald Rojas <ronladred@gmail.com>]

On Wed, Nov 30, 2016 at 12:30:04PM +1100, George Dunlap wrote:
> 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.")

Alright, I can change the commit message. I'm still getting used to
how to write patches but I'll keep it in mind of next time. 

> 
> 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

Understood :). I will do this in the next iteration of the patch

> 
> >  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.

Sorry but I don't really I don't really understand how the python
Makefile works. In particular I don't really understand how the 
build rule and setup.py works( maybe because I haven't written 
any python). Would you mind giving me some intuition on how it 
works?

> 
> 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

Understood.

> 
> >  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?
I'm not sure what you mean here. Doesn't this install the go file xenlight.go, 
not a binary file? 
Or do you want me to create a package and install that instead package? 
Sorry, I'm a little lost here.
> 
> Thanks,
>  -George
Thanks, 
Ronald 

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