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: inconsistent behavior when 'go' directive is missing from 'go.mod' #44976

Closed
bcmills opened this issue Mar 12, 2021 · 12 comments
Closed

cmd/go: inconsistent behavior when 'go' directive is missing from 'go.mod' #44976

bcmills opened this issue Mar 12, 2021 · 12 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 12, 2021

During work on CL 293689, I noticed some issues with the behavior of cmd/go when a go.mod file is present but contains no go directive (which cmd/go adds by default starting as of Go 1.12).

The go directive controls two things: the version of the Go language spec that applies for source code compiled within that module, and the assumptions that cmd/go makes about the contents and consistency of go.mod and vendor/modules.txt:

  • As of go 1.14, -mod=vendor is set by default when vendor/modules.txt is present. (In prior versions, it always had to be enabled explicitly.)
  • As of go 1.16, the all pattern under -mod=mod and -mod=readonly matches the same packages as under -mod=vendor. (In prior versions, the all pattern matched additional test dependencies in mod and readonly modes.)
  • Planned for go 1.17, cmd/go will assume that the go.mod file satisfies additional invariants, and will load dependencies lazily instead of eagerly (#36460).

When the go directive is missing from the main module's go.mod file, cmd/go currently adds a go directive for the latest Go version it supports. It does this regardles of the -mod setting, but only writes the go directive back to the go.mod file if -mod=mod is set. As of Go 1.16, -mod=mod is no longer the default behavior.

This leads to two problems:

  1. The implicitly-added go command triggers assumptions about the current state of the module that are not necessarily valid, especially for read-only modules found within the module cache. That can cause builds to spuriously fail (such as if the program uses a language feature that was removed in a subsequent version of the language, or if the go command assumes lazy-loading semantics for a Go 1.11 module).

  2. The behavior of builds with -mod=readonly is not reproducible. The dependencies in use may vary with changes in the behavior of the module loader from one version to the next, and packages that build successfully in the main module today may fail to build (without any changes in their source code) if features are removed from a future version of the language.

In addition, as implemented this behavior contains a (fairly serious) bug: the newly-added go version takes effect immediately for the compiler (controlling language versions), but does not take effect in the module loader until the go.mod file is written back to disk.


I can see two possible fixes:

  1. We could change cmd/go to only add the go directive if it intends to write the updated go.mod file back to disk, so that a read-only main module is treated the same as a read-only dependency module.

    • This would restore reproducibility by always either using the previous semantics of the module (-mod=readonly and -mod=vendor), or explicitly modifying the go.mod file.
    • It would also continue to pull modules toward the current semantics.
      • Notably, tools that write out their own one-line go.mod file instead of using go mod init would continue to jump to the latest version.
    • However, it would cause some invocations that succeed under -mod=readonly to suddenly have very different semantics (and possibly verbose failures) when repeated with -mod=mod.
  2. We could change cmd/go to assume go 1.11 (instead of the current language version) when a go directive is missing, regardless of the -mod flag.

    • This would restore reproducibility by always using the previous semantics of the module, full stop.
    • It would also produce the same behavior under -mod=mod as under -mod=readonly and -mod=vendor.
    • However, it would cause some modules initialized by means other than go mod init to stick at Go 1.11 semantics, instead of incorporating improvements to the language and module-mode semantics made since that time.
@bcmills bcmills added this to the Go1.17 milestone Mar 12, 2021
@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 12, 2021

@bcmills bcmills added the modules label Mar 12, 2021
@bcmills bcmills self-assigned this Mar 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 15, 2021

Change https://golang.org/cl/302051 mentions this issue: cmd/go: only add a 'go' directive to the main module when the go.mod file will be written

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 15, 2021

CL 302051 implements option (1), which makes the tests coherent enough to unblock CL 293689 (#36460).

That said, I think I still prefer option (2).

@bcmills bcmills added the Unfortunate label Mar 16, 2021
@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 16, 2021

Another alternative might be either of the above but defaulting to go 1.16 instead of go 1.11, since go 1.16 is the behavior as of the current release and its semantics are not as different from the preceding versions as lazy loading will be.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Mar 17, 2021

Option 2 seems better to me. I'd expect this to mostly affect a) modules that were converted with Go 1.11 then never upgraded, and b) projects that were never migrated to modules and don't have a real go.mod file.

For these projects, it's probably better to default to something older, particularly if the go directive is used to control backward-incompatible language changes later on.

gopherbot pushed a commit that referenced this issue Mar 17, 2021
…file will be written

Then, write the 'go.mod' file with that version before further
processing. That way, if the command errors out due to a change in
behavior, the reason for the change in behavior will be visible in the
file diffs.

If the 'go.mod' file cannot be written (due to -mod=readonly or
-mod=vendor), assume Go 1.11 instead of the current Go release.
(cmd/go has added 'go' directives automatically, including in 'go mod
init', since Go 1.12.)

For #44976

Change-Id: If9d4af557366f134f40ce4c5638688ba3bab8380
Reviewed-on: https://go-review.googlesource.com/c/go/+/302051
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 19, 2021

@jayconrod points out that packages authored today in GOPATH mode (without a go.mod file) may already be using Go 1.16 features, so we can't necessarily assume Go 1.11 semantics for dependencies.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 19, 2021

#45109 brings up another interesting complication: -mod=vendor triggers automatically as of Go 1.14, but that version was also the most recent addition to the language spec, so we can't actually assume coherent semantics if the main module does not specify a Go version.

If we assume go 1.14 or higher for the main module, then -mod=vendor will trigger as the default if the main module was written for Go 1.11 (which does not satisfy the 1.14 auto-vendor invariants — namely, it does not support version-consistency checks).

However, if we assume a version lower than go 1.14 for vendored modules, then vendored packages written against Go 1.14 and vendored in using Go 1.14–1.16 will spuriously fail to build.

I see a few possible solutions. All are unfortunate.

  1. If any module lacks a go directive, assume go 1.16 language semantics but go 1.11 module semantics.

  2. A go.mod file must exist for the main module, so it can't have been written assuming GOPATH mode. If the main module lacks a go directive, assume Go 1.11 semantics. For all other modules, assume Go 1.16.

  3. When we load a package from a module dependency, we can actually check to see whether that module contains an explicit go.mod file. If It does, we could assume Go 1.11 module-mode semantics; otherwise, we could assume Go 1.16 GOPATH-mode semantics. (But we would still need to assume Go 1.16 for all modules in vendor mode, at least if the main module is not at least go 1.17 assuming that we fix #36876 for Go 1.17.)

Of these options, I think (2) may be the least-bad.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 19, 2021

Change https://golang.org/cl/303229 mentions this issue: cmd/go: assume Go 1.16 instead of Go 1.11 for dependencies that lack explicit 'go' directives

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 19, 2021

If -mod=mod is set and the main module is missing a go directive, perhaps it would be clearer to error out explicitly instead of automatically writing a version that may be much too old (1.11) or so new as to accidentally change the dependency graph (1.17).

Or, at least, we could error out if the require list is non-empty, since hand-edited go.mod files that also lack go directives are likely to lack require directives too, and the require directives are where the semantic differences actually matter.

go mod tidy in particular does not need to error out either way: it has enough information to compute the full set of relevant requirements for a go 1.17 module. go mod tidy can instead load the original requirements using Go 1.11 semantics. then recompute the correct (lazy-compatible) expanded requirements and write them out along with an up-to-date go directive. (See #45094 for a more general form of this idea.)

gopherbot pushed a commit that referenced this issue Mar 19, 2021
…explicit 'go' directives

Fixes #45109
Updates #44976
Updates #36876

Change-Id: Icb00f8b6e0d4e076d82da1697e7058b9e7603916
Reviewed-on: https://go-review.googlesource.com/c/go/+/303229
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@toothrot
Copy link
Contributor

@toothrot toothrot commented Apr 1, 2021

@bcmills Any update on this issue? Just checking in as it is labeled as a release-blocker.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 6, 2021

It's mostly fixed at head already, although we may need to do some fine-tuning before the final release.

We discussed this issue today in a meeting with @rsc, @matloob, @jayconrod, and me, and decided that the differences in behavior from 1.11 to 1.16 are small enough that we may as well assume 1.16 uniformly when either the go.mod file is missing for a dependency or it is missing a go directive.

I'm going to try to implement #45094 as part of lazy loading (in order to provide a migration path), and as part of that we can also evaluate whether go mod tidy can pull the go version up to latest, and perhaps whether other go commands can do the same if the go.mod file has no existing requirements (by far the common case for go.mod files that lack versions).

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 8, 2021

Change https://golang.org/cl/308509 mentions this issue: cmd/go: assume Go 1.16 semantics uniformly for unversioned modules

@gopherbot gopherbot closed this in 0e09e41 Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants