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

embed: go:embed produces error when "go.mod" has go version < 1.16 #43980

Closed
lestrrat opened this issue Jan 29, 2021 · 32 comments
Closed

embed: go:embed produces error when "go.mod" has go version < 1.16 #43980

lestrrat opened this issue Jan 29, 2021 · 32 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@lestrrat
Copy link

lestrrat commented Jan 29, 2021

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

$ go version
go version go1.16rc1 darwin/arm64

Does this issue reproduce with the latest release?

with the rc release? yes.

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

go env Output
$ go env
go version go1.16rc1 darwin/arm64
lestrrat@finch jwk % go env
GO111MODULE="on"
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/lestrrat/Library/Caches/go-build"
GOENV="/Users/lestrrat/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/lestrrat/dev/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/lestrrat/dev"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.16rc1"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/lestrrat/dev/src/github.com/lestrrat-go/jwx/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vb/gf1mcqdd2s509731hrnnvkjm0000gn/T/go-build3140323037=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Using github.com/lestrrat-go/jwx@09b6dea (note that it's at a tracking branch)

lestrrat@finch jwx % cd jwk
lestrrat@finch jwk % go test               

What did you expect to see?

lestrrat@finch jwx % cd jwk
lestrrat@finch jwk % go test   
PASS
ok  	github.com/lestrrat-go/jwx/jwk	5.437s
lestrrat@finch jwk % 

What did you see instead?

lestrrat@finch jwx % cd jwk
lestrrat@finch jwk % go test               
# github.com/lestrrat-go/jwx/jwk_test [github.com/lestrrat-go/jwx/jwk.test]
./fs_test.go:13:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
FAIL	github.com/lestrrat-go/jwx/jwk [build failed]
lestrrat@finch jwk % 

Discussion

My go.mod contains the following

lestrrat@finch jwk % cat ../go.mod
module github.com/lestrrat-go/jwx

go 1.13

require (
	github.com/goccy/go-json v0.3.4
	github.com/lestrrat-go/backoff/v2 v2.0.7
	github.com/lestrrat-go/codegen v1.0.0
	github.com/lestrrat-go/httpcc v1.0.0
	github.com/lestrrat-go/iter v1.0.0
	github.com/lestrrat-go/option v1.0.0
	github.com/lestrrat-go/pdebug/v3 v3.0.1
	github.com/pkg/errors v0.9.1
	github.com/stretchr/testify v1.6.1
	golang.org/dl v0.0.0-20210128152142-7744cb1878f1 // indirect
	golang.org/x/crypto v0.0.0-20201217014255-9d1352758620
	golang.org/x/mod v0.4.1 // indirect
	golang.org/x/tools v0.0.0-20210114065538-d78b04bdf963
)

If I change the line go 1.13 to go 1.16, then it works on go1.16rc1

lestrrat@finch jwk % go test
# github.com/lestrrat-go/jwx/jwk_test [github.com/lestrrat-go/jwx/jwk.test]
./fs_test.go:13:3: go:embed requires go1.16 or later (-lang was set to go1.13; check go.mod)
FAIL	github.com/lestrrat-go/jwx/jwk [build failed]
lestrrat@finch jwk % go mod edit -go 1.16
lestrrat@finch jwk % go test             
PASS
ok  	github.com/lestrrat-go/jwx/jwk	5.648s
lestrrat@finch jwk % 

go1.16beta1 worked just fine.

lestrrat@finch jwk % go install golang.org/dl/go1.16beta1
lestrrat@finch jwk % go1.16beta1 download
go1.16beta1: already downloaded in /Users/lestrrat/sdk/go1.16beta1
lestrrat@finch jwk % go1.16beta1 test .
ok  	github.com/lestrrat-go/jwx/jwk	6.066s
lestrrat@finch jwk % 

Shouldn't features like this just depend on the runtime version, and not contents of go.mod?

@ianlancetaylor
Copy link
Contributor

This is intentional, because a //go:embed comment will only work correctly with Go 1.16 or later. A //go:embed comment requires importing the "embed" package, which is only available in Go 1.16. So compiling a package with an earlier language version will fail.

@lestrrat
Copy link
Author

Even with "// +build go1.16" protecting it? -- just making sure, because while I understand go:embed not working with go < 1.16, I would think that if the source code was not eligible for compilation via build tags, it should be ignored.

