prune Bazel build files from imported go deps and update to gazelle 0.10.1 #61575

ixdy · 2018-03-23T03:10:40Z

What this PR does / why we need it: updates the hack/godep-save.sh script to prune any imported BUILD, BUILD.bazel, or WORKSPACE files.

We assume anything imported through godep doesn't need Bazel, but a Bazel-enabled Go project may include BUILD files with missing dependencies not imported by godep. To prevent this from breaking the Bazel build, remove these. (Gazelle will then regenerate the BUILD files based on the go build metadata.)

Additionally, there seems to be a bug in gazelle where it follows the vendor/k8s.io staging symlinks only when run through hack/run-in-gopath.sh. Updating to gazelle 0.10.1 fixes this bug.

A similar PR was recently merged in kubernetes/test-infra: kubernetes/test-infra#7366

Release note:

NONE

/assign @thockin
cc @jayconrod

cblecker · 2018-03-23T03:26:03Z

Godeps/Godeps.json

@@ -1976,6 +1976,7 @@
 		},
 		{
 			"ImportPath": "github.com/inconshreveable/mousetrap",
+			"Comment": "v1.0",


Interesting that it's adding these back in.

🤷‍♂️ who even understands godep?

:left-shark:

cblecker · 2018-03-23T03:26:53Z

hack/godep-save.sh

+# Assume that anything imported through godep doesn't need Bazel to build.
+# Prune out any Bazel build files, since these can break the build due to
+# missing dependencies that aren't included by godep.
+find vendor/ -type f \( -name BUILD -o -name BUILD.bazel -o -name WORKSPACE \) \


Would this logic make more sense as part of hack/update-bazel.sh?

I thought about that, but I didn't want to unnecessarily recreate these files all the time, just only when we're actually updating vendor/

My thought is, if we're calling hack/update-bazel.sh for some reason, we are specifically saying we want to update the bazel files. Is there a large performance difference between updating without deleting, vs deleting?

I also wonder if there's some weird situation we could get into where a build file is updated in some other way, and starts failing the verify-bazel step because of the way it recreates in a temp folder.

I'm not stuck on it, but part of me thinks if you're we're gonna blow them away at all, just blow them all away every time.

the other reason not to do this is that kazel prints out every time it updates a file, which it'll do every time these are recreated, even though there will likely be no diff.

BenTheElder · 2018-03-23T03:37:51Z

/retest

ixdy · 2018-03-23T07:48:58Z

/retest

thockin · 2018-03-23T20:21:53Z

hack/update-bazel.sh

+(
+  # gazelle needs to run from the real ${KUBE_ROOT}, as otherwise its symlink
+  # following logic can get confused.
+  cd "${KUBE_ROOT}"


Not sure I understand this - under what circumstances is it not?

recent functionality in gazelle causes it to follow symlinks, but only if they're outside the repo root.

if you run hack/run-in-gopath.sh hack/update-bazel.sh, gazelle ends up running inside _output/local/go/src/k8s.io/kubernetes. gazelle doesn't currently resolve the repo root to an absolute path, so its check to verify that the symlink is out-of-repo fails (since it resolves symlinks its considering to their absolute path).

For example, consider vendor/k8s.io/apimachinery, which points to ../../staging/src/k8s.io/apimachinery.

When run in the real $KUBE_ROOT, gazelle detects the repo root as $HOME/go/src/k8s.io/kubernetes and the endpoint of this symlink as $HOME/go/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery. Since that's within the repo, it doesn't follow the symlink.

When run through hack/run-in-gopath.sh, gazelle detects the repo root as $HOME/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes, but still resolves the endpoint of the vendor symlink as $HOME/go/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery. It considers this as out-of-repo and thus follows the symlink, erroneously.

I tried passing -repo_root ${KUBE_ROOT} instead of the chdir, but this was causing different problems in vendor/ that I haven't yet figured out.

I also have refrained from changing the cwd for kazel, since kazel will blow up if it's not run from a valid GOPATH.

thockin · 2018-03-23T20:22:10Z

LGTM, just one Q

cblecker · 2018-03-23T20:25:54Z

/retest

BenTheElder · 2018-03-23T20:59:32Z

/retest

cblecker · 2018-03-23T21:48:38Z

/lgtm
/approve

thockin · 2018-03-23T22:02:30Z

recent functionality in gazelle causes it to follow symlinks, but only if

they're outside the repo root. How does it calculate repo root?

if you run hack/run-in-gopath.sh hack/update-bazel.sh, gazelle ends up

running inside _output/local/go/src/k8s.io/kubernetes. gazelle doesn't currently resolve the repo root to an absolute path, so its check to verify that the symlink is out-of-repo fails (since it resolves symlinks its considering to their absolute path). This sounds incredibly broken and fragile. If it needs to do anything like this, it should probably canonicalize the path, resolve symlinks, etc. `pwd -P`

For example, consider vendor/k8s.io/apimachinery, which points to

../../staging/src/k8s.io/apimachinery.

When run in the real $KUBE_ROOT, gazelle detects the repo root as

$HOME/go/src/k8s.io/kubernetes and the endpoint of this symlink as $HOME/go/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery. Since that's within the repo, it doesn't follow the symlink.

When run through hack/run-in-gopath.sh, gazelle detects the repo root as

$HOME/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes, but still resolves the endpoint of the vendor symlink as $HOME/go/src/ k8s.io/kubernetes/staging/src/k8s.io/apimachinery. It considers this as out-of-repo and thus follows the symlink, erroneously. Did you file a bug against gazelle? This is broken.

thockin · 2018-03-23T22:03:30Z

I'm happy of you just leave a TODO with a bug link to a bug against gazelle.

…

On Fri, Mar 23, 2018 at 3:02 PM Tim Hockin ***@***.***> wrote: > recent functionality in gazelle causes it to follow symlinks, but only if they're outside the repo root. How does it calculate repo root? > if you run hack/run-in-gopath.sh hack/update-bazel.sh, gazelle ends up running inside _output/local/go/src/k8s.io/kubernetes. gazelle doesn't currently resolve the repo root to an absolute path, so its check to verify that the symlink is out-of-repo fails (since it resolves symlinks its considering to their absolute path). This sounds incredibly broken and fragile. If it needs to do anything like this, it should probably canonicalize the path, resolve symlinks, etc. `pwd -P` > For example, consider vendor/k8s.io/apimachinery, which points to ../../staging/src/k8s.io/apimachinery. > When run in the real $KUBE_ROOT, gazelle detects the repo root as $HOME/go/src/k8s.io/kubernetes and the endpoint of this symlink as $HOME/go/src/k8s.io/kubernetes/staging/src/k8s.io/apimachinery. Since that's within the repo, it doesn't follow the symlink. > When run through hack/run-in-gopath.sh, gazelle detects the repo root as $HOME/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes, but still resolves the endpoint of the vendor symlink as $HOME/go/src/ k8s.io/kubernetes/staging/src/k8s.io/apimachinery. It considers this as out-of-repo and thus follows the symlink, erroneously. Did you file a bug against gazelle? This is broken.

ixdy · 2018-03-23T22:06:33Z

I agree that it's fragile. I'm discussing this with @jayconrod out-of-band to confirm that it's a bug in gazelle (I'm pretty sure it is).

ixdy · 2018-03-23T22:41:25Z

It looks like this was just fixed in bazelbuild/bazel-gazelle#166.

Updating to a newer gazelle causes more churn in some of the build files due to bazelbuild/bazel-gazelle#148, and it probably also requires updating rules_go.

I'm tempted to just leave a comment here pointing at bazelbuild/bazel-gazelle#166 and then plan to update this soon and remove the workaround, rather than bumping everything now. WDYT @thockin?

cblecker · 2018-03-23T22:43:51Z

/hold
for decision

jayconrod · 2018-03-23T22:48:07Z

@ixdy I'll cherry-pick that fix back to Gazelle 0.10 and tag 0.10.1.

Gazelle at tip will require a newer version of rules_go. In particular, bazelbuild/bazel-gazelle#148 generates importmap attributes on go_proto_library, and those were recently introduced. We need to have importmap attributes for vendored libraries in order to avoid conflicts when multiple copies of the same package are linked (which is more common than you would think).

ixdy · 2018-03-23T22:54:31Z

yeah, I was just about to comment that I tried updating to gazelle HEAD, and it complained that RULES_GO_VERSION is 0.10.0; updating to rules_go HEAD doesn't resolve that error message.

we're actually not using 0.10.0 right now, but rather 6 commits past it; we're using this version because we needed buildtools to be vendored.

ixdy · 2018-03-23T22:58:50Z

though: this PR will unblock #57600, which will allow us to vendor gazelle and buildtools ourselves.

at that point, it won't matter whether the version of gazelle we're using vendors buildtools or not.

jayconrod · 2018-03-23T22:59:40Z

In Gazelle 0.10.1, I'll include changes that vendor buildtools on our side, so you may not need to vendor it yourself unless you have a separate dependency.

jayconrod · 2018-03-23T23:07:05Z

Gazelle 0.10.1 is ready.

ixdy · 2018-03-23T23:19:51Z

/hold cancel

Updated to gazelle 0.10.1. Removed the chdir workaround; confirmed that we still get the same results via hack/update-bazel.sh and hack/run-in-gopath.sh hack/update-bazel.sh.

cblecker · 2018-03-23T23:22:22Z

@ixdy do we need the -repo_root flag on gazelle?

ixdy · 2018-03-23T23:43:53Z

nope; it'll auto-detect it based on the presence of the WORKSPACE file.

cblecker · 2018-03-23T23:45:51Z

/lgtm

k8s-ci-robot · 2018-03-23T23:47:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]
~~hack/OWNERS~~ [cblecker,ixdy]

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 · 2018-03-27T01:56:04Z

