Makefile: Add a pattern rule for oci-* commands #28

wking · 2016-09-22T05:51:02Z

Make it easy to (re)build a single command without building all of them. Using main.go as the only prerequisite cuts some corners, but prerequisites for this are hard ;). With this commit, you can touch main.go to force a rebuild. If that ends up being too awkward, we can make these .PHONY targets.

This is the current first commit in #5, to see if nibbling away at that PR one commit at a time makes review any easier.

runcom · 2016-09-22T16:40:39Z

If that ends up being too awkward, we can make these .PHONY targets.

yes please, split them into individual targets and then have "tools" to build them all

The previous commit only rebuilt them when the associated main.go was newer, which avoided some unnecessary rebuilds. Unfortunately, it also avoids rebuilding when another Go dependency changed, which could bite users that expected 'make tools' to give them fresh copies of the tools even if they only touched a dependency package. I'm fine either way, but Antonio prefers always rebuilding [1], so this commit adds it. I've switched the old pattern rule to a static pattern rule [2], because Make was treating the non-static pattern rules as implicit rules and .PHONY causes implicit rules to be skipped [3]. [1]: opencontainers#28 (comment) [2]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html [3]: https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html Signed-off-by: W. Trevor King <[email protected]>

wking · 2016-09-22T17:21:57Z

On Thu, Sep 22, 2016 at 09:40:40AM -0700, Antonio Murdaca wrote:

If that ends up being too awkward, we can make these .PHONY targets.

yes please, split them into individual targes and then have "tools"
to build them all

As is stood in 0179710, this PR already split them into individual
(pattern) targets and had the ‘tools’ target build them all. What it
did not do was complete dependency checking (which is tricky with Go)
to only rebuild when necessary. I've pushed 0179710 → 689ee3e with a
new commit that adds .PHONY to always rebuild. That rebuilds too
often, but it errs on the side of rebuilding while 0179710 errs on the
side of not-rebuilding.

vbatts · 2016-09-22T17:50:54Z

Makefile


 .PHONY: \
 	tools \
+	$(TOOLS) \


that is somewhat redundant

also, not true. When those binaries are built from the subdirectory, then the executable by those names is present.

vbatts · 2016-09-22T17:56:00Z

Makefile

-	go build ./cmd/oci-image-validate
+tools: $(TOOLS)
+
+$(TOOLS): oci-%:


Here the oci-% is looking for dependencies of the target that have the oci-% pattern. Is that intended?

wking · 2016-09-22T17:59:05Z

On Thu, Sep 22, 2016 at 10:50:54AM -0700, Vincent Batts wrote:

.PHONY:
tools \

$(TOOLS) \

that is somewhat redundant

We need $(TOOLS) so we rebuild them every time. You could remove
‘tools’ if you wanted, but there is no ‘tools’ file on the disk, so I
think keeping it in .PHONY is appropriate.

wking · 2016-09-22T18:01:31Z

On Thu, Sep 22, 2016 at 10:56:01AM -0700, Vincent Batts wrote:

+$(TOOLS): oci-%:

Here the oci-% is looking for dependencies of the target that have
the oci-% pattern. Is that intended?

This is a static pattern rule (linked from the the 689ee3e commit
message). It is saying “for all the targets in $(TOOLS) use the
‘oci-%: ’ pattern rule”. It has no dependencies (entries after the
second colon).

vbatts · 2016-09-22T18:27:41Z

would almost seem like to get the Makefile dependency usefulness, we could have something like

GO_DEPS = $(shell go list -f '{{range .Deps}}{{printf "%s" .}}{{end}}' $(1))
GO_SOURCE_FILES = $(shell go list -f '{{range .GoFiles}}{{printf "%s" .}}{{end}}' $(1))

oci-%: $(call GO_SOURCE_FILES,$(call GO_DEPS,./cmd/oci-%))
        go build ./cmd/$@

or similar

wking · 2016-09-22T18:44:08Z

On Thu, Sep 22, 2016 at 11:27:41AM -0700, Vincent Batts wrote:

would almost seem like to get the Makefile dependency usefulness, we could have something like

GO_DEPS = $(shell go list -f '{{range .Deps}}{{printf "%s" .}}{{end}}' $(1))
GO_SOURCE_FILES = $(shell go list -f '{{range .GoFiles}}{{printf "%s" .}}{{end}}' $(1))

oci-%: $(call GO_SOURCE_FILES,$(call GO_DEPS,./cmd/oci-%))

Having an accurate list of dependencies would be awesome, but the
.GoFiles line seems to print the filename and not the full path to the
file. I'm happy to add this sort of checking if you know the proper
incantation to get the full path.

On the other hand, ‘go build’ (as we call it without -a) only rebuilds
packages that are out of date, so I'm fine leaving this PR at
689ee3e, calling ‘go build’ every time, and having it bail out
if/when dependent packages are up to date.

vbatts · 2016-09-22T18:48:06Z

yes. go build is nicer.

wking · 2016-10-01T04:00:35Z

On Thu, Sep 22, 2016 at 11:48:06AM -0700, Vincent Batts wrote:

yes. go build is nicer.

So we're both happy with this PR as it stands with 689ee3e? Or is
there anything else I can do to help move this along?

vbatts · 2016-10-06T16:18:34Z

please rebase

Make it easy to (re)build a single command without building all of them. Using main.go as the only prerequisite cuts some corners, but prerequisites for this are hard ;). With this commit, you can touch main.go to force a rebuild. If that ends up being too awkward, we can make these .PHONY targets. Signed-off-by: W. Trevor King <[email protected]>

The previous commit only rebuilt them when the associated main.go was newer, which avoided some unnecessary rebuilds. Unfortunately, it also avoids rebuilding when another Go dependency changed, which could bite users that expected 'make tools' to give them fresh copies of the tools even if they only touched a dependency package. I'm fine either way, but Antonio prefers always rebuilding [1], so this commit adds it. I've switched the old pattern rule to a static pattern rule [2], because Make was treating the non-static pattern rules as implicit rules and .PHONY causes implicit rules to be skipped [3]. [1]: opencontainers#28 (comment) [2]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html [3]: https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html Signed-off-by: W. Trevor King <[email protected]>

wking · 2016-10-06T16:24:58Z

Rebased around #9 with 689ee3e → 2d06368.

vbatts · 2016-10-06T16:27:40Z

LGTM

vbatts · 2016-10-06T16:35:20Z

welp https://travis-ci.org/opencontainers/image-tools/builds/165582442

wking · 2016-10-06T16:39:04Z

On Thu, Oct 06, 2016 at 09:35:21AM -0700, Vincent Batts wrote:

welp https://travis-ci.org/opencontainers/image-tools/builds/165582442

Is that lint timeout reproducible? I'd just kick it again.

vbatts · 2016-10-06T17:13:01Z

funky. that did the trick.

wking mentioned this pull request Sep 22, 2016

image: Refactor to use cas/ref engines instead of walkers #5

Open

vbatts reviewed Sep 22, 2016

View reviewed changes

wking mentioned this pull request Sep 30, 2016

Add make install support #31

Closed

wking added 2 commits October 6, 2016 09:21

wking force-pushed the makefile-pattern-rule branch from 689ee3e to 2d06368 Compare October 6, 2016 16:22

vbatts merged commit ff224f8 into opencontainers:master Oct 6, 2016

wking deleted the makefile-pattern-rule branch October 15, 2016 06:04

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged

Makefile: Add a pattern rule for oci-* commands #28

Makefile: Add a pattern rule for oci-* commands #28

Uh oh!

Conversation

wking commented Sep 22, 2016

Uh oh!

runcom commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Sep 22, 2016

Uh oh!

vbatts Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

vbatts Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

vbatts Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 22, 2016

Uh oh!

wking commented Sep 22, 2016

Uh oh!

vbatts commented Sep 22, 2016

Uh oh!

wking commented Sep 22, 2016

Uh oh!

vbatts commented Sep 22, 2016

Uh oh!

wking commented Oct 1, 2016

Uh oh!

vbatts commented Oct 6, 2016

Uh oh!

wking commented Oct 6, 2016 via email

Uh oh!

vbatts commented Oct 6, 2016

Uh oh!

vbatts commented Oct 6, 2016

Uh oh!

wking commented Oct 6, 2016

Uh oh!

vbatts commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

runcom commented Sep 22, 2016 •

edited

Loading