And if so, I would appreciate a doc explaining that go will not consider build tags in that case. My skimming through the release notes or the embed docs didn't produce such explanation. I can certainly provide a patch if need be.

@go101
Copy link

go101 commented Jan 29, 2021

By my understanding,

  • "// +build go1.16" specify the exact minimum Go Toolchain version needed to compile a source file.
  • "go 1.N" directive in a "go.mod" file specifies the features the containing module could use (https://golang.org/ref/mod#go-mod-file-go). It likes the "-lang=go1.N" compilation option, but for the source files in a specified module. So
    • it doesn't specify the minimum Go Toolchain version to compile the module. Higher or lower Toolchain versions may be used if the desired features are supported these Toolchain versions.
    • it doesn't cover other modules in a program. https://play.golang.org/p/YlMYLWsP_0S

lestrrat added a commit to lestrrat-go/jwx that referenced this issue Jan 29, 2021
golang/go#43980

go1.16 probably won't allow us to use go:embed, so just implement
an in memory FS
@lestrrat
Copy link
Author

I have learned that go mod tidy on 1.15 which uses io/fs, regardless of build tags, will fail. Fair enough! Closing.

@mvdan
Copy link
Member

mvdan commented Jan 29, 2021

This is intentional, because a //go:embed comment will only work correctly with Go 1.16 or later. A //go:embed comment requires importing the "embed" package, which is only available in Go 1.16. So compiling a package with an earlier language version will fail.

@ianlancetaylor while I get the current check against -lang, I'm not sure I understand why it was done this way. If a module declares go 1.16 in its go.mod, and then only imports the "embed" package in a Go file guarded by // +build go1.16, why should that fail?

@ianlancetaylor
Copy link
Contributor

@mvdan Fair point. I'll reopen.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2021
@ianlancetaylor ianlancetaylor added this to the Go1.16 milestone Jan 29, 2021
@mvdan
Copy link
Member

mvdan commented Jan 29, 2021

I made a typo, by the way. I meant that the go.mod declares an older go language version, like go 1.15.

@ianlancetaylor
Copy link
Contributor

Thanks, understood.

@zikaeroh
Copy link
Contributor

zikaeroh commented Jan 29, 2021

I have learned that go mod tidy on 1.15 which uses io/fs, regardless of build tags, will fail. Fair enough! Closing.

For reference, that's #40067, and it even affects users of modules that try and use new standard library packages.

No module can successfully guard its use of a new standard library package with a build tag and have it not still break downstream users with older Go versions. (I expect this to start happening pretty often now that everyone is excited for embed, unlike when maphash was added.)

@amnonbc
Copy link

amnonbc commented Jan 30, 2021

Having spent little time playing around with //go:embed and encountering the error myself,
I would say that error message is simple, clear, and reasonable.

If you want to use a language feature introduced in Go 1.16, you will need tools >= 1.16
and version 1.16 or greater specified in your go.mod.
I may be missing something, but I can't think of any scenario where someone would want to add
embedding while keeping the Go version specified as 1.15 or less.

@lestrrat
Copy link
Author

lestrrat commented Jan 31, 2021

The confusion is in the fact that we (well, at least I) have used build tags to switch between certain features that were newly introduced. For example, I have the following successfully compiling in go1.16rc1

// +build go1.15

package ecutil

import (
  "math/big"
)

func bigIntFillBytes(v *big.Int, buf []byte) []byte {
  v.FillBytes(buf)
  return buf
}
// +build !go1.15

package ecutil

import (
  "math/big"
)

func bigIntFillBytes(v *big.Int, buf []byte) []byte {
  data := v.Bytes()
  if len(data) > len(buf) {
    panic("ecutil: invalid call to bigIntFillBytes (len(data) > len(buf))")
  }

  copy(buf[len(buf)-len(data):], data)
  return buf
}
// go.mod
module ...
go 1.13 // FillBytes doesn't exist in 1.13, let alone 1.14

In this scenario, I was able to provide support for go 1.14 that doesn't have (math/big).FillBytes() while utilizing the new feature that only appeared in go 1.15

At first glance I fail to see the difference between the above and below

// +build go1.16
import "embed"
// +build !go1.16
// (code without embed)
// go.mod file
module ...
go 1.15 // kaput

(math/big).Int.FillBytes doesn't exist in go 1.14, why should it compile, when import "embed" doesn't?
I still think this is inconsistent, but I closed this issue because I realized that this was not something new in go1.16 (and hence unlikely to be changed).

Hope that clears up my reasoning on this issue, at least.

@go101
Copy link

go101 commented Jan 31, 2021

I guess the difference between the two is: generally new added APIs are not features. I do admit sometimes this is a little confusing.

I guess there are several feature switches in the compiler code to control on/off of some admitted features.

@lestrrat
Copy link
Author

@go101 I can appreciate the distinction, but I would argue that to the layman the question is just "does this code run on go 1.xx?". In that sense, differentiating between API changes or language features or toolchain changes don't really mean anything to me. As a library writer, I tried to give maximum flexibility to my users by adding a feature to pass in a (io/fs).FS to specify where to read files from, guarded by // +build go1.16. But that doesn't compile while I keep go 1.15 in go.mod, I'm just not going to provide this functionality :/ oh well.

To the maintainers: while I maintain that the current state is inconsistent and confusing, I can appreciate the history, the decisions that went behind, etc, so I'm not demanding immediate change or anything. Again, I think it's confusing, so I would hope that relevant parties see that there is some room to either clarify the documentation or think this "feature" through a bit more now or in the future.

@amnonbc
Copy link

amnonbc commented Jan 31, 2021

As a developer I often rely on the compiler to tell me when I have broken the code.
When I change the signature of a function for instance, the compiler will tell me if I have
neglected to fix any call sites.

The fact that it will fail on errors on files guarded by build tags
is a good thing. I develop on an intel machine. If my change breaks the build on s390 machines,
the compiler will tell me. If it didn't I might end up shipping code which failed to compile
on my first s390 user.

Of course people will tell me I should make sure that my CI system builds and test on every possible
permutation of supported architectures, OSes, compiler versions etc. And they may be right.
But it is far better that compiler tells me about these problems straight away.

If this issue was "fixed" by allowing the code above to compile without error, it would make
development and testing far harder. Please don't do it.

To @lestrrat, my suggestion would be: if you really have to support Go versions < 1.16, then don't
use embed. In general I would aggressively kill off support for old Go versions, and only use language features
and APIs available on the oldest Go version you do support.

@go101
Copy link

go101 commented Jan 31, 2021

It would be great if there is an official document to list the features each version supports (and to explain features and functionalities are different).

@amnonbc
Copy link

amnonbc commented Jan 31, 2021

It would be great if there is an official document to list the features each version supports (and to explain features and functionalities are different).

https://tip.golang.org/doc/go1.16 explains which new features were added to go1.16 which are not in go1.15.
Once 1.16 is released, anything before 1.15 becomes unsupported.

If we try to "help" our users by maintaining a table mapping features to versions,
this will in effect be supporting unsupported versions of Go and telling people that it is OK to use them.
This effort will be far better spent encouraging the community to upgrade to the current tools.

@go101
Copy link

go101 commented Jan 31, 2021

@amnonbc
Go 1.16 release notes might explains what features were added togo1.16, but it doesn't differentiate what are features and what are not.

@lestrrat
I some misunderstood your above comments. It looks gc1.16rc1 views the embedding functionality (the //go:embed directive more specifically) as features, whywhich is not a good idea by my opinion. I think it is just a functionality, not a feature, for the //go:embed directive always requires import "embed".

I tried to give maximum flexibility to my users by adding a feature to pass in a (io/fs).FS to specify where to read files from, guarded by // +build go1.16. But that doesn't compile while I keep go 1.15 in go.mod, ...

It looks this is not true. gc1.16rc1 doesn't view (io/fs).FS as a feature.

@go101
Copy link

go101 commented Jan 31, 2021

On the other hand, it is really worth considering how to handle the case if a new Go version both introduces and removes features.

@amnonbc
Copy link

amnonbc commented Jan 31, 2021

The point I was trying to make earlier is that we are doing our users no favours if we try to "help" them continue to use
old versions of the tools.

The Go 1.0 promise guarantees the features don't get removed. But they will get added. And the best answer is
to upgrade to the current version of the compiler.

@go101
Copy link

go101 commented Feb 1, 2021

There might be features to be removed later, such as the string(neither_rune_nor_byte_integer) mentioned in https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md

@campoy
Copy link
Contributor

campoy commented Feb 9, 2021

I would like to point out that I ended up facing the same issue and created a gist reproducing it.

Then I realized this issue existed, but just in case the gist might be useful here it goes:

https://gist.github.com/campoy/f85824233233b8e25e53605087d4e871

@zikaeroh
Copy link
Contributor

zikaeroh commented Feb 9, 2021

@campoy That's #40067 and it's existed for a while (first seen when hash/maphash was added, see #43980 (comment)); I think the original issue is mainly about the fact that the Go compiler errors on //go:embed when the lang version is too low in go.mod, even when the code would be guarded by build tags (which IMO is the right way to guard the use, had it not been for #40067).

I'll also point out that the go.mod protection that was added really only makes sense for developers using //go:embed, but doesn't protect users in older compilers from potentially breaking their code, since modules with lower lang versions are allowed to require ones with newer lang versions (and embed.String and embed.Bytes were not added to force a compiler error via missing package), so I'm a bit dubious if this check will really end up having the intended effect to notify users that their code may silently break.

@rsc
Copy link
Contributor

rsc commented Feb 10, 2021

The expectation is that you don't use embed until you are ready to bump your users forward.
Build tags are not sufficient, precisely because of tools that need to analyze everything, such as go mod tidy.
The same thing happens (or will happen) with any language change gated by the go.mod version.

@rsc rsc closed this as completed Feb 10, 2021
@vearutop
Copy link
Contributor

vearutop commented Mar 15, 2021

Introducing embed dependency in an existing library (module) seems to effectively mean issuing a major version. Because isolating embed in a new package guarded with build tags is not possible due to this issue. And bumping required minimal Go version is a breaking change.

Am I missing a way to introduce embed dependency in a backwards compatible way?

I tried to use an app with go 1.16 in its go.mod (a user bumped forward) together with a go 1.11 module having embed-dependent package protected with build tags.

@amnonbc
Copy link

amnonbc commented Mar 15, 2021

I would not consider requiring the latest tools to build the code to be a breaking API change.

If the API stays the same, then the calling code does not need to change, so the major version should not be incremented.

New code using new features will generally fail to build using old tools.
The answer is for people to upgrade their tools to the current version.
They will anyway soon have no choice as 1.15 will be unsupported within a year.

So I would not try to protect new features with build tags. That way lies madness.

@vearutop
Copy link
Contributor

Funny enough, I can "workaround" the issue by removing go 1.11 statement from library's go.mod. :)

@amnonbc
Copy link

amnonbc commented Mar 15, 2021

Funny enough, I can "workaround" the issue by removing go 1.11 statement from library's go.mod. :)

Yes having no go VERSION statement is logically equivalent to having a go LATEST statement
where LATEST is the actual version of the compiler used to compile the code.

@rogpeppe
Copy link
Contributor

For the record, you can work around this issue by declaring go 1.16 in your go.mod file and using build tags to guard the use of go:embed. For example:

// +build go1.16

package m

import (
	_ "embed"
	"fmt"
)

//go:embed hello.txt
var s string

@amnonbc
Copy link

amnonbc commented Apr 14, 2021

For the record, you can work around this issue by declaring go 1.16 in your go.mod file and using build tags to guard the use of go:embed. For example:

// +build go1.16

package m

import (
	_ "embed"
	"fmt"
)

//go:embed hello.txt
var s string

Why do you need the build tag?

@rogpeppe
Copy link
Contributor

Why do you need the build tag?

Because the embed functionality and package is only available under 1.16 and later. You might want to add functionality that's available under later Go versions without breaking existing clients that are using earlier versions. The build tag allows you to do that.

@philippgille
Copy link

philippgille commented May 5, 2021

@rogpeppe Was that changed in a recent 1.15.x version? Because in the other comments in this thread it's mentioned that build tags don't help in this particular case, which is one issue that the OP also mentions.

Using the code from Francesc's Gist I can reproduce the issue with Go 1.15.7, but not with Go 1.15.11. I'm interested to know whether the build tag solution now properly works for embed or if there's another reason why the error doesn't occur in 1.15.11.

@rogpeppe
Copy link
Contributor

rogpeppe commented May 6, 2021

I believe that the build tag solution now works correctly with 1.15.11 - go mod tidy is now resilient to stdlib imports which don't exist.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

13 participants