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: vendored dependencies lack language version annotations #36876

Closed
bcmills opened this issue Jan 29, 2020 · 19 comments
Closed

cmd/go: vendored dependencies lack language version annotations #36876

bcmills opened this issue Jan 29, 2020 · 19 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2020

In writing #36875, I realized that we will have a bit of a problem with vendored dependencies if and when we start making incompatible changes to the language.

Specifically, the vendor directory is a flattened package tree, and it does not include go.mod files unless the vendored package dependencies happen to include a package at the root of the vendored module. Many modules (such as golang.org/x/tools) don't even have an importable package at the root, let alone one that someone would want to import.

Unfortunately, the go.mod files are currently the only place where we record the language version to use for those dependencies. (I don't know what language version we end up using for -mod=vendor builds today, but I don't see how it could possibly be the correct one in general.)

I can think of two possible fixes:

  1. Extend the ## annotation comments that we added for cmd/go: automatically check and use vendored packages #33848 so that, for any module that provides one or more packages, we annotate the go version (if any) found in the dependency's original go.mod file. (Fortunately, we intentionally designed that part of the format to tolerate the addition of new annotations.)
    For example:

    # golang.org/x/text v0.3.3
    ## explicit, go 1.14
    golang.org/x/text/number
    
  2. Explicitly copy in the go.mod files for each module that provides one or more packages, even if there is no corresponding package in that part of the vendor tree.

I have a slight preference for approach (1) but don't feel strongly about it.

(CC @ianlancetaylor @griesemer @jayconrod @matloob @mvdan)

@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker modules labels Jan 29, 2020
@bcmills bcmills added this to the Go1.15 milestone Jan 29, 2020
@mvdan
Copy link
Member

mvdan commented Jan 29, 2020

If anyone upgrades to a new Go version, and updates their go.mod to use that newer language version which happens to remove a language feature, could the build break until the author re-runs go mod vendor? If that's planned, we should make sure to mention it well in the changelog.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 29, 2020

@mvdan, ideally, a user changing the language version in their own go.mod file should not affect the language version used to compile their dependencies (regardless of vendoring), and therefore should not require any update to their vendor tree.

@mvdan
Copy link
Member

mvdan commented Jan 29, 2020

Hm, maybe I misunderstood. I thought re-running go mod vendor with Go 1.15 could add a bunch of those extra ## lines, necessary to add the missing language version information for vendored dependencies.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 29, 2020

Re-running go mod vendor with Go 1.15 presumably would add some ## lines, but I don't think that should be conditioned on the go version declared in the main module's go.mod file.

To minimize churn in vendor/modules.txt, we could omit the version annotations for any module that specifies a version at least as old as the default (#36875). As long as those dependencies built with Go 1.14 (or whatever default version we choose), they should continue to build without re-vendoring.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 29, 2020

(The go version annotations probably do not need a consistency check like the one added in #33848, because they currently have no effect on the computed module dependency graph.)

@thepudds
Copy link
Contributor

thepudds commented Jan 29, 2020

@bcmills

  1. Explicitly copy in the go.mod files for each module that provides one or more packages, even if there is no corresponding package in that part of the vendor tree.

FWIW, I would suggest thinking a bit more about the possible virtues of your approach 2., including as things evolve in the future (e.g., perhaps another bit of information might get added to go.mod in the future).

Also, some small-ish chance that storing the current go.mod files might be a stepping stone towards storing old go.mod files as well, which might end up enabling more cmd/go functionality when in vendor mode (e.g., #30240 (comment) and next couple comments there), though I think storing the old go.mod files was not a preferred approach in the past.

@jayconrod
Copy link
Contributor

I'd lean toward option (1). Seems simpler.

go.mod files affect module boundaries, so it would be hard to get this right. @bcmills mentioned in a meeting that commands run inside the vendor directory would get confused (a go.mod inside the vendor directory would define the main module, possibly the wrong main module). Tools and editors would have this problem, too.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 5, 2020
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 5, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Feb 5, 2020

Seems like the vendor/modules.txt annotation is the way to go: that way we only have to load one file (vendor/modules.txt) to be able to build packages in vendor mode, rather than N different go.mod files, and the diffs are generally smaller, and we won't have to change the version logic if we happen to make go mod vendor start pruning out go.mod files (for example, in order to be less confusing for tools and editors).

@andybons
Copy link
Member

@bcmills what are the next steps here?

@bcmills
Copy link
Contributor Author

bcmills commented Apr 18, 2020

Since there haven't been any breaking language changes in Go 1.15, this doesn't need to be a release-blocker. (I do still plan to add the annotation, but I'm not sure whether it will make 1.15.)

@bcmills bcmills modified the milestones: Go1.15, Go1.16 May 4, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/244078 mentions this issue: cmd/go/internal/modload: cache the Go language version for each module globally

gopherbot pushed a commit that referenced this issue Aug 24, 2020
…e globally

Previously, this cache was a member of the (ephemeral) modload.loader
struct. However, the Go language version for a given module version
does not vary based on the build list, the set of loaded packages, the
build tags in use, the meaning of the "all" pattern, or anything else
that can be configured for an instance of the package loader. The map
containing that information is therefore not appropriate as a field of
the (configurable, package-list-dependent) loader struct.

The Go language version mapping could, in theory, be read from the
go.mod file in the module cache (or replacement directory) every time
it is needed: this map is just a cache, and as such it belongs
alongside the other caches and indexes in the modload package, which
are currently found in modfile.go.

We may want to do the same sort of global caching for the mapping from
each module.Version to its list of direct requirements (which are
similarly idempotent), but for now that is left for a future change.

For #36460
For #36876

Change-Id: I90ac176ffea97f30c47d6540c3dfb874dc9cfa4f
Reviewed-on: https://go-review.googlesource.com/c/go/+/244078
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@bcmills bcmills modified the milestones: Go1.16, Go1.17 Dec 8, 2020
@gopherbot
Copy link
Contributor

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

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>
@bcmills
Copy link
Contributor Author

bcmills commented Apr 28, 2021

This is probably not happening for 1.17, but AFAICT that's ok because there have been no changes to the language between 1.16 and 1.17.

I'm still planning to get to it relatively soon, though.

@bcmills bcmills modified the milestones: Go1.17, Go1.18 Apr 28, 2021
@josharian
Copy link
Contributor

Conversion from slice to array pointer is new in 1.17. Maybe also adding unsafe pointer addition?

@bcmills
Copy link
Contributor Author

bcmills commented Apr 28, 2021

Ah, good point. I might at least need to fine-tune the fix for #44976 before the release.

@bcmills bcmills modified the milestones: Go1.18, Go1.17 Apr 30, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Apr 30, 2021

This turned out to be pretty easy. It will make 1.17 after all.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/315409 mentions this issue: cmd/go: annotate versions in vendor/modules.txt

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/316209 mentions this issue: cmd/go/internal/modload: remove outdated comments and redundant tests

gopherbot pushed a commit that referenced this issue May 4, 2021
The outdated comment in modfile.go was missed in CL 315409.

Upon a closer look at the test case in mod_go_version_vendor.txt, it
is almost completely redundant with the new test in
mod_vendor_goversion.txt. Make it completely redundant and remove it.

Updates #36876

Change-Id: Ibcd1f6b426460aaafbd6dc0be93078547904572b
Reviewed-on: https://go-review.googlesource.com/c/go/+/316209
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>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/335050 mentions this issue: _content/ref/mod: mention other Go 1.17 behaviors that depend on the go version

gopherbot pushed a commit to golang/website that referenced this issue Jul 16, 2021
…go version

Updates golang/go#36876
Updates golang/go#42970
Updates golang/go#45965

Change-Id: I542e9ece986806f9b4a062f242387b1ca47f5814
Reviewed-on: https://go-review.googlesource.com/c/website/+/335050
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>
@rsc rsc unassigned bcmills Jun 23, 2022
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
…go version

Updates golang/go#36876
Updates golang/go#42970
Updates golang/go#45965

Change-Id: I542e9ece986806f9b4a062f242387b1ca47f5814
Reviewed-on: https://go-review.googlesource.com/c/website/+/335050
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>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants