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

go/build: importing with UseAllFiles on Go 1.20 and 1.21 fails for build-constrained files that use Go 1.22 #cgo directives #63293

Closed
corhere opened this issue Sep 29, 2023 · 12 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@corhere
Copy link

corhere commented Sep 29, 2023

What version of Go are you using (go version)?

$ go version
go version go1.21.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/Users/corysnider/Library/Caches/go-build'
GOENV='/Users/corysnider/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Volumes/Workspace/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Volumes/Workspace'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/Cellar/go/1.21.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/Cellar/go/1.21.1/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/corysnider/tmp/cgovendorbug/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/8p/tzn5bkn967vfgttpr2d2kpjm0000gq/T/go-build1712739847=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

$ go mod init example.com/cgovendorbug
go: creating new go.mod: module example.com/cgovendorbug
$  go get github.com/golang-fips/openssl/v2
go: added github.com/golang-fips/openssl/v2 v2.0.0-rc.3
$ cat > main.go
package main

import _ "github.com/golang-fips/openssl/v2"

func main() {}
^D
$ go mod vendor

What did you expect to see?

No output; go mod vendor succeeds.

What did you see instead?

internal error: failed to find embedded files of github.com/golang-fips/openssl/v2: /Volumes/Workspace/pkg/mod/github.com/golang-fips/openssl/[email protected]/cgo_go122.go: invalid #cgo line: #cgo noescape go_openssl_EVP_PKEY_derive

The cgo_go122.go file has a //go:build go1.22 build constraint so the module is otherwise compatible with Go 1.21; it builds without error. The module just cannot be vendored.

corhere added a commit to corhere/golang-fips-openssl that referenced this issue Sep 29, 2023
The presence of Go 1.22 #cgo directives in any file prevents the module
from being vendored by Go 1.21 or below, even if the file has build
constraints. Revert the Go 1.22 directives for now so the module can be
vendored into projects using Go 1.21 or Go 1.20.
@qmuntal qmuntal added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 29, 2023
@mknyszek mknyszek added GoCommand cmd/go and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Oct 4, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 13, 2023

Huh, yep. This was probably introduced in https://go.dev/cl/283641, and is related to this TODO:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/modcmd/vendor.go;l=304-306;drc=33cdafed5213a17d23baa92568ede4bb16e7af0c

It looks like we're also missing a vendorE check to allow go mod vendor -e to push past the error. 🤦‍♂️

The right solution might be to always ignore errors from ctxt.ImportDir, although that's not entirely obvious to me.

@qmuntal
Copy link
Contributor

qmuntal commented Oct 24, 2023

Even if this issue is not new, given that it now will occur more due to the addition of the new #cgo directives in go1.22, would it make sense to mark this as release blocker?

@bcmills bcmills added this to the Go1.22 milestone Oct 25, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023

I think we should consider reworking the new #cgo directive so that it doesn't break the parsers in older releases. Marking as release-blocker to consider the options.

@bcmills bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 25, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023

@golang/compiler: what are your opinions on adjusting the directive syntax?

@dr2chase dr2chase added compiler/runtime Issues related to the Go compiler and/or runtime. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Oct 25, 2023
@cherrymui
Copy link
Member

@bcmills do you have a suggestion for a different syntax? Is that any #cgo directive that is not previously recognized would error out? That means we cannot use #cgo directive for this?

@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023

Yes, it looks that way. The Go 1.21 parser implementation is here:
https://cs.opensource.google/go/go/+/release-branch.go1.21:src/go/build/build.go;l=1674-1751;drc=d22f287f12f4782082cd93785ad91e78dbe0d4d6

Notably, it has a default: return fmt.Errorf(…) at the end of each line.

To me, that suggests that maybe we could use a //go:-style directive instead?

@mknyszek
Copy link
Contributor

In compiler/runtime triage, we're wondering if it's an option to update older supported releases to accept the new cgo directive?

@mknyszek mknyszek moved this to In Progress in Go Compiler / Runtime Oct 25, 2023
@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023

That may be an option, yes. It would involve at least backporting portions of https://go.dev/cl/497837 in the go/build and cmd/go/internal/modindex packages.

@cherrymui
Copy link
Member

Another option would be have the go command to not error for directives in files with a build tag of newer version of Go. This would also help if in the future we add other cgo directives.

Would syntax like #cgo go1.22 noescape help? I guess the go command is already capable to understand the go:build build tags, so maybe this is not really necessary?

@bcmills
Copy link
Contributor

bcmills commented Oct 25, 2023

Another option would be have the go command to not error for directives in files with a build tag of newer version of Go.

Yep, we should probably do that but it's a somewhat more involved change, and of course it only helps once a fix is backported and released and users upgrade to it. 🙃

Would syntax like #cgo go1.22 noescape help?

Unfortunately no: any program that uses go/build to parse files containing #cgo directives will end up rejecting any #cgo directive that doesn't match one of the existing tags.

(Perhaps for the purpose of future-proofing we should make go/build more robust to new #cgo directives, though.)

@bcmills bcmills changed the title cmd/go: vendoring fails if Go 1.22 #cgo directive is present in dependent module go/build: importing with UseAllFiles on Go 1.20 and 1.21 fails for build-constrained files that use Go 1.22 #cgo directives Nov 1, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 1, 2023

@corhere, for the moment I suggest that you file an issue against https://github.com/golang-fips/openssl to roll back the use of these new directives.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539235 mentions this issue: cmd/cgo: disable #cgo noescape/nocallback until Go 1.23

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Nov 2, 2023
@dmitshur dmitshur removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 3, 2023
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 3, 2023
@golang golang locked and limited conversation to collaborators Nov 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

8 participants