Vendor gazelle and kazel

[thockin]

Rather that relying on upstream git repos that can break, vendor it all. These are NOT head of tree, respectively - they are some backrev forms that were previously being used.

NONE

[fejta]

Love the idea. Any idea why tests are failing?

[thockin]

godep is not picking up a transitive dep - trying to figure out why. Some of teh bazel repos were renamed, and kazel uses the old name, so I wonder if something is confused. Of course, godep is useless...

github.com/bazelbuild/buildifier is now github.com/bazelbuild/buildtools

[cblecker]

Just came across this.. was taking a look at the viability of this last week!

The transitive dep issue is fixed in this one: kubernetes/repo-infra#54.. repo-infra doesn't have a dependency definition file at all, just a vendor dir.. but you can't vendor a vendor dir.

@thockin: Is it safer to install these utils to a temp gopath, rather than clobber whatever version is in the running user's gopath? (my WIP branch: https://github.com/kubernetes/kubernetes/compare/master...cblecker:vendor-bazel-stuff?expand=1)

[thockin]

I started installing to the phony local GOPATH, but I am torn on it. It's trivial to do (1 line). The godep scripts don't do that either. Open to opinion

[thockin]

Indeed, it seems that godep satisfies itself with github.com/kubernetes/repo-infra/vendor/github.com/bazelbuild/buildifier/core but that does not include the tables sub-package. It's amazing this works at all.

[thockin]

Ugh, I hate GOPATH. Non-hermetic no matter what I do. Kazel only builds because of it. If I run-in-gopath, then the missing dep is satisfied from GOPATH but ONLY IF I previously ran godep-restore.

I really want different GOPATHs for different purposes...

[fejta]

Agreed that GOPATH and how golang handles symlinks is pretty terrible 😞

[thockin]

Need to re-run the godep imports, out of time for tonight.

[ixdy]

note for when you revisit this: the 0.8.0 tag in bazelbuild/bazel-gazelle is compatible with the version of gazelle we're using now, and is preferred over the gazelle in rules_go.

[thockin]

no 0.8.0, but 0.8 matches the hash.

[thockin]

Pushed with updated gazelle and repo-infra that satisfies godep.

[ixdy]

I think this might finally be ready for final rebase and merge.

Note we're now using gazelle 0.10.1 in k/k. I've also ensured that the master branch of kazel now builds with the version of bazelbuild/buildtools vendored by gazelle.

[ixdy]

this seems pretty fragile - it's can be possible that $GOPATH/bin is not in your PATH, but there might already be a gazelle installed in $PATH anyway, which will cause this to pass unexpectedly.

Maybe instead set GOBIN="${KUBE_OUTPUT_BINPATH}" (i.e. $KUBE_ROOT/_output/bin) before running go install?

this is what I did for kubernetes/test-infra#7429 and kubernetes/repo-infra#65 and it seems to be working well.

[thockin]

Not a bad idea. I forgot we have that.

[thockin]

Rebased & revendored. Let's see...

[ixdy]

Oh, hah, verify-bazel.sh is also diffing _output/local/bin. This PR is cursed.

I0329 17:08:15.675] Verifying verify-bazel.sh
I0329 17:08:15.691] 
I0329 17:08:15.692] +++ Running case: verify.bazel 
I0329 17:08:15.694] +++ working dir: /go/src/k8s.io/kubernetes
I0329 17:08:15.696] +++ command: bash "hack/make-rules/../../hack/verify-bazel.sh"
W0329 17:08:29.053] hack/make-rules/../../hack/verify-bazel.sh: line 45: warning: command substitution: ignored null byte in input
W0329 17:08:31.307] diff -Naupr /go/src/k8s.io/kubernetes/_output/local/bin/gazelle /tmp/verify-bazel.ZI9Uhx/go/src/k8s.io/kubernetes/_output/local/bin/gazelle
W0329 17:08:31.308] --- /go/src/k8s.io/kubernetes/_output/local/bin/gazelle	1970-01-01 00:00:00.000000000 +0000
W0329 17:08:31.308] +++ /tmp/verify-bazel.ZI9Uhx/go/src/k8s.io/kubernetes/_output/local/bin/gazelle	2018-03-29 17:08:20.781360056 +0000
W0329 17:08:31.309] @@ -0,0 +1,32099 @@
W0329 17:08:31.309] +�ELF����>�À¤E@p�@8
...

[cblecker]

I have a feeling that the method I use in #59210 would work here. Because we install it, then record the path it was installed to for re-use, it doesn't try to do things like install it a second time into temp paths.

[thockin]

/retest

[ixdy]

I think adding an exclude on _output to the diff should fix the remaining failure:

diff --git a/hack/verify-bazel.sh b/hack/verify-bazel.sh
index 4a110ebad0..132428fc12 100755
--- a/hack/verify-bazel.sh
+++ b/hack/verify-bazel.sh
@@ -42,7 +42,7 @@ cp -a "${KUBE_ROOT}" "${_tmp_kuberoot}/.."
 cd "${_tmp_kuberoot}"
 GOPATH="${_tmp_gopath}" PATH="${_tmp_gopath}/bin:${PATH}" ./hack/update-bazel.sh
 
-diff=$(diff -Naupr "${KUBE_ROOT}" "${_tmp_kuberoot}" || true)
+diff=$(diff -Naupr -x '_output' "${KUBE_ROOT}" "${_tmp_kuberoot}" || true)
 
 if [[ -n "${diff}" ]]; then
   echo "${diff}" >&2

[thockin]

w00t! it's green.

[cblecker]

Mostly fine. One question, and one potential request for follow up.

[cblecker]

Do we need to export this? I don't like modifying a user's environment beyond the life of the script. There's a couple other cases that you're doing this too, so I'm not sure if it's strictly a requirement.

[thockin]

export exists for the life of the shell, not longer. Maybe I misunderstand. I did not put it in a generic place that gets sourced, for this reason.

$ echo $FOO; (export FOO=foo; echo $FOO; (export FOO=bar; echo $FOO); echo $FOO); echo $FOO

foo
bar
foo

[cblecker]

I don't like that there is inconsistency between the way we handle godep, and the way we are handling gazelle/kazel/misspell.

It's not enough of a dislike to block this PR that we badly need, but I'd really like if we could unify on a single way (maybe via a helper util function) to install a specific version of a go app from vendor, use it in the context of the script, with as little pollution of the environment as possible. Maybe I'll look at this in a follow up PR?

[thockin]

We could set GOBIN in init.sh or something, and then always call $GOBIN/godep

But as long as we're checking the version of godep, it shouldn't matter. I agree it would be cleaner to be consistent. Our scripts are a mess here.

function gobin() {
  echo "${KUBE_OUTPUT_BINPATH}"
}

function go_install() {
  GOBIN="$(gobin)" go install "$@"
}

function run_private() {
  PATH="$(gobin):${PATH}" "$@"
}

function run_global() {
  "$@"
}


go_install ./vendor/github.com/tools/godep/
run_private godep save

alternatively, make teh callers of ensure_godep_version pass the path to the godep they are using, and they can pass ${GOBIN}/godep ?

Agree it should be a followup

[cblecker]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• Godeps/OWNERS [cblecker,thockin]
• api/OWNERS [thockin]
• build/OWNERS [cblecker,thockin]
• hack/OWNERS [cblecker,thockin]
• pkg/OWNERS [thockin]
• vendor/OWNERS [cblecker,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

[ixdy]

welp, the openapi generator in kazel is picking up itself, erroneously. I'll fix this.

[thockin]

thanks