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

Update to support latest godep #22807

Merged
merged 4 commits into from May 13, 2016
Merged

Conversation

eparis
Copy link
Contributor

@eparis eparis commented Mar 10, 2016

godep v53 and earlier included all subdirs of includes in ./...
godep v54 and later only includes the exact packages required.

So all of the 'extra' packages which were subdirs but were dead code are removed.

That bit us because both codecgen and ginkgo are binaries which we got by chance in Godeps. When godep started tracking exactly what was needed instead of just grabbing entire subdirs we lost those binaries. To solve that problem godeps now have to be built with godep save ginko codecgen ./... so that that it explicitly pulls in those two packages. Because no one will ever remember that, I created a script in hack which lists those deps explicitly.

The better import tacking also means that it lists every single package included (transitively) in Godeps.json. Which I believe makes the godep license concatenator from @karlkfi explode in size.

But from an actual 'code that was built' PoV, and easy way to test is to see if a build with and without this PR have any difference. They should be identical.


This change is Reviewable

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 10, 2016
@eparis eparis changed the title Update to support latest godep [WIP] Update to support latest godep Mar 10, 2016
@eparis eparis added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 10, 2016
@eparis
Copy link
Contributor Author

eparis commented Mar 10, 2016

@k8s-bot e2e test this issue: #22807

ok so I lied to you bot, that is my number, but I really just want to know if the last failure was a flake...

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2016
@ghost
Copy link

ghost commented Mar 10, 2016

@eparis I have no idea how to effectively review a monster PR like this, but let me know when you're ready for review, and I'll try to make a plan.

@ghost ghost removed their assignment Mar 10, 2016
@eparis eparis changed the title [WIP] Update to support latest godep Update to support latest godep Mar 10, 2016
@eparis
Copy link
Contributor Author

eparis commented Mar 10, 2016

give it to @thockin that's a good way to do it :)

It is ready. It's really not that hard to review. I'll update the first comment with more info.

@@ -1,7 +1,9 @@
{
"ImportPath": "k8s.io/kubernetes",
"GoVersion": "go1.5",
"GoVersion": "go1.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

busted! now you know what version of go I'm using. It's pointless/harmless/not used by anything at all. I could put it back to 1.5, but it really doesn't mean anything at all.

@eparis
Copy link
Contributor Author

eparis commented Mar 10, 2016

p.s. I added 'do-not-merge' just to be a bit nice/slushy with 1.2 going on. This isn't a huge rush, but getting godep working on head will save some people time, just ask @derekwaynecarr

@eparis
Copy link
Contributor Author

eparis commented Mar 10, 2016

Not that you have any reason to believe me necessarily but building with this PR gives me

f5688877edf16b0495a171acd4b11f2d94d2e9d8981ace044346ce5c6d549725  _output/local/go/bin/kube-apiserver
080ab9e2beed61af304374eff57adf36781b41715e002a114e80a65f7d921c84  _output/local/go/bin/kube-controller-manager
95a5731c753700f27597b5eef1f05e00668269b1e754f5c054d8c694feb890e2  _output/local/go/bin/kubectl
1414dbc0498d453d432c25bbfc82d90ea13d60de7c908a9bd1b189a247cdfc24  _output/local/go/bin/kubelet
ab155a5459be701adc1c4b579e42be5f39677fe579032cda507d34822dc7a0bb  _output/local/go/bin/kubemark
1eda57c4607c0672e3b7c865bc44cc3b791b06210f5a7c62db5db299e553f408  _output/local/go/bin/kube-proxy
91bcf5de87d18cb24952e2500d891422a432d27e10f0cc6446cd3ba9864b4f98  _output/local/go/bin/kube-scheduler

And building at cdd3395 (right before this PR) give me:

f5688877edf16b0495a171acd4b11f2d94d2e9d8981ace044346ce5c6d549725  _output/local/go/bin/kube-apiserver
080ab9e2beed61af304374eff57adf36781b41715e002a114e80a65f7d921c84  _output/local/go/bin/kube-controller-manager
95a5731c753700f27597b5eef1f05e00668269b1e754f5c054d8c694feb890e2  _output/local/go/bin/kubectl
1414dbc0498d453d432c25bbfc82d90ea13d60de7c908a9bd1b189a247cdfc24  _output/local/go/bin/kubelet
ab155a5459be701adc1c4b579e42be5f39677fe579032cda507d34822dc7a0bb  _output/local/go/bin/kubemark
1eda57c4607c0672e3b7c865bc44cc3b791b06210f5a7c62db5db299e553f408  _output/local/go/bin/kube-proxy
91bcf5de87d18cb24952e2500d891422a432d27e10f0cc6446cd3ba9864b4f98  _output/local/go/bin/kube-scheduler

@@ -1,6 +0,0 @@
assignees:
Copy link

Choose a reason for hiding this comment

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

@eparis Is the removal of this intentional?

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2016
@ghost
Copy link

ghost commented Mar 10, 2016

@eparis Any PR that removes this many files must be good, by definition :-) LGTM, barring the one minor nit. Feel free to remove the "do-not-merge" label when it makes sense.

