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: the vendor/ folder should be kept up to date if present #29058

Closed
FiloSottile opened this issue Dec 1, 2018 · 14 comments
Closed

cmd/go: the vendor/ folder should be kept up to date if present #29058

FiloSottile opened this issue Dec 1, 2018 · 14 comments
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@FiloSottile
Copy link
Contributor

Using go mod vendor is great for backwards compatibility (and for self-contained builds), but it requires re-running every time any dependency changes. Multiple times now I have committed changes that I tested with GO11MODULES=on, only to have them fail in CI where they rely on vendor/.

The go tool already updates go.mod and go.sum when making builds, I believe it should do the same with the vendor/ folder if it's present, to avoid getting in an inconsistent state.

An out of date vendor/ folder feels like an entirely inconsistent and confusing state to be in, causing builds to unexpectedly fail only for certain users, with no advantages, so the tooling should probably just not let it happen. If a developer doesn't want to keep the vendor/ folder up to date, it should be deleted.

/cc @rsc @bcmills

@FiloSottile FiloSottile added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go modules labels Dec 1, 2018
@FiloSottile
Copy link
Contributor Author

This by the way feels like a requirement of #27227, but I think that even if using vendor/ is not the default, its presence suggests it's an option, so it shouldn't be broken.

@bcmills
Copy link
Contributor

bcmills commented Dec 3, 2018

An out of date vendor/ folder feels like an entirely inconsistent and confusing state to be in, causing builds to unexpectedly fail only for certain users, with no advantages,

Conversely, one of the most consistent use-cases I have heard for the vendor directory is as a place to experiment with patches to dependencies.

We clearly do need to be able to verify the consistency of the vendor directory (see also #27348), especially as a pre-commit hook, and go release should ensure that vendor is up-to-date (#26420 (comment)), but I don't think we can reasonably update it without any intervention at all.

@bcmills bcmills added this to the Unplanned milestone Dec 3, 2018
@FiloSottile
Copy link
Contributor Author

FiloSottile commented Dec 3, 2018

I feel like the fact that vendor/ is used to experiment with patches to dependencies makes it even worse for it to be inconsistent: a dev will be running builds in modules mode (or this issue does not apply) with patches in vendor/, and they will silently not work, while they did for someone else who was running in GOPATH mode. At least if they get rewritten it will be clear what's happening.

I agree that existing packages should not be modified when using -mod=vendor, because then they are authoritative in themselves. (But new ones should be added as needed.) If vendor is being used to experiment with patches to dependencies, -mod=vendor should (and probably will) be used anyway.

(I am very skeptical of the adoption of pre-commit hooks.)

@bcmills
Copy link
Contributor

bcmills commented Dec 3, 2018

If vendor is being used to experiment with patches to dependencies, -mod=vendor should (and probably will) be used anyway.

Ideally I would like the -mod=vendor flag to go away entirely. (The vendor directory should behave more like a set of implicit replace statements than a separate build mode.)

@FiloSottile
Copy link
Contributor Author

(The vendor directory should behave more like a set of implicit replace statements than a separate build mode.)

+1 on that. (Which I guess is a solution to #27227?) In that case it's even more natural to update vendor/ to reflect the build, because all existing packages stay as they are, and missing ones are filled in, providing backwards compatibility.

@bcmills
Copy link
Contributor

bcmills commented Dec 3, 2018

Given #27227, it's not clear to me when we would update the vendor directory at all. Perhaps during go get if the previous contents were still clean?

That seems pretty subtle, though.

@FiloSottile
Copy link
Contributor Author

FiloSottile commented Dec 3, 2018

Is the use case of vendor/ as backwards compatibility for GOPATH mode not a supported goal?

Also, a lot of the community support for vendor/ was not as implicit replaces but to guarantee self-contained builds, and if it's not kept up to date that goal is defeated.

Personally I never liked using vendor/ patches as it's subtle and implicit, and I don't think it's something we should be encouraging.

@nomad-software
Copy link

Ideally I would like the -mod=vendor flag to go away entirely. (The vendor directory should behave more like a set of implicit replace statements than a separate build mode.)

@bcmills I completely disagree with this and think this is a really bad idea. This may be ok for Google's workflow but would break lots of other company's.

The vendor folder is currently the only way to guarantee reproducible builds on all machines even if offline. Also, if you remove it, how are you going to tackle the problem of remote dependencies disappearing from the internet? These problems need addressing before any talk on removing the vendor folder.

@bcmills
Copy link
Contributor

bcmills commented Dec 4, 2018

@nomad-software I said I want to remove the flag, not the vendor directory. If we use vendor by default, then we don't need an explicit flag to enable it.

@nomad-software
Copy link

@bcmills Ok, I misunderstood but that still raises the question how do we build against the download cache or the vendor folder? That flag is really handy. You shouldn't just remove it without a really good reason or an alternative to achieve the functionality of what we enjoy now.

@aarzilli
Copy link
Contributor

Given #27227, it's not clear to me when we would update the vendor directory at all. Perhaps during go get if the previous contents were still clean?

Older dependency management programs (govendor, go dep, glide, etc) all overwrote the vendor directory unconditionally. This didn't prevent people from using it to experiment with changes to dependencies. Modules are a bit different because dependencies are updated implicitly as part of the build process, however I think that this simple rule: "if go.mod is changed run go mod vendor", would satisfy everyone. It would keep the vendor directory up to date with go.mod and let people make changes inside vendor.

@bcmills
Copy link
Contributor

bcmills commented Feb 11, 2019

I've been looking into the details for #27227, and I tend to agree that it will make this proposal necessary as well.

@bcmills
Copy link
Contributor

bcmills commented Oct 10, 2019

#33848 explicitly checks for consistency.

For the reasons described in #30240 (comment), we should not update the vendor directory automatically.

@bcmills
Copy link
Contributor

bcmills commented Oct 10, 2019

Closing in favor of #33848.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge GoCommand cmd/go modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants