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

*: remove vendoring #528

Merged
merged 1 commit into from
Jan 27, 2017
Merged

Conversation

runcom
Copy link
Member

@runcom runcom commented Jan 25, 2017

Fix #527

specs-go acts as a package. No need to have vendoring in here as it
would just cause major pain to people using the specs Go library.

Signed-off-by: Antonio Murdaca runcom@redhat.com

specs-go acts as a package. No need to have vendoring in here as it
would just cause major pain to people using the specs Go library.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@vbatts
Copy link
Member

vbatts commented Jan 25, 2017

lets see. We just added it in #486
There is something nice about the posterity of having the precise source used for the spec provided in the release tarball. Though these compilation issues due to it being in the "vendor/" directory are troublesome.

@jonboulle
Copy link
Contributor

jonboulle commented Jan 25, 2017 via email

@runcom
Copy link
Member Author

runcom commented Jan 25, 2017

Yeah. Honestly I'd rather just move the code from that repo in-tree here.

this would be nice probably

@vbatts
Copy link
Member

vbatts commented Jan 25, 2017

Or even just mv ./vendor/ ./_deps. It's part of this golang vendor experiment. The path resolution assumes long path available is the choice, just not having this stashed code in a directory named "vendor" would fix this issue

@jonboulle
Copy link
Contributor

jonboulle commented Jan 25, 2017 via email

@stevvooe
Copy link
Contributor

That doesn't solve the issue of vendored deps being used in exported
interfaces

Is this considered not best practice? Are we not supposed to vendored types in exported types?

@vbatts vbatts self-assigned this Jan 25, 2017
@vbatts
Copy link
Member

vbatts commented Jan 25, 2017

Current Idea is to move the ./vendor directory(s) under the packages that use remote repos.

vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Jan 26, 2017
Due to the new vendoring logic in the golang compiler it can cause
issues for projects that import a package that has vendored a package
that is used locally. See containers/image#223

This change moves the vendored sources to the package that uses them,
rather than for the whole project. Also is explictly is not vendoring
code repos from "github.com/opencontainers/". For now we'll consider
these non-remote. Though versioning may likely be future concern.

Fixes opencontainers#527
Obsoletes opencontainers#528

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@mtrmac
Copy link
Contributor

mtrmac commented Jan 27, 2017

That doesn't solve the issue of vendored deps being used in exported
interfaces

Is this considered not best practice? Are we not supposed to vendored types in exported types?

(referring to e.g. func Foo(param vendoredpkg.Type), where vendoredpkg is visible to consumers as GitHub.com/opencontainers/image-spec/vendor/foo/bar/vendoredpkg; importing a package with /vendor/ is prohibited by Go).

  • It is impossible to even name the vendored types (e.g. if I want to declare a variable, or a helper function with a parameter, or a return value)
  • It is also impossible to call constructors in the vendored packages.

Basically, exported vendored types are only usable as return values which are immediately consumed.

@vbatts
Copy link
Member

vbatts commented Jan 27, 2017

as our repo is effectively only library, then this is a best option.
For packages that are main, they can vendor these sources in their package.

LGTM

This may need to be included in the RC4 (#537)

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Jan 27, 2017

LGTM

I am generally fine with this approach. At some point, I'd like to stabilize the Go packages, but we'll just have to be flexible with future build problems.

Approved with PullApprove

@stevvooe stevvooe merged commit e29647f into opencontainers:master Jan 27, 2017
@jonboulle
Copy link
Contributor

Gah, no like this. Now we provide NO guidance for downstream consumers for what they should use? Go is a mess.

@runcom runcom deleted the remove-vendoring branch January 27, 2017 21:53
@wking wking mentioned this pull request Feb 3, 2017
@cyphar
Copy link
Member

cyphar commented Nov 12, 2017

I think that, while it is a massive hack, https://gopkg.in/ is the only sane way of handling these sorts of problems (though I don't know how they've handed the whole vendor "experiment").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants