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

Vendor dependencies with vndr #231

Closed
wants to merge 3 commits into from
Closed

Vendor dependencies with vndr #231

wants to merge 3 commits into from

Conversation

erikh
Copy link
Contributor

@erikh erikh commented Jan 31, 2017

This adds vendored dependencies. It also adjusts the tests to avoid the vendor dir.

As mentioned in containers/storage#24 I think this
would best be implemented eventually as vendor being a separate monorepo that
could share deps with both projects, while reducing repo churn. Maybe skopeo
too.

I started running into problems with some build tags so I imported a version of
hcsshim which has explicit build tags. You can see the appropriate ticket here:
microsoft/hcsshim#106

The fork is mine at erikh/hcsshim and it's a fork off master, should be good.

Please let me know if anything should change.

Erik Hollensbe added 3 commits January 30, 2017 13:18
Signed-off-by: Erik Hollensbe <github@hollensbe.org>
Signed-off-by: Erik Hollensbe <github@hollensbe.org>
Note the Microsoft/hcsshim package is a branch of erikh/hcsshim right
now. This allows us to compile, because hcsshim does not have
appropriate build tags.

Signed-off-by: Erik Hollensbe <github@hollensbe.org>
@mtrmac
Copy link
Collaborator

mtrmac commented Jan 31, 2017

Thanks for the PR.

Recently we’ve run into very serious issues trying to use libraries which vendor their dependencies: distribution/distribution#2130 and opencontainers/image-spec#527 ; basically the vendored dependencies are not possible to use from callers of the library, without callers of the library using their own vendoring tool which strips the vendored dependencies… and then we might just as well not ship the vendored copies at all because we are again not in control of the versions of the dependencies.

I do agree that this January, when containers/image has been broken by dependencies changing the API about half of the time, has been very painful, but it's not clear to me that this is noticeably better.

(Also, in principle, we do want to be able to use the latest versions, or at least very recent versions, of the dependencies, most of the time; vendoring them would make it too easy to freeze very old versions without noticing. But that could, I suppose, be handled with some process change.)

@runcom ?

@erikh
Copy link
Contributor Author

erikh commented Feb 1, 2017 via email

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 2, 2017

This is why I'm advocating for the monorepo that's shared amongst ALL containers/ projects. Dependencies

I don’t know, that could make sense (as a git submodule?), though it’s non-obvious that the coordination would work well. Somebody will always want to stay behind because they don’t have time now, and somebody will always want to push ahead for a new nifty feature.

But even if that monorepo / submodule did exist, the above failure cases still suggest that containers/image should not just be shipping with a vendor subdirectory: even if both containers/image and its consumer had an exactly identical vendor subdirectory from the monorepo, the named types would still not be interoperable! That monorepo would make sense to use probably only as an extra element in $GOPATH when running tests. We, kind of, get a similar behavior with make test-skopeo which does use the vendored versions (but, sure, we first test against master for the internal unit tests(.

@erikh
Copy link
Contributor Author

erikh commented Feb 2, 2017

ok. I'm gonna close this then. You're right, this probably won't fly.

@erikh erikh closed this Feb 2, 2017
@erikh erikh deleted the vndr branch February 4, 2017 02:02
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.

2 participants