Fake Juju

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

  1. makefile-tweaks
  2. Merge into trunk-old
Proposed by Eric Snow
Status: Merged
Approved by: Eric Snow
Approved revision: 37
Merged at revision: 36
Proposed branch: lp:~ericsnowcurrently/fake-juju/makefile-tweaks
Merge into: lp:~landscape/fake-juju/trunk-old
Diff against target: 169 lines (+93/-57)
1 file modified
Makefile (+93/-57)
To merge this branch: bzr merge lp:~ericsnowcurrently/fake-juju/makefile-tweaks
Reviewer Review Type Date Requested Status
Free Ekanayaka (community) Approve
Chad Smith Approve
🤖 Landscape Builder test results Approve
Review via email: mp+306506@code.launchpad.net

Commit message

Refactor Makefile to support targeting a single version.

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

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)
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

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
Revision history for this message
Eric Snow (ericsnowcurrently) wrote :

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

Revision history for this message
Chad Smith (chad.smith) wrote :

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
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

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

Revision history for this message
Eric Snow (ericsnowcurrently) wrote :

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

Revision history for this message
Eric Snow (ericsnowcurrently) wrote :

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

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

@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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile'
--- Makefile 2016-09-15 20:06:25 +0000
+++ Makefile 2016-09-22 15:58:27 +0000
@@ -1,24 +1,103 @@
1JUJUCLIENT_DOWNLOADS = $(shell pwd)/tests/jujuclient-archive
2JUJUCLIENT_REQ = $(JUJUCLIENT_DOWNLOADS)/requirements
3
4
5ifdef JUJU_VERSION #############################
6# for one version
7
8GO_VERSION = 1.6
9JUJU_TARBALL = juju-core_$(JUJU_VERSION).tar.gz
10JUJU_PATCH = patches/juju-core_$(JUJU_VERSION).patch
11INSTALLDIR = $(DESTDIR)/usr/bin
12INSTALLED = $(INSTALLDIR)/fake-juju-$(JUJU_VERSION)
13
14$(JUJU_VERSION)/$(JUJU_VERSION):
15 case $(JUJU_VERSION) in \
16 1.*) $(MAKE) build-common PATH=$$PATH JUJU_VERSION=$(JUJU_VERSION) ;;\
17 2.*) $(MAKE) build-common PATH=/usr/lib/go-$(GO_VERSION)/bin:$$PATH JUJU_VERSION=$(JUJU_VERSION) ;;\
18 esac
19
20juju-core_%.tar.gz:
21 case $* in \
22 1.*) PROJECT="juju-core" ;;\
23 2.*) PROJECT="juju" ;;\
24 esac;\
25 wget https://launchpad.net/$$PROJECT/$(shell (echo $* | cut -f 1 -d - | cut -f 1,2 -d .))/$*/+download/$@
26
27.PHONY: build
28build: $(JUJU_VERSION)/$(JUJU_VERSION)
29
30.PHONY: build-common
31build-common: $(JUJU_TARBALL) $(JUJU_PATCH)
32 rm -rf $(JUJU_VERSION)/src # Go doesn't play nice with existing files.
33 tar -C $(JUJU_VERSION) --strip=1 -z -xf $(JUJU_TARBALL)
34 patch -p0 < $(JUJU_PATCH)
35 cd $(JUJU_VERSION) && GOPATH=$(shell pwd)/$(JUJU_VERSION) PATH=$(PATH) go build
36
37.PHONY: install
38install: $(JUJU_VERSION)/$(JUJU_VERSION)
39 install -D $(JUJU_VERSION)/$(JUJU_VERSION) $(INSTALLED)
40
41.PHONY: install-dev
42install-dev: $(JUJU_VERSION)/$(JUJU_VERSION)
43 mkdir -p $(INSTALLDIR)
44 ln -s --backup=existing --suffix .orig $(shell pwd)/$(JUJU_VERSION)/$(JUJU_VERSION) $(INSTALLED)
45
46.PHONY: clean
47clean:
48 rm -f $(JUJU_TARBALL)
49 rm -rf $(JUJU_VERSION)/src
50 rm -f $(JUJU_VERSION)/$(JUJU_VERSION)
51 rm -rf _trial_temp
52 rm -f tests/*.pyc
53
54.PHONY: test
55test: $(JUJU_VERSION)/$(JUJU_VERSION)
56 env JUJU_VERSION=$(JUJU_VERSION) python3 -m unittest tests.test_fake
57
58
59else ###########################################
60# for all versions
61
1JUJU1_VERSIONS = 1.24.7 1.25.662JUJU1_VERSIONS = 1.24.7 1.25.6
2JUJU2_VERSIONS = 2.0-beta1763JUJU2_VERSIONS = 2.0-beta17
3VERSIONS = $(JUJU1_VERSIONS) $(JUJU2_VERSIONS)64VERSIONS = $(JUJU1_VERSIONS) $(JUJU2_VERSIONS)
4GO_VERSION = 1.6
5TARBALLS = $(foreach version,$(VERSIONS),juju-core_$(version).tar.gz)
6BUILT_VERSIONS = $(foreach version,$(VERSIONS),$(version)/$(version))65BUILT_VERSIONS = $(foreach version,$(VERSIONS),$(version)/$(version))
7JUJU_TARBALL = juju-core_$(JUJU_VERSION).tar.gz66
8JUJU_PATCH = patches/juju-core_$(JUJU_VERSION).patch67$(BUILT_VERSIONS):
9JUJUCLIENT_DOWNLOADS = $(shell pwd)/tests/jujuclient-archive68 for VERSION in $(VERSIONS); do \
10JUJUCLIENT_REQ = $(JUJUCLIENT_DOWNLOADS)/requirements69 $(MAKE) build JUJU_VERSION=$$VERSION; \
70 done
1171
12.PHONY: build72.PHONY: build
13build: $(BUILT_VERSIONS)73build: $(BUILT_VERSIONS)
1474
15$(BUILT_VERSIONS):75.PHONY: install
16 for VERSION in $(JUJU1_VERSIONS); do \76install:
17 $(MAKE) build-common PATH=$$PATH JUJU_VERSION=$$VERSION; \77 for VERSION in $(VERSIONS); do \
18 done78 $(MAKE) install JUJU_VERSION=$$VERSION; \
19 for VERSION in $(JUJU2_VERSIONS); do \79 done
20 $(MAKE) build-common PATH=/usr/lib/go-$(GO_VERSION)/bin:$$PATH JUJU_VERSION=$$VERSION; \80
21 done81.PHONY: install-dev
82install-dev:
83 for VERSION in $(VERSIONS); do \
84 $(MAKE) install-dev JUJU_VERSION=$$VERSION; \
85 done
86
87.PHONY: clean
88clean:
89 for VERSION in $(VERSIONS) ; do \
90 $(MAKE) clean JUJU_VERSION=$$VERSION; \
91 done
92
93.PHONY: test
94# Use xargs here so that we don't throw away the return codes, and correctly fail if any of the tests fail
95test: $(BUILT_VERSIONS)
96 #@echo -n $(VERSIONS) | xargs -t -d' ' -I {} env JUJU_VERSION={} python3 -m unittest tests.test_fake
97 @echo -n $(VERSIONS) | xargs -t -d' ' -I {} $(MAKE) test JUJU_VERSION={}
98
99
100endif ##########################################
22101
23.PHONY: ci-test102.PHONY: ci-test
24ci-test:103ci-test:
@@ -34,47 +113,4 @@
34 --no-index \113 --no-index \
35 --find-links $(JUJUCLIENT_DOWNLOADS) \114 --find-links $(JUJUCLIENT_DOWNLOADS) \
36 --requirement $(JUJUCLIENT_REQ)115 --requirement $(JUJUCLIENT_REQ)
37 make test116 $(MAKE) test
38
39.PHONY: build-common
40build-common: $(JUJU_TARBALL) $(JUJU_PATCH)
41 rm -rf $(JUJU_VERSION)/src # Go doesn't play nice with existing files.
42 tar -C $(JUJU_VERSION) --strip=1 -z -xf $(JUJU_TARBALL)
43 patch -p0 < $(JUJU_PATCH)
44 cd $(JUJU_VERSION) && GOPATH=$(shell pwd)/$(JUJU_VERSION) PATH=$(PATH) go build
45
46.PHONY: install
47install:
48 for VERSION in $(VERSIONS); do \
49 $(MAKE) install-common JUJU_VERSION=$$VERSION; \
50 done
51
52.PHONY: install-common
53install-common: $(JUJU_VERSION)/$(JUJU_VERSION)
54 install -D $(JUJU_VERSION)/$(JUJU_VERSION) $(DESTDIR)/usr/bin/fake-juju-$(JUJU_VERSION)
55
56.PHONY: clean
57clean:
58 for VERSION in $(VERSIONS) ; do \
59 JUJU_VERSION=$$VERSION make clean-common; \
60 done
61
62.PHONY: clean-common
63clean-common:
64 rm -f $(JUJU_TARBALL)
65 rm -rf $(JUJU_VERSION)/src
66 rm -f $(JUJU_VERSION)/$(JUJU_VERSION)
67 rm -rf _trial_temp
68 rm -f tests/*.pyc
69
70.PHONY: test
71# Use xargs here so that we don't throw away the return codes, and correctly fail if any of the tests fail
72test: $(BUILT_VERSIONS)
73 @echo -n $(VERSIONS) | xargs -t -d' ' -I {} env JUJU_VERSION={} python3 -m unittest tests.test_fake
74
75juju-core_%.tar.gz:
76 case $* in \
77 1.*) PROJECT="juju-core" ;;\
78 2.*) PROJECT="juju" ;;\
79 esac;\
80 wget https://launchpad.net/$$PROJECT/$(shell (echo $* | cut -f 1 -d - | cut -f 1,2 -d .))/$*/+download/$@

Subscribers

People subscribed via source and target branches

to all changes: