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

Open
FiloSottile opened this Issue Dec 1, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@FiloSottile
Member

FiloSottile commented Dec 1, 2018

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

This comment has been minimized.

Member

FiloSottile commented Dec 1, 2018

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

FiloSottile commented Dec 3, 2018

(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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

nomad-software commented Dec 4, 2018

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

This comment has been minimized.

Member

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

This comment has been minimized.

nomad-software commented Dec 4, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment