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

cmd/vet: +build comment error is confusingly worded #31410

Open
navytux opened this Issue Apr 11, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@navytux
Copy link
Contributor

commented Apr 11, 2019

Please answer these questions before submitting your issue. Thanks!

What did you do?

Hello up there. I was updating my tracing/xruntime package for Go1.12 and hit test error:

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go test
# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line
FAIL    lab.nexedi.com/kirr/go123/tracing/internal/xruntime [build failed]

The error here complains about +build directive in assembly file:

---- 8< ---- (runtime_g_amd64.s)

#include "textflag.h"

// +build amd64 amd64p

// func getg() *g
TEXT ·getg(SB),NOSPLIT,$0-8
	MOVQ (TLS), R14
	MOVQ R14, ret+0(FP)
	RET

(https://lab.nexedi.com/kirr/go123/blob/7ee2de42/tracing/internal/xruntime/runtime_g_amd64.s)

It was working with Go1.11 and previous releases.

What did you expect to see?

Build and test succeed; test pass, as with e.g. Go1.11:

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go1.11 test -v
=== RUN   TestStartStopTheWorld
--- PASS: TestStartStopTheWorld (1.00s)
PASS
ok      lab.nexedi.com/kirr/go123/tracing/internal/xruntime     1.003s

What did you see instead?

.../src/lab.nexedi.com/kirr/go123/tracing/internal/xruntime$ go1.12 test -v
# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line
FAIL    lab.nexedi.com/kirr/go123/tracing/internal/xruntime [build failed]

System details

go version go1.12.3 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kirr/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/kirr/go"
GOPROXY=""
GORACE=""
GOROOT="/home/kirr/src/tools/go/go"
GOTMPDIR=""
GOTOOLDIR="/home/kirr/src/tools/go/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.3 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.3
uname -sr: Linux 4.19.0-4-amd64
Distributor ID:	Debian
Description:	Debian GNU/Linux buster/sid
Release:	testing
Codename:	buster
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.28-8) stable release version 2.28.
gdb --version: GNU gdb (Debian 8.2.1-2) 8.2.1

navytux added a commit to navytux/go123 that referenced this issue Apr 11, 2019

tracing/runtime: Try to add support for Go1.12
Generate g for Go 1.12; no changes here compared to Go 1.11, however
go1.12 build build fails because of +build in .s file:

	# lab.nexedi.com/kirr/go123/tracing/internal/xruntime
	./runtime_g_amd64.s:3:1: +build comment must appear before package clause and be followed by a blank line

Build problem reported upstream:

	golang/go#31410
@navytux

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Thanks beforehand for looking into this,
Kirill

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@bcmills bcmills added this to the Go1.13 milestone Apr 11, 2019

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

The error could be improved (obviously there is no package clause), but it is correct to be complaining. The // +build line in this file has no effect. You should move it up above the #include.

A more appropriate error would be

./runtime_g_amd64.s:3:1: +build comment must appear before first non-comment source line

and the "blank line" should be dropped in this instance. When the blank line is relevant the message should be

./runtime_g_amd64.s:3:1: blank line must separate +build comment from first non-comment source line

@rsc rsc added the NeedsFix label Apr 12, 2019

@rsc rsc changed the title cmd/go: .s: +build comment must appear before package clause and be followed by a blank line (regression) cmd/vet: +build comment error is confusingly worded Apr 12, 2019

navytux added a commit to navytux/go123 that referenced this issue Apr 12, 2019

tracing/runtime: Fix build for Go1.12
This continues b46436b (tracing/runtime: Try to add support for Go1.12):

Move `+build ...` constraint in assembly files to the top, as suggested
by Russ Cox:

	golang/go#31410 (comment)

Then go stops complaining about it.

The build constraint had no effect previously, but it was not caught
because getg is used only on race builds and I was testing on amd64
only, which is practically almost the only ISA supported by race
detector as of Go1.11 .
@navytux

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@rsc thanks for feedback. I confirm that moving +build ... to the top fixed my package for Go1.12. Good idea to improve the error message.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@ianthehat, how do you want us to triage cmd/vet issues?

(@alandonovan is still listed as the primary owner on dev.golang.org/owners, but I'm guessing there might be someone else on your team stepping up for vet maintenance?)

CC @josharian @mvdan (secondary owners)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.