Use dep for repo-infra deps #54
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @ixdy |
verify/update-bazel.sh
Outdated
| # exists; there is a specific test_suite rule that breaks importing | ||
| rm -f ${REPOINFRA_ROOT}/vendor/github.com/bazelbuild/buildtools/BUILD.bazel | ||
|
|
||
| GOBIN="${TMP_GOPATH}/bin" go install "./kazel" |
There was a problem hiding this comment.
this one is a bit weird, since in theory the scripts under verify/ in repo-infra should be usable in other repos.
There was a problem hiding this comment.
right. wasn't thinking about that -- only thinking about in the context of local. will fix.
| 0.8 \ | ||
| "${TMP_GOPATH}" | ||
|
|
||
| touch "${REPOINFRA_ROOT}/vendor/BUILD" |
There was a problem hiding this comment.
FYI this isn't really needed for repo-infra (since we don't use the srcs rules here), but it makes sense if this script is used elsewhere, so I'm fine leaving it.
|
|
||
| # manually remove BUILD file for github.com/bazelbuild/buildtools/BUILD.bazel if it | ||
| # exists; there is a specific test_suite rule that breaks importing | ||
| rm -f ${REPOINFRA_ROOT}/vendor/github.com/bazelbuild/buildtools/BUILD.bazel |
There was a problem hiding this comment.
ugh, mixing go vendoring + bazel is the worst. :(
There was a problem hiding this comment.
I wonder if it's worth just.. find/deleting all BUILD/BUILD.bazel files in vendor/. @BenTheElder had suggested this before in test-infra, but I wasn't sure if there was possibly other rules in those files that would be useful in the context of go vendoring (when we strip tests and other files).
There was a problem hiding this comment.
This SGTM except that it would stink to need to regenerate all of these every update run, lots of update runs shouldn't touch vendor/ at all 🤔
There was a problem hiding this comment.
@BenTheElder We could perhaps use kube::util::has_changes_against_upstream_branch to check if there has been changes in vendor/ against upstream, and only trigger if there has. This is used for verifying core godeps only if there has been changes (or forced in CI)
There was a problem hiding this comment.
Deleting all existing BUILD files can be dangerous, since there might be genrules or other customizations we need.
I filed bazelbuild/bazel-gazelle#63 upstream to figure out a better long-term fix for this issue.
|
@ixdy PTAL |
|
I am waiting for this, or something like it, for kubernetes/kubernetes#57600 |
verify/update-bazel.sh
Outdated
| "${TMP_GOPATH}/bin/gazelle" fix \ | ||
| -build_file_name=BUILD,BUILD.bazel \ | ||
| -external=vendored \ | ||
| -proto=legacy \ |
There was a problem hiding this comment.
I think we can remove this line here. it's been removed from test-infra as well.
| - gometalinter --install | ||
| - go install ./... | ||
|
|
||
| script: | ||
| - verify/verify-boilerplate.sh --rootdir="$GOPATH/src/k8s.io/repo-infra" -v | ||
| - verify/verify-go-src.sh --rootdir "$GOPATH/src/k8s.io/repo-infra" -v | ||
| - buildifier -mode=check $(find . -name BUILD -o -name '*.bzl' -type f) | ||
| - buildifier -mode=check $(find . -name BUILD -o -name '*.bzl' -type f -not -wholename '*/vendor/*') |
There was a problem hiding this comment.
Caught the following error when building with vendor (https://travis-ci.org/kubernetes/repo-infra/builds/320873011):
$ buildifier -mode=check $(find . -name BUILD -o -name '*.bzl' -type f)
./vendor/github.com/bazelbuild/buildtools/build/build_defs.bzl # reformat
The command "buildifier -mode=check $(find . -name BUILD -o -name '*.bzl' -type f)" exited with 4.
There was a problem hiding this comment.
Ha, that's amazing.
(I will maybe go send them a PR to fix that. :)
There was a problem hiding this comment.
oh, per bazelbuild/buildtools#109, we should probably only be running buildifier on BUILD files, not *.bzl files, though this is fine as-is.
|
I have a few lingering questions but otherwise LGTM. Sorry for delay. |
|
@ixdy updated |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
(we should maybe switch from travis to prow and setup tide here) |
Also fixes path for
github.com/bazelbuild/buildtools/build