Merge lp:~allenap/juju-core/makefile-stuff into lp:~go-bot/juju-core/trunk

[Gavin Panella]

Improve build tooling.

Some build targets - build, check, install and clean - can only run
when the tree is in the right place on GOPATH. The Makefile now guards
this and provides useful errors.

The system package dependencies are now more readably listed.

The ~juju PPAs are not added on Saucy; they do not work there.

A new clean target removes test executables and other detritus. *.test
files are also now ignored.

Improve build tooling

Some build targets - build, check, install and clean - can only run
when the tree is in the right place on GOPATH. The Makefile now guards
this and provides useful errors.

The system package dependencies are now more readably listed.

The ~juju PPAs are not added on Saucy; they do not work there.

A new clean target removes test executables and other detritus. *.test
files are also now ignored.

https://codereview.appspot.com/12949047/

[Gavin Panella (allenap)]

Reviewers: mp+181113_code.launchpad.net,

Message:
Please take a look.

Description:
Improve build tooling

Some build targets - build, check and install - can only run when the
tree is in the right place on GOPATH. The Makefile now guards this and
provides useful errors.

The system package dependencies are now more readably listed.

The ~juju PPAs are not added on Saucy; they do not work there.

A new clean target removes test executables. These are also ignored.

Editor artifacts - tag files, .emacs.desktop* - are no longer
ignored. These belong in a developer's local ~/.bazaar/ignore file,
not in a project's.

https://code.launchpad.net/~allenap/juju-core/makefile-stuff/+merge/181113

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12949047/

Affected files:
   M .bzrignore
   M Makefile
   A [revision details]

Index: .bzrignore
=== modified file '.bzrignore'
--- .bzrignore 2013-02-27 14:48:01 +0000
+++ .bzrignore 2013-08-20 17:44:53 +0000
@@ -3,7 +3,4 @@
  cmd/builddb/builddb
  cmd/charmd/charmd
  cmd/charmload/charmload
-./tags
-./TAGS
-.emacs.desktop
-.emacs.desktop.lock
+*.test

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2013-08-13 10:58:22 +0000
+++ Makefile 2013-08-20 17:40:17 +0000
@@ -1,40 +1,80 @@
+#
  # Makefile for juju-core.
-PROJECT=launchpad.net/juju-core
-
-# Default target. Compile, just to see if it will.
+#
+
+ifndef GOPATH
+$(warning You need to set up a GOPATH. See the README file.)
+endif
+
+PROJECT := launchpad.net/juju-core
+PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PACKAGE))
+
+define DEPENDENCIES
+ build-essential
+ bzr
+ distro-info-data
+ git-core
+ golang
+ mercurial
+ mongodb-server
+ zip
+endef
+
+default: build
+
+# Start of GOPATH-dependent targets. Some targets only make sense -
+# and will only work - when this tree is found on the GOPATH.
+ifeq ($(CURDIR),$(PROJECT_DIR))
+
  build:
   go build $(PROJECT)/...

-# Run tests.
  check:
   go test $(PROJECT)/...

+install:
+ go install -v $(PROJECT)/...
+
+else # --------------------------------
+
+build:
+ $(error Cannot build package outside of GOPATH)
+
+check:
+ $(error Cannot check package outside of GOPATH)
+
+install:
+ $(error Cannot install package from outside of GOPATH)
+
+endif
+# End of GOPATH-dependent targets.
+
  # Reformat the source files.
  format:
- go fmt $(PROJECT)/...
-
-# Install juju into $GOPATH/bin.
-install:
- go install -v $(PROJECT)/...
-
-# Install packages required to develop Juju and run tests.
+ gofmt -w -l .
+
+# Invoke gofmt's "simplify" option to streamline the source code.
+simplify:
+ gofmt -w -l -s .
+
+# Clean the tree, including removing test executables.
+clean:
+ find . -name '*.test' -print0 | xargs -r0 $(RM) -v
+
+# Install packages required to develop Juju and run tests. The stable
+# PPA includes the required mongodb-server binaries. However, neither
+# PPA works on Saucy just yet.
  install-dependencies:
+ifneq ($(shell lsb_release -cs),saucy)
   @echo Adding juju PPAs for golang and mongodb-server
   @sudo apt-add-repository ppa:juju/golang
- # The stable PPA includes the required mongodb-server binaries.
   @sudo apt-ad...

[Gavin Panella (allenap)]

Please take a look.

https://codereview.appspot.com/12949047/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-08-20 21:52, Gavin Panella wrote:
> Gavin Panella has proposed merging
> lp:~allenap/juju-core/makefile-stuff into lp:juju-core.
>
> Requested reviews: juju hackers (juju)
>
> For more details, see:
> https://code.launchpad.net/~allenap/juju-core/makefile-stuff/+merge/181113
>
> Improve build tooling
>
> Some build targets - build, check and install - can only run when
> the tree is in the right place on GOPATH. The Makefile now guards
> this and provides useful errors.
>
> The system package dependencies are now more readably listed.
>
> The ~juju PPAs are not added on Saucy; they do not work there.
>
> A new clean target removes test executables. These are also
> ignored.
>

> Editor artifacts - tag files, .emacs.desktop* - are no longer
> ignored. These belong in a developer's local ~/.bazaar/ignore
> file, not in a project's.
>

While I sort of agree, I don't find it harmful and it means things
"just work" for people who haven't finished configuring them. Why do
you feel they must be removed? (IOW, while I slightly agree the net
benefit of having them in there outweighs the tiny downside of having
a slightly larger ignore file, but maybe I'm missing something.)

The rest is fine to me though:
+clean:
+ find . -name '*.test' -print0 | xargs -r0 $(RM) -v

Should probably run 'go clean' somehow. Or at least delete 'go build'
output for various cmd programs.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIUmRAACgkQJdeBCYSNAAMriQCgvdO5ZnYzHHRzq5uCw+nVl2JB
dYAAoMhNEUCJ+wbLgiCEXAusGw3sj1yv
=D3av
-----END PGP SIGNATURE-----

[Gavin Panella (allenap)]

> While I sort of agree, I don't find it harmful and it means things
> "just work" for people who haven't finished configuring them. Why do
> you feel they must be removed? (IOW, while I slightly agree the net
> benefit of having them in there outweighs the tiny downside of having
> a slightly larger ignore file, but maybe I'm missing something.)

This was a misguided attempt to make `bzr clean-tree --ignored` work
consistently for everyone... but the patterns in ~/.bazaar/ignore are
merged into the --ignored set, so my plan backfired, which was kind of
obvious in retrospect. I've resurrected those patterns.

>
> The rest is fine to me though:
> +clean:
> +       find . -name '*.test' -print0 | xargs -r0 $(RM) -v
>
>
> Should probably run 'go clean' somehow. Or at least delete 'go build'
> output for various cmd programs.

I didn't know of `go clean` before, and I've switched to it now.

Thanks John!

[Gavin Panella (allenap)]

Please take a look.

https://codereview.appspot.com/12949047/

[Roger Peppe (rogpeppe)]

LGTM but please wait for someone that's more familiar with GNU make
syntax.

https://codereview.appspot.com/12949047/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode10
Makefile:10: PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PACKAGE))
Nice trick with -e there.

https://codereview.appspot.com/12949047/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-08-27 19:38, Gavin Panella wrote:
>> While I sort of agree, I don't find it harmful and it means
>> things "just work" for people who haven't finished configuring
>> them. Why do you feel they must be removed? (IOW, while I
>> slightly agree the net benefit of having them in there outweighs
>> the tiny downside of having a slightly larger ignore file, but
>> maybe I'm missing something.)
>
> This was a misguided attempt to make `bzr clean-tree --ignored`
> work consistently for everyone... but the patterns in
> ~/.bazaar/ignore are merged into the --ignored set, so my plan
> backfired, which was kind of obvious in retrospect. I've
> resurrected those patterns.

You could use "bzr clean-tree --detritus" ? It is the one I use most
often (to clean out things that bzr itself puts into the tree). For
good or bad bzr never did get the idea of "this is ignored because it
is garbage" separate "this is ignored because it is my precious
secrets that cannot be shared".

>
>>
>> The rest is fine to me though: +clean: + find . -name
>> '*.test' -print0 | xargs -r0 $(RM) -v
>>
>>
>> Should probably run 'go clean' somehow. Or at least delete 'go
>> build' output for various cmd programs.
>
> I didn't know of `go clean` before, and I've switched to it now.
>
> Thanks John!
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIdlcAACgkQJdeBCYSNAAOAsgCeLtPdJC2dobi2t6zQC44huIiy
OgEAoKUtB5YwARSdL68/2wCbRr9gWDIF
=gdkE
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

Some comments, but overall LGTM

https://codereview.appspot.com/12949047/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode10
Makefile:10: PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PACKAGE))
You are using "PROJECT_DIR := .... $(PACKAGE)"