@eparis
Copy link
Contributor Author

eparis commented Mar 11, 2016

@ixdy this is messing with godeps!

@eparis
Copy link
Contributor Author

eparis commented Mar 11, 2016

@quinton-hoole good catch on OWNERS. Fixed.

@eparis eparis added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 11, 2016
@ixdy
Copy link
Member

ixdy commented Mar 11, 2016

Fix for godep-licenses check in #22834, though Travis will still be sad.

@eparis
Copy link
Contributor Author

eparis commented Mar 11, 2016

I don't understand why travis is sad....

@ixdy
Copy link
Member

ixdy commented Mar 11, 2016

@k8s-bot unit test this please, issue #22834

@ixdy
Copy link
Member

ixdy commented Mar 11, 2016

@k8s-bot unit test this, issue #22843

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2016
@eparis eparis added this to the v1.3 milestone May 11, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2016
@@ -196,17 +196,6 @@ export GOPATH=$HOME/go-tools
export PATH=$PATH:$GOPATH/bin
```

Note:
Copy link
Member

Choose a reason for hiding this comment

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

I'm adding a change to require >= v63, so you can drop this hunk

eparis and others added 4 commits May 12, 2016 22:04
Since its hard for an individual to remember that we need codecgen and
ginkgo in godeps but don't actually have that dependancy listed in a way
that godep can automatically find.
@eparis eparis added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 13, 2016
@eparis
Copy link
Contributor Author

eparis commented May 13, 2016

/me grumbles @saad-ali that I did this PR over 2 months ago and have been in rebase hell ever since. You managed to do 99.9% of it and get it merged all in one night. So really that's awesome I guess. Thanks saad.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 13, 2016
@saad-ali
Copy link
Member

It was all @thockin I just banged on the keyboard on his behalf

@eparis
Copy link
Contributor Author

eparis commented May 13, 2016

That was totally going to pad my LoC removed stats!

@thockin
Copy link
Member

thockin commented May 13, 2016

LGTM

@k8s-bot
Copy link

k8s-bot commented May 13, 2016

GCE e2e build/test passed for commit 0114eef.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d2ef57a into kubernetes:master May 13, 2016
@karlkfi
Copy link
Contributor

karlkfi commented May 13, 2016

Is it really desirable to vendor binary dependencies using godep? Why not just vendor the source (outside of godeps) and compile them on-demand as part of the build process?

@eparis
Copy link
Contributor Author

eparis commented May 14, 2016

In both cases some of the code from the project is USED in the repo. So we'd have duplication of at least some of the binary's code...

@thockin
Copy link
Member

thockin commented May 15, 2016

Is there any reason NOT to vendor it? If every build needs it, why have a
different repo? They seem isomorphic to me. One is more "normal" and the
other is more all-in-one.
On May 14, 2016 3:15 PM, "Eric Paris" notifications@github.com wrote:

In both cases some of the code from the project is USED in the repo. So
we'd have duplication of at least some of the binary's code...


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#22807 (comment)

@karlkfi
Copy link
Contributor

karlkfi commented May 16, 2016

Vendoring binaries in Git can ballon the size of the git repo, slowing down the speed of checkout. Every new version completely replaces the old, since no diff can be made.

@freeformz
Copy link

godep only vendors source (not binaries), but can vendor additional packages when specified as part of the package spec. Those additional packages are usually other mains. Those additional mains can then be installed like so: go install k8s.io/kubernetes/vendor/github.com/ugorji/go/codec/codecgen

@eparis eparis deleted the godep-v57 branch November 30, 2016 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants