| Message ID | 1480353483-12643-2-git-send-email-ronladred@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 28, 2016 at 12:18:03PM -0500, Ronald Rojas wrote: > Created basic Makfele for the Golang xenlight Makefile. > project so that the project is built and > installed. A template template is alo added. Duplicated "template". > --- > 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 This shouldn't be built unconditionally. Please use configure to probe for go compiler first. "GNU autotools" is the term to google for. But I guess it wouldn't be too hard to follow existing examples. Don't hesitate to ask questions. > 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 > I think I would prefer to organise this like python bindings. That is, to have a dedicated Makefile under $LANG (python or golang) directory. What do you think? > 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 This should probably be: GO ?= go > + > +.PHONY: all > +all: build > + > +.PHONY: build > +build: xenlight > + > +.PHONY: install > +install: build > + [ -f $(BINARY) ] || $(INSTALL_PROG) xenlight.go $(DESTDIR)$(bindir) > + > +.PHONY: clean > +clean: > + $(RM) $(BINARY) If there are other intermediate files generated, they should be deleted, too. > + > +.PHONY: distclean > +distclean: clean > + > + > +xenlight: xenlight.go xenlightgo: xenlight.go and delete BINARY or $(BINARY): xenlight.go > + $(GO) build -o $(BINARY) xenlight.go Use $@ instead of $(BINARY). Use $< instead of xenlight.go. But before we spend too much time on details, let's sort out some higher level issues first. My golang knowledge is rather rusted, please bear with me. :-) I have a question: how does golang build a package that has multiple files? Presumably it has some sort of file that acts like Makefile. If so, you should probably introduce that now and use that to build this package. Otherwise in order to avoid repetition you probably need something like the %.o: %.c rule in tools/Rules.mk. If you need more information about GNU Make, please use `info make` on your system or ask questions here. > + > + > +-include $(DEPS) > diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go > new file mode 100644 > index 0000000..50e8d8d > --- /dev/null > +++ b/tools/golang/xenlight/xenlight.go > @@ -0,0 +1,7 @@ > +package main > + > +import "fmt" > + > +func main() { > + fmt.Println("vim-go") vim-go ? > +} > -- > 2.7.4 >
On Tue, Nov 29, 2016 at 6:19 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Mon, Nov 28, 2016 at 12:18:03PM -0500, Ronald Rojas wrote: >> 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 > > This shouldn't be built unconditionally. > > Please use configure to probe for go compiler first. > > "GNU autotools" is the term to google for. But I guess it wouldn't be > too hard to follow existing examples. > > Don't hesitate to ask questions. BTW I suggested that Ronald start with enabling it unconditionally, so that he could get something straightforward related to his project (golang bindings) accomplished before having to dive right into something complicated and unrelated (autoconf). But it's probably worth communicating that here with a comment. I'll respond directly to his mail. >> 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 >> > > I think I would prefer to organise this like python bindings. That is, > to have a dedicated Makefile under $LANG (python or golang) directory. > > What do you think? That's what I had in mind as well. > >> 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 > > This should probably be: > > GO ?= go With an additional comment above it: # FIXME Use autoconf to find this? >> + >> +.PHONY: all >> +all: build >> + >> +.PHONY: build >> +build: xenlight >> + >> +.PHONY: install >> +install: build >> + [ -f $(BINARY) ] || $(INSTALL_PROG) xenlight.go $(DESTDIR)$(bindir) >> + >> +.PHONY: clean >> +clean: >> + $(RM) $(BINARY) > > If there are other intermediate files generated, they should be deleted, > too. At the moment there won't be any intermediate files -- "go build" creates a temporary directory for those and deletes them after it's done automatically. >> +.PHONY: distclean >> +distclean: clean >> + >> + >> +xenlight: xenlight.go > > xenlightgo: xenlight.go and delete BINARY > > or > > $(BINARY): xenlight.go > >> + $(GO) build -o $(BINARY) xenlight.go > > Use $@ instead of $(BINARY). Use $< instead of xenlight.go. > > But before we spend too much time on details, let's sort out some higher > level issues first. My golang knowledge is rather rusted, please bear > with me. :-) > > I have a question: how does golang build a package that has multiple > files? Presumably it has some sort of file that acts like Makefile. If > so, you should probably introduce that now and use that to build this > package. Not really -- you either hand it all the files it needs (like you would a C compiler), or you hand it some of the files it needs and also give it a directory to the source code for your packages, or you do things entirely its way and just type "go build" and it figures out what to do. If you want Makefile-like functionality you probably want make. :-) In our case we'll probably end up doing the first for building the xenlight package, and the second for building the test tool. But there's no point in getting into the nitty-gritty now because what Ronald has here isn't a package (meant to be linked against), but a program (meant to be run). So we'll have to wait until we have basic package stuff to figure out the best way to link it. -George
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
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
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 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) + +.PHONY: clean +clean: + $(RM) $(BINARY) + +.PHONY: distclean +distclean: clean + + +xenlight: xenlight.go + $(GO) build -o $(BINARY) xenlight.go + + +-include $(DEPS) diff --git a/tools/golang/xenlight/xenlight.go b/tools/golang/xenlight/xenlight.go new file mode 100644 index 0000000..50e8d8d --- /dev/null +++ b/tools/golang/xenlight/xenlight.go @@ -0,0 +1,7 @@ +package main + +import "fmt" + +func main() { + fmt.Println("vim-go") +}