Automatic merge from submit-queue (batch tested with PRs 61546, 61038, 61575, 60779, 61496). If you want to cherry-pick this change to another branch, please follow the instructions here.

…57600-#62499-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #61575: Remove all upstream BUILD, BUILD.bazel, and WORKSPACE #57600: Vendor gazelle #62499: Update kazel to include openapi tag detection fix Cherry pick of #61575 #57600 #62499 on release-1.10. #61575: Remove all upstream BUILD, BUILD.bazel, and WORKSPACE #57600: Vendor gazelle #62499: Update kazel to include openapi tag detection fix This is an alternative to #64928, and would fix #64925. We can't just cherry-pick #57600, since there was a prerequisite PR and a fixup PR afterwards.

k8s-ci-robot assigned thockin Mar 23, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 23, 2018

k8s-ci-robot requested review from dchen1107 and gmarek March 23, 2018 03:10

ixdy force-pushed the godep-remove-upstream-vendored-BUILD-files branch from e28319c to 9d686e2 Compare March 23, 2018 03:13

ixdy mentioned this pull request Mar 23, 2018

Vendor gazelle and kazel #57600

Merged

cblecker reviewed Mar 23, 2018

View reviewed changes

cblecker self-assigned this Mar 23, 2018

thockin reviewed Mar 23, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 23, 2018

jayconrod mentioned this pull request Mar 23, 2018

Evaluate symlinks in repo root bazelbuild/bazel-gazelle#166

Merged

Remove all upstream BUILD, BUILD.bazel, and WORKSPACE files from vendor/

7015f19

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2018

ixdy added 2 commits March 23, 2018 16:16

Update to gazelle 0.10.1

009134a

run hack/godep-restore.sh && hack/godep-save.sh

f7ec474

ixdy force-pushed the godep-remove-upstream-vendored-BUILD-files branch from 9d686e2 to f7ec474 Compare March 23, 2018 23:18

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 23, 2018

ixdy changed the title ~~prune Bazel build files from imported go deps~~ prune Bazel build files from imported go deps and update to gazelle 0.10.1 Mar 23, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2018

k8s-github-robot merged commit 9b14be1 into kubernetes:master Mar 27, 2018

ixdy deleted the godep-remove-upstream-vendored-BUILD-files branch May 15, 2018 23:39

ixdy mentioned this pull request Jun 8, 2018

Automated cherry pick of #61575: Remove all upstream BUILD, BUILD.bazel, and WORKSPACE #57600: Vendor gazelle #62499: Update kazel to include openapi tag detection fix #64929

Merged

nikhita mentioned this pull request Jun 24, 2018

godep-save remove all "BUILD" file #65403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prune Bazel build files from imported go deps and update to gazelle 0.10.1 #61575

prune Bazel build files from imported go deps and update to gazelle 0.10.1 #61575

ixdy commented Mar 23, 2018 •

edited

Loading

cblecker Mar 23, 2018

ixdy Mar 23, 2018

cblecker Mar 23, 2018

cblecker Mar 23, 2018

ixdy Mar 23, 2018

cblecker Mar 23, 2018

ixdy Mar 23, 2018

BenTheElder commented Mar 23, 2018

ixdy commented Mar 23, 2018

thockin Mar 23, 2018

ixdy Mar 23, 2018

thockin commented Mar 23, 2018

cblecker commented Mar 23, 2018

BenTheElder commented Mar 23, 2018

cblecker commented Mar 23, 2018

thockin commented Mar 23, 2018 via email

thockin commented Mar 23, 2018 via email

ixdy commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

jayconrod commented Mar 23, 2018

ixdy commented Mar 23, 2018

ixdy commented Mar 23, 2018

jayconrod commented Mar 23, 2018

jayconrod commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

k8s-ci-robot commented Mar 23, 2018

k8s-github-robot commented Mar 27, 2018

prune Bazel build files from imported go deps and update to gazelle 0.10.1 #61575

prune Bazel build files from imported go deps and update to gazelle 0.10.1 #61575

Conversation

ixdy commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Mar 23, 2018

ixdy commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Mar 23, 2018

cblecker commented Mar 23, 2018

BenTheElder commented Mar 23, 2018

cblecker commented Mar 23, 2018

thockin commented Mar 23, 2018 via email

thockin commented Mar 23, 2018 via email

ixdy commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

jayconrod commented Mar 23, 2018

ixdy commented Mar 23, 2018

ixdy commented Mar 23, 2018

jayconrod commented Mar 23, 2018

jayconrod commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

ixdy commented Mar 23, 2018

cblecker commented Mar 23, 2018

k8s-ci-robot commented Mar 23, 2018

k8s-github-robot commented Mar 27, 2018

ixdy commented Mar 23, 2018 •

edited

Loading