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/go: go:generate causes unnecessary packages in std & binary releases #31920

Closed
bradfitz opened this issue May 8, 2019 · 10 comments

Comments

Projects
None yet
4 participants
@bradfitz
Copy link
Member

commented May 8, 2019

I notice that on non-Windows platforms, the Windows-only internal/syscall/windows/registry gets listed in go list std and gets shipped in binary releases:

$ find ~/go1.12/ | grep registry.a$
/home/bradfitz/go1.12/pkg/linux_amd64/internal/syscall/windows/registry.a
/home/bradfitz/go1.12/pkg/linux_amd64_race/internal/syscall/windows/registry.a
$ go list std | grep registry
internal/syscall/windows/registry

Likewise with internal/syscall/windows.

That's all because of the packages having a single go file that's not behind +build windows:

$ cat mksyscall.go 
// Copyright 2016 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package registry

//go:generate go run $GOROOT/src/syscall/mksyscall_windows.go -output zsyscall_windows.go syscall.go

No code. Just a "go generate" line.

If we made that file +build windows, then we couldn't run "go generate" on Linux. Is that useful? Probably.

Perhaps we could make it +build generate and have go generate add the generate build tag?

/cc @robpike @alexbrainman @ianlancetaylor @josharian @rsc @bcmills @dmitshur

@gopherbot

This comment has been minimized.

Copy link

commented May 8, 2019

Change https://golang.org/cl/176019 mentions this issue: internal/windows/sysdll: mark package as Windows-only

@dmitshur

This comment has been minimized.

Copy link
Member

commented May 8, 2019

The go generate command accepts the -tags flag. One can also do GOOS=windows go generate.

If we made that file +build windows, then we couldn't run "go generate" on Linux.

To clarify, did you mean it's not possible to go generate on Linux without any flags/configuration? Otherwise, it is possible, either go generate -tags=windows or GOOS=windows go generate would work.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

To clarify, did you mean it's not possible to go generate on Linux without any flags/configuration?
Otherwise, it is possible, either go generate -tags=windows or GOOS=windows go generate would work.

If the person has to think about or research which flags to go generate to use then there's no point to using go generate. The whole point of it was to have a consistent way for developers to update auto-generated files. Making users write go generate -tags=something is no better than the old situation of go run -something updatefoo.go or ./mkall.sh.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Wait, from the design doc,

https://golang.org/s/go1.4-generate

The go generate tool also sets the build tag "generate" so that files may be examined by go generate but ignored during build.

So that was the plan!

But it wasn't implemented it seems:

bradfitz@go:~/go/src/internal/syscall/windows$ git di
diff --git a/src/internal/syscall/windows/mksyscall.go b/src/internal/syscall/windows/mksyscall.go
index a8edafb3c3..0bf87dc95c 100644
--- a/src/internal/syscall/windows/mksyscall.go
+++ b/src/internal/syscall/windows/mksyscall.go
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
+// +build generate
+
 package windows
 
 //go:generate go run $GOROOT/src/syscall/mksyscall_windows.go -output zsyscall_windows.go syscall_windows.go security_windows.go psapi_windows.go symlink_windows.go
bradfitz@go:~/go/src/internal/syscall/windows$ go generate
can't load package: package internal/syscall/windows: build constraints exclude all Go files in /home/bradfitz/go/src/internal/syscall/windows
bradfitz@go:~/go/src/internal/syscall/windows$ go generate --tags=generate
bradfitz@go:~/go/src/internal/syscall/windows$ 
@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I looked at Go 1.4 and Go 1.5 and neither supported adding the "generate" build tag for file selection, so it's not like we accidentally deleted it at some point. I guess it was just never done.

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@gopherbot

This comment has been minimized.

Copy link

commented May 8, 2019

Change https://golang.org/cl/175984 mentions this issue: internal/syscall/windows{,/registry}: mark packages as Windows-only

@gopherbot

This comment has been minimized.

Copy link

commented May 8, 2019

Change https://golang.org/cl/175983 mentions this issue: cmd/go: set the "generate" build tag in go generate, per design doc

gopherbot pushed a commit that referenced this issue May 8, 2019

internal/syscall/windows/sysdll: mark package as Windows-only
Updates #31920

Change-Id: Ie24ed5bab249e2f90d1740f42a8b8d94fd0983f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/176019
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>

@gopherbot gopherbot closed this in 41df5ae May 8, 2019

@gopherbot

This comment has been minimized.

Copy link

commented May 9, 2019

Change https://golang.org/cl/175899 mentions this issue: windows: add "generate" build tag

@myitcv

This comment has been minimized.

Copy link
Member

commented May 9, 2019

If the person has to think about or research which flags to go generate to use then there's no point to using go generate.

I'm not sure this is true in general; some generators rely on build tags because they rely on type information. Hence they rely on the values of GOOS, GOARCH and -tags. See also #27898

We're wrestling with this problem in golang-tools world; not only for go generate, but also go vet, gopls, and other tools (e.g. staticcheck) where the "matrix" of valid builds needs to be known.

In the case of gopls (and editors), the user might also need to specify (from their editor) which build they are interested in.

Considering go vet, staticcheck et al, in a CI context they might need/want to iterate over the "matrix" of valid builds.

gopherbot pushed a commit to golang/sys that referenced this issue May 9, 2019

windows: add "generate" build tag
cmd/go supports the "generate" build tag as of CL 175983. Add it to the
files which are just used for generating errors and syscall wrappers.

Also see golang/go#31920

Change-Id: Ib26c90af0ac1fb7bae81366a46dedf028b787566
Reviewed-on: https://go-review.googlesource.com/c/sys/+/175899
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

shuLhan added a commit to shuLhan/ciigo that referenced this issue May 19, 2019

all: fix the main generate directive to use "+build ignore"
The latest change on Go tip [1] somehow make the "go generate" with
"+build generate" on main package cause an error,

  can't load package: package github.com/shuLhan/ciigo: found packages ciigo (ciigo.go) and main (generate_main.go) in /home/ms/go/src/github.com/shuLhan/ciigo

From the source code of Go itself, seems like the correct directive
is "+build ignore" instead of "+build generate".

[1] golang/go#31920
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.