Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile kops-install default #3426

Merged
merged 2 commits into from
Sep 22, 2017

Conversation

alrs
Copy link
Contributor

@alrs alrs commented Sep 21, 2017

Makefile binary targets depend on SOURCES

I botched an attempt to merge master into the previous PR, so here is a do-over.

make will install the kops binary in $GOPATH/bin by default.

The binary targets all depend on the ${SOURCES} variable, so they will be rebuilt whenever any .go file in the tree is modified.

There was a concern in the previous PR from @chrislovecnm about missing .PHONY declarations. .PHONY is only needed for targets that aren't named after the artifact they create. Do you see any examples of .PHONY targets that aren't maked as such?

Makefile binary targets depend on SOURCES
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 21, 2017
@alrs
Copy link
Contributor Author

alrs commented Sep 21, 2017 via email

@@ -93,8 +96,21 @@ ifndef SHASUMCMD
$(error "Neither sha1sum nor shasum command is available")
endif

.PHONY: kops-install # Install kops to local $GOPATH/bin
kops-install: ${KOPS}
cp ${KOPS} ${GOPATH_1ST}/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is go build still much slower than go install?

@justinsb
Copy link
Member

@alrs the reason we've used PHONY for targets is because it's hard to compute the set of dependencies for a go program, and go install does a really good job. Unless we're going to declare all the dependencies, we need PHONY for a target with the same name as the output file. Or am I missing a trick?

@alrs
Copy link
Contributor Author

alrs commented Sep 22, 2017

@justinsb The SOURCES variable is a list of all .go extension files in the tree. If any of them change the non-PHONY binary targets will be rebuilt on a subsequent run of make.

kops-dev: ${BINDATA_TARGETS}
go install ${EXTRA_BUILDFLAGS} -ldflags "-X k8s.io/kops.Version=${VERSION} -X k8s.io/kops.GitVersion=${GITSHA} ${EXTRA_LDFLAGS}" k8s.io/kops/cmd/kops/

.PHONY: ${GOBINDATA}
${GOBINDATA}:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs .PHONY or some deps

@justinsb
Copy link
Member

OK, I still think we might need some more deps but I'm going to get this merged along with the others so we can have a look.

BTW, @alrs have you check out bazel - once I saw bazel I kinda felt that as long as the makefile worked, that was good enough - the future is coming! Now if only they didn't keep breaking it ;-)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alrs, justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. .

@k8s-github-robot k8s-github-robot merged commit a1a3625 into kubernetes:master Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants