Merge lp:~ericsnowcurrently/fake-juju/makefile-tweaks into lp:~landscape/fake-juju/trunk-old

[Eric Snow]

Refactor Makefile to support targeting a single version.

This patch also adds the "install-dev" target.

Refactor Makefile to support targeting a single version.

This patch also adds the "install-dev" target.

Testing instructions:

Run each of the make targets, with and without JUJU_VERSION set.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 37
Branch: lp:~ericsnowcurrently/fake-juju/makefile-tweaks
Jenkins: https://ci.lscape.net/job/latch-test-xenial/10/

review: Approve (test results)

[Free Ekanayaka (free.ekanayaka)]

I'm a bit confused, it seems the makefile already supports targeting single versions? E.g.:

JUJU_VERSION=1.25.6 make build-common

Already does the right thing in trunk.

Not sure what I'm missing.

review: Needs Information

[Eric Snow (ericsnowcurrently)]

Yes, you could do it before. However, it wasn't very clear. Now the version-specific case is much clearer.

[Chad Smith (chad.smith)]

I like the changes as-is as it feels like a slight improvement, we can tweak as we need to.
The looping construct over "all" targets seems to exit too quickly (@1.24.7) if nothing is needed for that target.

If I run initially make all targets, make changes to 2.0.X, the subsequent make attempt will exit early upon first noop success without trying to build 1.25 or 2.0 targets

make: '1.24.7/1.24.7' is up to date.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

I find it a bit odd to have those "duplicate" targets with a different implementation depending on whether JUJU_VERSION is set or not. Personally, I'd find more natural to have a JUJU_VERSIONS environment variable instead, that should be set to a space or comma separated list of versions you want to build, that defaults to all versions. This will both make the Makefile more natural and allow you to possibly build not only one specific version but also more than one version. If you want to keep the JUJU_VERSION variable, you could add support for that by making JUJU_VERSIONS=$(JUJU_VERSION) if it's set.

All in all, I'm fine with the idea of the branch, but find the implementation a bit convoluted.

I'm keeping the N/I vote to see what you think of the above, but since it's all relatively minor, I'll be fine if you want to keep the implementation like it.s

[Eric Snow (ericsnowcurrently)]

@chad.smith, could you give me explicit steps to reproduce? I tried:

1. make build
2. rm 1.25.6/1.25.6
3. make build

and it did the right thing.

[Eric Snow (ericsnowcurrently)]

@free.ekanayaka, we have to accommodate the "$(JUJU_VERSION)/$(JUJU_VERSION)" case as a target to leverage "make" properly. So Makefile must handle both the single and the multiple cases separately. Makefile would definitely be simpler if that were not the case.

[Free Ekanayaka (free.ekanayaka)]

@eric: hmm, I didn't dig but my impression is that "$(JUJU_VERSION)/$(JUJU_VERSION)" is just an internal naming detail. I believe you could name it $(VERSION)/$(VERSION) and set VERSION as appropriate and have it all working as described (i.e. single-version would be a particular case of multi-version, not a case on its own). Anyway, it's a minor detail as long as it works and you're happy enough with it (exp. from a code maintenance point of view), so +1.

review: Approve

=== modified file 'Makefile'
--- Makefile	2016-09-15 20:06:25 +0000
+++ Makefile	2016-09-22 15:58:27 +0000
@@ -1,24 +1,103 @@
+JUJUCLIENT_DOWNLOADS = $(shell pwd)/tests/jujuclient-archive
+JUJUCLIENT_REQ = $(JUJUCLIENT_DOWNLOADS)/requirements
+
+
+ifdef JUJU_VERSION  #############################
+# for one version
+
+GO_VERSION = 1.6
+JUJU_TARBALL = juju-core_$(JUJU_VERSION).tar.gz
+JUJU_PATCH = patches/juju-core_$(JUJU_VERSION).patch
+INSTALLDIR = $(DESTDIR)/usr/bin
+INSTALLED = $(INSTALLDIR)/fake-juju-$(JUJU_VERSION)
+
+$(JUJU_VERSION)/$(JUJU_VERSION):
+	case $(JUJU_VERSION) in \
+		1.*) $(MAKE) build-common PATH=$$PATH JUJU_VERSION=$(JUJU_VERSION) ;;\
+		2.*) $(MAKE) build-common PATH=/usr/lib/go-$(GO_VERSION)/bin:$$PATH JUJU_VERSION=$(JUJU_VERSION) ;;\
+	esac
+
+juju-core_%.tar.gz:
+	case $* in \
+		1.*) PROJECT="juju-core" ;;\
+		2.*) PROJECT="juju" ;;\
+	esac;\
+	wget https://launchpad.net/$$PROJECT/$(shell (echo $* | cut -f 1 -d - | cut -f 1,2 -d .))/$*/+download/$@
+
+.PHONY: build
+build: $(JUJU_VERSION)/$(JUJU_VERSION)
+
+.PHONY: build-common
+build-common: $(JUJU_TARBALL) $(JUJU_PATCH)
+	rm -rf $(JUJU_VERSION)/src  # Go doesn't play nice with existing files.
+	tar -C $(JUJU_VERSION) --strip=1 -z -xf $(JUJU_TARBALL)
+	patch -p0 < $(JUJU_PATCH)
+	cd $(JUJU_VERSION) && GOPATH=$(shell pwd)/$(JUJU_VERSION) PATH=$(PATH) go build
+
+.PHONY: install
+install: $(JUJU_VERSION)/$(JUJU_VERSION)
+	install -D $(JUJU_VERSION)/$(JUJU_VERSION) $(INSTALLED)
+
+.PHONY: install-dev
+install-dev: $(JUJU_VERSION)/$(JUJU_VERSION)
+	mkdir -p $(INSTALLDIR)
+	ln -s --backup=existing --suffix .orig $(shell pwd)/$(JUJU_VERSION)/$(JUJU_VERSION) $(INSTALLED)
+
+.PHONY: clean
+clean:
+	rm -f $(JUJU_TARBALL)
+	rm -rf $(JUJU_VERSION)/src
+	rm -f $(JUJU_VERSION)/$(JUJU_VERSION)
+	rm -rf _trial_temp
+	rm -f tests/*.pyc
+
+.PHONY: test
+test: $(JUJU_VERSION)/$(JUJU_VERSION)
+	env JUJU_VERSION=$(JUJU_VERSION) python3 -m unittest tests.test_fake
+
+
+else  ###########################################
+# for all versions
+
 JUJU1_VERSIONS = 1.24.7 1.25.6
 JUJU2_VERSIONS = 2.0-beta17
 VERSIONS = $(JUJU1_VERSIONS) $(JUJU2_VERSIONS)
-GO_VERSION = 1.6
-TARBALLS = $(foreach version,$(VERSIONS),juju-core_$(version).tar.gz)
 BUILT_VERSIONS = $(foreach version,$(VERSIONS),$(version)/$(version))
-JUJU_TARBALL = juju-core_$(JUJU_VERSION).tar.gz
-JUJU_PATCH = patches/juju-core_$(JUJU_VERSION).patch
-JUJUCLIENT_DOWNLOADS = $(shell pwd)/tests/jujuclient-archive
-JUJUCLIENT_REQ = $(JUJUCLIENT_DOWNLOADS)/requirements
+
+$(BUILT_VERSIONS):
+	for VERSION in $(VERSIONS); do \
+	    $(MAKE) build JUJU_VERSION=$$VERSION; \
+	done
 
 .PHONY: build
 build: $(BUILT_VERSIONS)
 
-$(BUILT_VERSIONS):
-	for VERSION in $(JUJU1_VERSIONS); do \
-	    $(MAKE) build-common PATH=$$PATH JUJU_VERSION=$$VERSION; \
-	done
-	for VERSION in $(JUJU2_VERSIONS); do \
-		$(MAKE) build-common PATH=/usr/lib/go-$(GO_VERSION)/bin:$$PATH JUJU_VERSION=$$VERSION; \
-	done
+.PHONY: install
+install:
+	for VERSION in $(VERSIONS); do \
+	    $(MAKE) install JUJU_VERSION=$$VERSION; \
+	done
+
+.PHONY: install-dev
+install-dev:
+	for VERSION in $(VERSIONS); do \
+	    $(MAKE) install-dev JUJU_VERSION=$$VERSION; \
+	done
+
+.PHONY: clean
+clean:
+	for VERSION in $(VERSIONS) ; do \
+		$(MAKE) clean JUJU_VERSION=$$VERSION; \
+	done	
+
+.PHONY: test
+# Use xargs here so that we don't throw away the return codes, and correctly fail if any of the tests fail
+test: $(BUILT_VERSIONS)
+	#@echo -n $(VERSIONS) | xargs -t -d' ' -I {} env JUJU_VERSION={} python3 -m unittest tests.test_fake
+	@echo -n $(VERSIONS) | xargs -t -d' ' -I {} $(MAKE) test JUJU_VERSION={}
+
+
+endif  ##########################################
 
 .PHONY: ci-test
 ci-test:
@@ -34,47 +113,4 @@
 		--no-index \
 		--find-links $(JUJUCLIENT_DOWNLOADS) \
 		--requirement $(JUJUCLIENT_REQ)
-	make test
-
-.PHONY: build-common
-build-common: $(JUJU_TARBALL) $(JUJU_PATCH)
-	rm -rf $(JUJU_VERSION)/src  # Go doesn't play nice with existing files.
-	tar -C $(JUJU_VERSION) --strip=1 -z -xf $(JUJU_TARBALL)
-	patch -p0 < $(JUJU_PATCH)
-	cd $(JUJU_VERSION) && GOPATH=$(shell pwd)/$(JUJU_VERSION) PATH=$(PATH) go build
-
-.PHONY: install
-install: 
-	for VERSION in $(VERSIONS); do \
-	    $(MAKE) install-common JUJU_VERSION=$$VERSION; \
-	done
-
-.PHONY: install-common
-install-common: $(JUJU_VERSION)/$(JUJU_VERSION)
-	install -D $(JUJU_VERSION)/$(JUJU_VERSION) $(DESTDIR)/usr/bin/fake-juju-$(JUJU_VERSION)
-
-.PHONY: clean
-clean:
-	for VERSION in $(VERSIONS) ; do \
-	    JUJU_VERSION=$$VERSION make clean-common; \
-	done	
-
-.PHONY: clean-common
-clean-common:
-	rm -f $(JUJU_TARBALL)
-	rm -rf $(JUJU_VERSION)/src
-	rm -f $(JUJU_VERSION)/$(JUJU_VERSION)
-	rm -rf _trial_temp
-	rm -f tests/*.pyc
-
-.PHONY: test
-# Use xargs here so that we don't throw away the return codes, and correctly fail if any of the tests fail
-test: $(BUILT_VERSIONS)
-	@echo -n $(VERSIONS) | xargs -t -d' ' -I {} env JUJU_VERSION={} python3 -m unittest tests.test_fake
-
-juju-core_%.tar.gz:
-	case $* in \
-		1.*) PROJECT="juju-core" ;;\
-		2.*) PROJECT="juju" ;;\
-	esac;\
-	wget https://launchpad.net/$$PROJECT/$(shell (echo $* | cut -f 1 -d - | cut -f 1,2 -d .))/$*/+download/$@
+	$(MAKE) test