Skip to content

Vendor dependencies, add hacking guide#148

Merged
vbatts merged 3 commits into
opencontainers:masterfrom
s-urbaniak:vendor
Jun 27, 2016
Merged

Vendor dependencies, add hacking guide#148
vbatts merged 3 commits into
opencontainers:masterfrom
s-urbaniak:vendor

Conversation

@s-urbaniak

@s-urbaniak s-urbaniak commented Jun 16, 2016

Copy link
Copy Markdown
Collaborator

This PR vendors all OCI image spec Go dependencies using glide and glide-vc.

Additionally it adds a hacking guide since we accumulated quite some tools/prerequisites for various make targets. The hacking guide documents them.

Fixes #76, #162

@s-urbaniak

Copy link
Copy Markdown
Collaborator Author

@vbatts git-validation complains about blanks at EOF in vendored files:

vendor/github.com/pkg/errors/LICENSE:24: new blank line at EOF.
vendor/go4.org/LICENSE:202: new blank line at EOF.

Can I somehow tell it to ignore any files in vendor/**?

@wking

wking commented Jun 16, 2016

Copy link
Copy Markdown
Contributor

On Thu, Jun 16, 2016 at 02:54:51AM -0700, Sergiusz Urbaniak wrote:

This PR vendors all OCI image spec Go dependencies using glide and
glide-vc.

Plugged in yesterday's Git Rev News 1: manul 2, which uses
submodules for vendoring. I'm unlikely to be a big Go consumer, and I
don't have meaninful personal experience with any Go vendoring tools,
but I thought I'd mention it because I don't like committing external
code to my own repositories ;).

@s-urbaniak

Copy link
Copy Markdown
Collaborator Author

Committing vendored code is quite idiomatic in Go. We do this in many other projects.

Using git submodules for vendoring has been tried before (including me) with mixed/bad results, see https://groups.google.com/forum/m/#!topic/go-package-management/KR2zrSIsRho for details.

@jonboulle

Copy link
Copy Markdown
Contributor

I think we should just fix git-validation

On 17 June 2016 at 15:43, Sergiusz Urbaniak notifications@github.com
wrote:

Committing vendored code is quite idiomatic in Go. We do this in many
other projects.

Using git submodules for vendoring has been tried before (including me)
with mixed/bad results, see
https://groups.google.com/forum/m/#!topic/go-package-management/KR2zrSIsRho
for details.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#148 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACewN6zlXu2RL47EaRxOVj-rO5htzqkqks5qMqSBgaJpZM4I3N3y
.

@philips

philips commented Jun 17, 2016

Copy link
Copy Markdown
Contributor

@vbatts please help with git-validation

On Fri, Jun 17, 2016 at 6:49 AM Jonathan Boulle notifications@github.com
wrote:

I think we should just fix git-validation

On 17 June 2016 at 15:43, Sergiusz Urbaniak notifications@github.com
wrote:

Committing vendored code is quite idiomatic in Go. We do this in many
other projects.

Using git submodules for vendoring has been tried before (including me)
with mixed/bad results, see

https://groups.google.com/forum/m/#!topic/go-package-management/KR2zrSIsRho
for details.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<
#148 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe/ACewN6zlXu2RL47EaRxOVj-rO5htzqkqks5qMqSBgaJpZM4I3N3y

.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#148 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACDCPQn6oagwIwSxbIBwFCVSLvuDFjIks5qMqX7gaJpZM4I3N3y
.

@vbatts

vbatts commented Jun 21, 2016

Copy link
Copy Markdown
Member

man. Since it is using git show --check ... I am not seeing a flag to ignore paths in that git command. Apart from wholly reimplementing this functionality outside of git, it may be faster to either
A) fix the offending issues in a correction commit
B) turn off the whitespace check for a bit

@wking

wking commented Jun 21, 2016

Copy link
Copy Markdown
Contributor

On Tue, Jun 21, 2016 at 10:53:28AM -0700, Vincent Batts wrote:

A) fix the offending issues in a correction commit

I think the validator checks each commit, so you'd probably need to
patch the initial vendoring commit.

@philips

philips commented Jun 22, 2016

Copy link
Copy Markdown
Contributor

I think we should just merge it :-/

LGTM

Approved with PullApprove

Sergiusz Urbaniak added 3 commits June 23, 2016 08:51
Signed-off-by: Sergiusz Urbaniak <sur@coreos.com>
Signed-off-by: Sergiusz Urbaniak <sur@coreos.com>
Signed-off-by: Sergiusz Urbaniak <sur@coreos.com>
@s-urbaniak

s-urbaniak commented Jun 23, 2016

Copy link
Copy Markdown
Collaborator Author

What a mess, I did some duck-taping to hopefully fix everything:

I am a bit worried whether this EOL-sed-fix will work on OSX though.

@vbatts @philips PTAL

@vbatts

vbatts commented Jun 23, 2016

Copy link
Copy Markdown
Member

@s-urbaniak are we concerned about OSX? 😉

Comment thread Makefile
glide update --strip-vcs --strip-vendor --update-vendored --delete
glide-vc --only-code --no-tests
# see http://sed.sourceforge.net/sed1line.txt
for f in $$(find vendor -type f); do sed -i -e :a -e '/^\n*$$/{$$d;N;ba' -e '}' $$f; done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to use find's -exec instead of the for loop (-execdir is safer, but not in POSIX). Although you may have to keep the for loop, because -i is not in POSIX for sed.

@vbatts

vbatts commented Jun 26, 2016

Copy link
Copy Markdown
Member

LGTM

Approved with PullApprove

@jonboulle

jonboulle commented Jun 27, 2016

Copy link
Copy Markdown
Contributor

LGTM (but do we need to worry about #148 (comment) ? I will leave it to @vbatts or one of the OS X users to merge)

Approved with PullApprove

@vbatts

vbatts commented Jun 27, 2016

Copy link
Copy Markdown
Member

For loops are generally fine, though this would break if there were a path that had a space in it. That's generally frowned upon though 🤓.
As for the sed -i, I'm OK waiting until that is an issue.

@vbatts
vbatts merged commit ca58cdf into opencontainers:master Jun 27, 2016
vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Jun 27, 2016
After mulling on opencontainers#148
perhaps removing the for loop is a bit more simple and safe.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Jul 11, 2016
After mulling on opencontainers#148
perhaps removing the for loop is a bit more simple and safe.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
dattgoswami9lk5g added a commit to dattgoswami9lk5g/bremlinr that referenced this pull request Jun 6, 2022
After mulling on opencontainers/image-spec#148
perhaps removing the for loop is a bit more simple and safe.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
laventuraw added a commit to laventuraw/Kihara-tony0 that referenced this pull request Jun 6, 2022
After mulling on opencontainers/image-spec#148
perhaps removing the for loop is a bit more simple and safe.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
tomalopbsr0tt added a commit to tomalopbsr0tt/fabiojosej that referenced this pull request Oct 6, 2022
After mulling on opencontainers/image-spec#148
perhaps removing the for loop is a bit more simple and safe.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
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.

5 participants