but I don't see where PACKAGE is getting set. Did you mean to use
PROJECT there?

eg:
PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PROJECT))

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode44
Makefile:44: $(error Cannot build package outside of GOPATH)
This is really more of "you must be at $PROJECT_DIR to run build".
Which isn't strictly true from my experience.
You can run "go build launchpad.net/juju-core/..." and it will build
everything under there just fine. (I often use syntax similar to that
when I'm in a subdir and want to build something in a sibling dir.)

But given that Makefile is at the root, I don't think people will really
do:
  make -f ../../../Makefile

anyway, so I don't think it is a big deal. So if it helps you to have
the check for CWD you can leave it in.

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode64
Makefile:64: gofmt -w -l -s .
Given that "time make format" and "time make simplify" doen't seem to
differ, we probably just want to always default to simplify.

So:

format: simplify

simplify:
   gofmt -w -l -s .

Though really *I* prefer

go fmt $(PROJECT)/...

Because that makes sure I get the files in another directory. (If I'm in
the subdir cmd/juju I'm probably also hacking on code in environs/tools
or whatever)

Then again, Makefile isn't available easily in subdirs, so it doesn't
matter *for me*.

https://codereview.appspot.com/12949047/

[Gavin Panella (allenap)]

Thanks for the reviews, Roger & John.

https://codereview.appspot.com/12949047/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode10
Makefile:10: PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PACKAGE))
On 2013/08/28 06:23:17, jameinel wrote:
> You are using "PROJECT_DIR := .... $(PACKAGE)"

> but I don't see where PACKAGE is getting set. Did you mean to use
PROJECT there?

Oh, embarrassing. `go list` does the right thing when in the project
directory on GOPATH so I didn't spot this. Fixed, thanks for spotting
that.

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode44
Makefile:44: $(error Cannot build package outside of GOPATH)
On 2013/08/28 06:23:17, jameinel wrote:
> This is really more of "you must be at $PROJECT_DIR to run build".
> Which isn't strictly true from my experience.
> You can run "go build launchpad.net/juju-core/..." and it will build
everything
> under there just fine. (I often use syntax similar to that when I'm in
a subdir
> and want to build something in a sibling dir.)

> But given that Makefile is at the root, I don't think people will
really do:
> make -f ../../../Makefile

> anyway, so I don't think it is a big deal. So if it helps you to have
the check
> for CWD you can leave it in.

I think it's good to keep it. For example, if someone runs `make build`
from a tree that's not on GOPATH it will build a different tree
(assuming one exists) that is on GOPATH, and it'll get confusing. I'll
change the message though, to something like "Cannot build; $(CURDIR) is
not found in GOPATH". I think that better explains what's happening.

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode64
Makefile:64: gofmt -w -l -s .
On 2013/08/28 06:23:17, jameinel wrote:
> Given that "time make format" and "time make simplify" doen't seem to
differ, we
> probably just want to always default to simplify.

> So:

> format: simplify

> simplify:
> gofmt -w -l -s .

Okay, I'll do that.

> Though really *I* prefer

> go fmt $(PROJECT)/...

> Because that makes sure I get the files in another directory. (If I'm
in the
> subdir cmd/juju I'm probably also hacking on code in environs/tools or
whatever)

> Then again, Makefile isn't available easily in subdirs, so it doesn't
matter
> *for me*.

Instead of using `make -f ../../Makefile format` you can use `make -C
../.. format` which will DTRT for you, and is shorter.

https://codereview.appspot.com/12949047/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/12949047/diff/7001/Makefile
File Makefile (right):

https://codereview.appspot.com/12949047/diff/7001/Makefile#newcode64
Makefile:64: gofmt -w -l -s .
On 2013/08/28 11:54:23, allenap wrote:
> On 2013/08/28 06:23:17, jameinel wrote:
> > Given that "time make format" and "time make simplify" doen't seem
to differ,
> we
> > probably just want to always default to simplify.
> >
> > So:
> >
> > format: simplify
> >
> > simplify:
> > gofmt -w -l -s .

> Okay, I'll do that.

I'm not sure that we should always run simplify.
I think that format should mean just exactly gofmt;
"simplify" is a step further and should only be
done with a watchful eye, IMHO.

https://codereview.appspot.com/12949047/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Why is that? The code is currently simplify clean. What do you expect
it to break?

John
=:->

>
> I'm not sure that we should always run simplify. I think that
> format should mean just exactly gofmt; "simplify" is a step further
> and should only be done with a watchful eye, IMHO.
>
> https://codereview.appspot.com/12949047/
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIeDz0ACgkQJdeBCYSNAANgpwCgo7dIJ4kOlFb4BVsuulpBe5s9
9AEAmQE8j06+pHazMlGnHa1zBOCveu65
=o3OY
-----END PGP SIGNATURE-----

[Roger Peppe (rogpeppe)]

On 28 August 2013 15:54, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Why is that? The code is currently simplify clean. What do you expect
> it to break?

Perhaps it's just that I haven't looked into what tranformations
simplify applies. But as long as gofmt is different from gofmt -s,
I'm think I'm happier to keep "make format" distinct from "make simplify".

[Go Bot (go-bot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Gavin Panella (allenap)]

Please take a look.

https://codereview.appspot.com/12949047/

[Gavin Panella (allenap)]

LGTM self-review.

https://codereview.appspot.com/12949047/

[Go Bot (go-bot)]

The attempt to merge lp:~allenap/juju-core/makefile-stuff into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.733s
ok launchpad.net/juju-core/agent/tools 0.238s
ok launchpad.net/juju-core/bzr 7.010s
ok launchpad.net/juju-core/cert 1.929s
ok launchpad.net/juju-core/charm 0.546s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.019s
ok launchpad.net/juju-core/cmd 0.275s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 113.072s
panic: watcher was stopped cleanly

goroutine 762 [running]:
launchpad.net/juju-core/state/watcher.MustErr(0xc20073f5d0, 0xc200516900, 0x1237ce8, 0xc20073f5d0)
 /home/tarmac/trees/src/launchpad.net/juju-core/state/watcher/helpers.go:33 +0x86
launchpad.net/juju-core/state.(*entityWatcher).loop(0xc2003e1500, 0xc200e15e10, 0xc2004d7448, 0x1, 0x0, ...)
 /home/tarmac/trees/src/launchpad.net/juju-core/state/watcher.go:1070 +0x460
launchpad.net/juju-core/state.func·029()
 /home/tarmac/trees/src/launchpad.net/juju-core/state/watcher.go:1041 +0xa4
created by launchpad.net/juju-core/state.newEntityWatcher
 /home/tarmac/trees/src/launchpad.net/juju-core/state/watcher.go:1042 +0xfb

goroutine 1 [chan receive]:
testing.RunTests(0xd9b818, 0x122c700, 0x2, 0x2, 0xc2001b0701, ...)
 /usr/lib/go/src/pkg/testing/testing.go:434 +0x88e
testing.Main(0xd9b818, 0x122c700, 0x2, 0x2, 0x1236ea0, ...)
 /usr/lib/go/src/pkg/testing/testing.go:365 +0x8a
main.main()
 launchpad.net/juju-core/cmd/jujud/_test/_testmain.go:45 +0x9a

goroutine 2 [syscall]:

goroutine 5 [chan receive]:
launchpad.net/juju-core/provider/dummy.func·001()
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:177 +0x3a
created by launchpad.net/juju-core/provider/dummy.init·1
 /home/tarmac/trees/src/launchpad.net/juju-core/provider/dummy/environs.go:179 +0xc1

goroutine 7 [chan receive]:
launchpad.net/gocheck.(*suiteRunner).runTest(0xc2002c31c0, 0xc2002c39a0, 0xc2004183c0)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:771 +0x4f
launchpad.net/gocheck.(*suiteRunner).run(0xc2002c31c0, 0xc2001dc280)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/gocheck.go:584 +0x1c5
launchpad.net/gocheck.Run(0xc239c0, 0xc2001dc280, 0xc2001c9c80, 0xc20027b380)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:76 +0x47
launchpad.net/gocheck.RunAll(0xc2001c9c80, 0xc2001c9c80)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:68 +0xc2
launchpad.net/gocheck.TestingT(0xc2001b0870)
 /home/tarmac/trees/src/src/launchpad.net/gocheck/run.go:56 +0x2a2
launchpad.net/juju-core/testing.MgoTestPackage(0xc2001b0870)
 /home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:134 +0xcf
launchpad.net/juju-core/cmd/jujud.TestPackage(0xc2001b0870)
 /home/tarmac/trees/src/launchpad.net/juju-core/cmd/jujud/main_test.go:40 +0x149
testing.tRunner(0xc2001b0870, 0x122c700)
 /usr/lib/go/src/pkg/testing/testing.go:353 +0x8a
created by testing.RunTests
 /usr/lib/go/src/pkg/testing/testing.go:433 +0x86b

gor...

=== modified file '.bzrignore'
--- .bzrignore	2013-02-27 14:48:01 +0000
+++ .bzrignore	2013-08-28 20:12:35 +0000
@@ -7,3 +7,4 @@
 ./TAGS
 .emacs.desktop
 .emacs.desktop.lock
+*.test

=== modified file 'Makefile'
--- Makefile	2013-08-13 10:58:22 +0000
+++ Makefile	2013-08-28 20:12:35 +0000
@@ -1,40 +1,82 @@
+#
 # Makefile for juju-core.
-PROJECT=launchpad.net/juju-core
-
-# Default target.  Compile, just to see if it will.
+#
+
+ifndef GOPATH
+$(warning You need to set up a GOPATH.  See the README file.)
+endif
+
+PROJECT := launchpad.net/juju-core
+PROJECT_DIR := $(shell go list -e -f '{{.Dir}}' $(PROJECT))
+
+define DEPENDENCIES
+  build-essential
+  bzr
+  distro-info-data
+  git-core
+  golang
+  mercurial
+  mongodb-server
+  zip
+endef
+
+default: build
+
+# Start of GOPATH-dependent targets. Some targets only make sense -
+# and will only work - when this tree is found on the GOPATH.
+ifeq ($(CURDIR),$(PROJECT_DIR))
+
 build:
 	go build $(PROJECT)/...
 
-# Run tests.
 check:
 	go test $(PROJECT)/...
 
-# Reformat the source files.
+install:
+	go install -v $(PROJECT)/...
+
+clean:
+	go clean $(PROJECT)/...
+
+else # --------------------------------
+
+build:
+	$(error Cannot $@; $(CURDIR) is not on GOPATH)
+
+check:
+	$(error Cannot $@; $(CURDIR) is not on GOPATH)
+
+install:
+	$(error Cannot $@; $(CURDIR) is not on GOPATH)
+
+clean:
+	$(error Cannot $@; $(CURDIR) is not on GOPATH)
+
+endif
+# End of GOPATH-dependent targets.
+
+# Reformat source files.
 format:
-	go fmt $(PROJECT)/...
-
-# Install juju into $GOPATH/bin.
-install:
-	go install -v $(PROJECT)/...
-
-# Install packages required to develop Juju and run tests.
+	gofmt -w -l .
+
+# Reformat and simplify source files.
+simplify:
+	gofmt -w -l -s .
+
+# Install packages required to develop Juju and run tests. The stable
+# PPA includes the required mongodb-server binaries. However, neither
+# PPA works on Saucy just yet.
 install-dependencies:
+ifneq ($(shell lsb_release -cs),saucy)
 	@echo Adding juju PPAs for golang and mongodb-server
-	@sudo apt-add-repository ppa:juju/golang
-	# The stable PPA includes the required mongodb-server binaries.
-	@sudo apt-add-repository ppa:juju/stable
+	@sudo apt-add-repository --yes ppa:juju/golang
+	@sudo apt-add-repository --yes ppa:juju/stable
 	@sudo apt-get update
+endif
 	@echo Installing dependencies
-	@sudo apt-get install golang mongodb-server build-essential bzr \
-		zip git-core mercurial distro-info-data
-	@if [ -z "$(GOPATH)" ]; then \
-		echo; \
-		echo "You need to set up a GOPATH.  See the README file."; \
-	fi
-
-# Invoke gofmt's "simplify" option to streamline the source code.
-simplify:
-	find "$(GOPATH)/src/$(PROJECT)/" -name \*.go | xargs gofmt -w -s
-
-
-.PHONY: build check format install-dependencies simplify
+	@sudo apt-get --yes install $(strip $(DEPENDENCIES))
+
+
+.PHONY: build check install
+.PHONY: clean format simplify
+.PHONY: install-dependencies