ci-tools: versions of golang #547

vbatts · 2016-08-29T13:41:44Z

Add and update golang versions. Also fix install.tools target for
installing govet

Signed-off-by: Vincent Batts [email protected]

With dash 0.5.7: # make docs /bin/sh: 1: test: 1.3.3: unexpected operator /bin/sh: 1: test: 1.3.3: unexpected operator /bin/sh: 1: test: 1.3.3: unexpected operator Makefile:47: *** cannot build output//oci-runtime-spec.pdf without either pandoc or docker. Stop. # command -V test test is a shell builtin POSIX defines '=' for string comparison [1]; the '==' form is a Bashism. SHELL was added in f3fdf03 (Makefile: prefer bash, 2016-05-25, opencontainers#455) to avoid these "unexpected operator" errors, but there's no reason to require Bash when we can make the comparison's POSIX compliant. [1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html Signed-off-by: W. Trevor King <[email protected]>

wking · 2016-08-29T18:06:09Z

Makefile

 OUTPUT_DIRNAME		?= output/
 DOC_FILENAME		?= oci-runtime-spec
-SHELL			?= $(shell command -v bash 2>/dev/null)
+SHELL			?= $(shell command -v sh 2>/dev/null)


I don't think we need this. From the GNU Make docs:

The program used as the shell is taken from the variable SHELL. If this variable is not set in your makefile, the program /bin/sh is used as the shell.

Although GNU Make does some dancing for DOS/Windows. Are you including this line to skip that dance? If so, why?

fixed it

On Mon, Aug 29, 2016 at 2:06 PM W. Trevor King [email protected]
wrote:

In Makefile
#547 (comment)
:

@@ -2,7 +2,7 @@
EPOCH_TEST_COMMIT := 78e6667
OUTPUT_DIRNAME ?= output/
DOC_FILENAME ?= oci-runtime-spec
-SHELL ?= $(shell command -v bash 2>/dev/null)
+SHELL ?= $(shell command -v sh 2>/dev/null)

I don't think we need this. From the GNU Make docs
https://www.gnu.org/software/make/manual/html_node/Choosing-the-Shell.html
:

The program used as the shell is taken from the variable SHELL. If this
variable is not set in your makefile, the program /bin/sh is used as the
shell.

Although GNU Make does some dancing for DOS/Windows. Are you including
this line to skip that dance? If so, why?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/opencontainers/runtime-spec/pull/547/files/300eb002e587ff9851578f9147b438cc53c9b5d0#r76655651,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6Wsldb7cZg5PLBbm9ycIlQ_OtVrrks5qkx-SgaJpZM4JvfF1
.

wking · 2016-08-29T19:59:14Z

Makefile

 .install.govet:
-ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)
-	go get golang.org/x/tools/cmd/vet
+ifneq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)


This looks like it's allowing 1.4 again. I think we want to check if the Go version is in a whitelisted set, or not in a blacklisted set.

But we're not testing against 1.4 in Travis.

On Mon, Aug 29, 2016, 15:59 W. Trevor King [email protected] wrote:

In Makefile
#547 (comment)
:

endif

go vet is now included in >=go1.5, so no need to get it.

.install.govet:
-ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

go get golang.org/x/tools/cmd/vet
+ifneq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

This looks like it's allowing 1.4 again. I think we want to check if the
Go version is in a whitelisted set, or not in a blacklisted set.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/opencontainers/runtime-spec/pull/547/files/a24d96131cdcc41bff4cd64cae01dfe719a79000#r76675840,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6XSyNiLO20rjNI6gDypnullgTJ6Rks5qkzoTgaJpZM4JvfF1
.

On Mon, Aug 29, 2016 at 04:03:25PM -0700, Vincent Batts wrote:

endif

go vet is now included in >=go1.5, so no need to get it.

.install.govet:
-ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

go get golang.org/x/tools/cmd/vet
+ifneq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

But we're not testing against 1.4 in Travis.

Travis is not the only Makefile consumer. If it was, you wouldn't
need this check at all.

I'm not seeing where go1.4 is supported. What are you saying? that go1.4 ought to be allowed, but isn't? or that govet now support go1.4?

On Tue, Aug 30, 2016 at 10:08:31AM -0700, Vincent Batts wrote:

endif

go vet is now included in >=go1.5, so no need to get it.

.install.govet:
-ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

go get golang.org/x/tools/cmd/vet
+ifneq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)

I'm not seeing where go1.4 is supported. What are you saying? that
go1.4 ought to be allowed, but isn't? or that govet now support
go1.4?

With your change, the Makefile will no longer error out when the host
Go version is 1.4. But govet does not support Go 1.4, so we still
want to error out in that situation. Travis will not invoke the
Makefile with a host Go 1.4, but a non-Travis user/developer might, so
we still want to cover the Go 1.4 situation.

turns out that it's not needed. govet is not installable even with go1.4. I've removed the install line entirely

vbatts · 2016-08-31T17:30:02Z

PTAL

crosbymichael · 2016-08-31T17:32:36Z

LGTM

wking · 2016-08-31T18:01:31Z

Makefile

-# go vet is now included in >=go1.5, so no need to get it.
-.install.govet:
-ifeq ($(call ALLOWED_GO_VERSION,1.5,$(HOST_GOLANG_VERSION)),true)
-	go get golang.org/x/tools/cmd/vet


If you drop this, you'll also want to rewrite/remove the line in the .govet recipe with the:

ERROR: 'go vet' not found. Consider 'make install.tools' target

message.

Add and update golang versions. Also fix install.tools target for installing govet Signed-off-by: Vincent Batts <[email protected]>

vbatts · 2016-09-08T13:53:21Z

PTAL

wking · 2016-09-08T16:29:12Z

d4ede0d looks good to me.

crosbymichael · 2016-09-08T16:45:36Z

LGTM

tianon · 2016-09-08T20:43:13Z

LGTM

vbatts force-pushed the go_vet branch 2 times, most recently from b574036 to 300eb00 Compare August 29, 2016 14:07

vbatts mentioned this pull request Aug 29, 2016

Makefile: Use a POSIX-compatible test ('==' -> '=') #542

Closed

wking reviewed Aug 29, 2016
View reviewed changes

vbatts force-pushed the go_vet branch from 300eb00 to a24d961 Compare August 29, 2016 19:31

wking reviewed Aug 29, 2016
View reviewed changes

vbatts force-pushed the go_vet branch from a24d961 to 0f9d923 Compare August 31, 2016 17:18

wking reviewed Aug 31, 2016
View reviewed changes

ci-tools: versions of golang

d4ede0d

Add and update golang versions. Also fix install.tools target for installing govet Signed-off-by: Vincent Batts <[email protected]>

vbatts force-pushed the go_vet branch from 0f9d923 to d4ede0d Compare September 8, 2016 13:53

tianon merged commit 7a36e7e into opencontainers:master Sep 8, 2016

vbatts deleted the go_vet branch September 13, 2016 18:14

vbatts mentioned this pull request Dec 7, 2016

Version bump for v1.0.0-rc3 #642

Merged

ci-tools: versions of golang #547

ci-tools: versions of golang #547

Uh oh!

Conversation

vbatts commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

go vet is now included in >=go1.5, so no need to get it.

Uh oh!

Choose a reason for hiding this comment

go vet is now included in >=go1.5, so no need to get it.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

go vet is now included in >=go1.5, so no need to get it.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Aug 31, 2016

Uh oh!

crosbymichael commented Aug 31, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 8, 2016

Uh oh!

wking commented Sep 8, 2016 via email

Uh oh!

crosbymichael commented Sep 8, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon commented Sep 8, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crosbymichael commented Aug 31, 2016 •

edited by caniszczyk

Loading

crosbymichael commented Sep 8, 2016 •

edited by caniszczyk

Loading

tianon commented Sep 8, 2016 •

edited by caniszczyk

Loading