Skip to content

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 [email protected]

Makefile Outdated
default: docs

.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

@vbatts vbatts force-pushed the target-install-tools branch from db96e57 to a79fdef Compare March 21, 2016 20:38
@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 vbatts force-pushed the target-install-tools branch from 0e4425f to 8f70c5f Compare April 12, 2016 02:58
@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 [email protected]
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

vbatts added 2 commits April 27, 2016 13:11
As we have several tools used in the Makefile, might as well make them
easier to install.

Signed-off-by: Vincent Batts <[email protected]>
@vbatts vbatts force-pushed the target-install-tools branch from 8f70c5f to c4b846c Compare April 27, 2016 17:16
@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