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 get', 'go mod tidy' should update the vendor directory when present #45161

Open
jayconrod opened this issue Mar 22, 2021 · 6 comments
Open

Comments

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Mar 22, 2021

This is basically the same issue as #29058, but the default behavior has changed, so it may be worth reopening the discussion.

Since 1.14, the go command may use the vendor directory by default (without the -mod=vendor flag) if the go version in go.mod is 1.14 or higher and vendor/modules.txt is present. The go command reports an error if go.mod and vendor/modules.txt are out of sync.

Two commands primarily change go.mod: go mod tidy and go get. When these commands make changes, they break vendor builds until go mod vendor is run again.

We've avoided rebuilding the vendor directory implicitly, since the go command currently deletes the entire folder, loads the import graph, and recreates it from scratch. That may cause developers to lose local changes (like a patch being prepared for a dependency).

I propose some new behavior:

  • We need a mechanism to detect local changes to vendor. That probably means comparing the contents of each package directory to the corresponding directory in the module cache. That would help us implement #27348.
  • go get would add or update packages in the vendor directory automatically if it's present. Only packages needed to build the packages named on the command line would be affected.
  • go mod tidy would implicitly rebuild the vendor directory. go mod vendor would be a no-op after go mod tidy.
  • Any command that modifies the vendor directory would report an error if local changes would be overwritten. We might want a flag to force overwrite, or the developer could run git checkout or rm -rf vendor first.
  • No change to go build and other commands. -mod=readonly and -mod=mod would continue to ignore the vendor directory.

cc @bcmills @FiloSottile @matloob

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 22, 2021

FWIW, I think the major behavior change that impacts this since #29058 is #42466.

My main concern was that, because go.mod dependency updates happened implicitly, we would implicitly update a vendored package and accidentally discard a local modification (for example, one important to an ongoing debugging session). However, since we now default to -mod=readonly, the set of commands that may trigger an accidental overwrite is now much smaller.

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 22, 2021

  • go get would add or update packages in the vendor directory automatically if it's present. Only packages needed to build the packages named on the command line would be affected.

I'm a bit worried about that behavior. In my experience, users often invoke go get on module roots rather than the specific packages they need, so automatically vendoring the packages named by go get could be both overly conservative (doesn't add other needed packages from the module) and overly aggressive (does add packages needed by the module root, even though the user doesn't actually care about the module's root package).

@oiooj
Copy link
Member

@oiooj oiooj commented Mar 23, 2021

go mod vendor distinguishes well between mode readonly and mode vendor, but I feel that the boundary between the two modes are becoming blurred. Should we reduce implicit changes for developers ?

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 23, 2021

@oiooj, go mod vendor itself does not have a -mod flag, and therefore does not distinguish between -mod=readonly and -mod=vendor. So I don't think I understand what you're getting at...

@oiooj
Copy link
Member

@oiooj oiooj commented Mar 23, 2021

If user uses go mod vendor, it clearly runs in vendor mode. and the user needs to run this command regularly to ensure that the dependencies are synchronized.

But if it automatically updates vendor, it‘s not easy for users to notice the change, even the mode they used.

@oiooj, go mod vendor itself does not have a -mod flag, and therefore does not distinguish between -mod=readonly and -mod=vendor. So I don't think I understand what you're getting at...

If the user manually executes go mod vendor, it reminds you that you are in vendor mode. I think this has many benefits, sorry for my bad English.

@jayconrod
Copy link
Contributor Author

@jayconrod jayconrod commented Mar 23, 2021

@oiooj To clarify, the -mod flag controls how packages are loaded and built in module mode. It accepts three values, mod (uses module cache and network; updates go.mod automatically), readonly (like mod but does not update go.mod), and vendor (uses vendor directory; does not use module cache or network; does not update go.mod).

If the go version in go.mod is 1.14 or higher, and the file vendor/modules.txt is present (not necessarily in sync with go.mod), then the go command defaults to -mod=vendor. Otherwise, it defaults to -mod=readonly.

-mod is not accepted by go mod subcommands including go mod vendor, nor is it accepted by go get. go mod vendor uses the module cache and network to build the vendor directory.

...

So the current user experience is that a developer runs go mod vendor, and then vendoring is enabled by default in their project. If they want to explicitly control whether vendoring is enabled, they can add -mod=readonly or -mod=vendor to GOFLAGS.

If they change go.mod without running go mod vendor again, they'll see an error because vendor/modules.txt is out of sync. I don't see the benefit in having to run go mod vendor again; it's just an extra step.

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
3 participants