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: add target for tools #349

Merged
merged 2 commits into from
Apr 27, 2016

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Mar 17, 2016

As we have several tools used in the Makefile, might as well make them
easier to install.

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

.PHONY: docs
docs: pdf html
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're touching these targets, maybe adjust the pdf and html targets to actual filenames (output/docs.pdf?) or mark them phony as well. If you go the filename route, you'll want to specify prerequisites for both targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair

@wking wking mentioned this pull request Mar 22, 2016
@vbatts vbatts force-pushed the target-install-tools branch 3 times, most recently from 70be85b to 0e4425f Compare March 30, 2016 15:31
@vbatts
Copy link
Member Author

vbatts commented Apr 12, 2016

Updated, fixed and rebased. PTAL.


# golint does not even build for <go1.5
.install.golint:
ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably just remove the ALLOWED_GO_VERSION stuff, since c506ce6 (Fix the build by getting rid of go get for vet, 2016-04-07, #372) completely dropped Go < 1.5. We can always drag it out of the PR history if we need that sort of logic to partially support Go 1.5 (or whatever) in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's being used on the install of go vet, and someone's local go version
may vary.

On Tue, Apr 12, 2016 at 1:11 AM, W. Trevor King notifications@github.com
wrote:

In Makefile
#349 (comment)
:

git-validation -q -run DCO,short-subject -v -range $(EPOCH_TEST_COMMIT)..HEAD

+.PHONY: install.tools
+install.tools: .install.golint .install.govet .install.gitvalidation
+
+# golint does not even build for <go1.5
+.install.golint:
+ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

We can probably just remove the ALLOWED_GO_VERSION stuff, since c506ce6
c506ce6
(Fix the build by getting rid of go get for vet, 2016-04-07, #372
#372) completely
dropped Go < 1.5. We can always drag it out of the PR history if we need
that sort of logic to partially support Go 1.5 (or whatever) in the future.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/runtime-spec/pull/349/files/8f70c5f3d3a7b083248ca79edd1b9a261da4de09#r59321441

Copy link
Contributor

Choose a reason for hiding this comment

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

On Tue, Apr 12, 2016 at 06:49:07AM -0700, Vincent Batts wrote:

it's being used on the install of go vet, and someone's local go
version may vary.

Ah, good point. I guess folks might be running these checks outside
of Travis ;).

@crosbymichael
Copy link
Member

LGTM

As we have several tools used in the Makefile, might as well make them
easier to install.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts vbatts merged commit e6427cb into opencontainers:master Apr 27, 2016
@vbatts vbatts deleted the target-install-tools branch April 27, 2016 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants