Use dep v0.4.1 in kubernetes/test-infra #6443

ixdy · 2018-01-25T18:45:39Z

It does seem like the new release of dep does a better job of removing unused libraries.

In this PR, I enabled the new prune options (I turned off pruning non-go files, because Bazel).

I then ran dep ensure followed by hack/update-bazel.sh - I didn't run any of @fejta's other dep scripts in hack/. It seems like it removed some dependencies that the scripts missed, but potentially added some it had removed?

Amazingly, bazel build ... && bazel test ... still works.

fixes #5488

fejta · 2018-01-25T18:48:05Z

IMO we should simplify update-deps.sh (remove the prune command, and the drop-dep calls) and still use it.

ixdy · 2018-01-25T18:51:36Z

For fun, I ran hack/prune-libraries.sh --fix and added a commit with its results:

$ hack/prune-libraries.sh --fix
Unused libraries:
  DEAD: //vendor/google.golang.org/appengine/urlfetch:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/urlfetch:go_default_library
  DEAD: //vendor/google.golang.org/appengine:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/modules:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/app_identity:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/remote_api:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/log:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/datastore:go_default_library
  DEAD: //vendor/google.golang.org/appengine/internal/base:go_default_library
  DEAD: //vendor/golang.org/x/text/internal/ucd:go_default_library
  DEAD: //vendor/golang.org/x/text/internal/triegen:go_default_library
  DEAD: //vendor/golang.org/x/text/internal/gen:go_default_library
  DEAD: //vendor/golang.org/x/text/unicode/cldr:go_default_library
  DEAD: //vendor/github.com/spf13/cobra/cobra/cmd:go_default_library
  DEAD: //vendor/github.com/petar/GoLLRB/llrb:go_default_library
Cleaning up unused dependencies...
INFO: Empty results
All remaining go libraries are alive

ixdy · 2018-01-25T18:55:21Z

And I pushed one more commit which simplifies hack/update-deps.sh.

real    0m50.324s
user    0m15.784s
sys     0m5.880s

We probably need some way to ensure that the right version of dep is being used.

stevekuznetsov · 2018-01-25T21:09:57Z

Simple solution would be to put the script in a presubmit and validate no changes occur?

BenTheElder · 2018-01-26T00:14:32Z

/meow box

krzyzacy · 2018-01-26T00:19:47Z

/joke

k8s-ci-robot · 2018-01-26T00:19:48Z

@krzyzacy: My wife is on a tropical fruit diet, the house is full of stuff. It is enough to make a mango crazy.

In response to this:

/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

BenTheElder · 2018-01-26T00:43:35Z

/meow boxes

k8s-ci-robot · 2018-01-26T00:43:36Z

@BenTheElder:

In response to this:

/meow boxes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta

/lgtm
/hold

We should consider integrating prune-libraries.sh into update-bazel.sh

fejta · 2018-01-26T01:42:16Z

hack/update-deps.sh

 hack/update-bazel.sh
-drop-dep vendor/golang.org/x/text/language vendor/golang.org/x/text/internal


So amazing! Drop the drop-dep method?

ixdy · 2018-01-26T02:30:57Z

I was able to vendor dep and get it to run under bazel run, if we want to ensure that hack/update-deps.sh is consistent for everyone.

ixdy · 2018-01-26T02:46:56Z

Rebased and added dep to vendor/, along with teaching hack/prune-libraries.sh how to ignore vendored commands like this.

BenTheElder · 2018-01-26T06:03:19Z

fixes #5488

ixdy · 2018-01-26T22:11:01Z

I guess we don't actually have any presubmits checking this, yet. We should figure out how to do that... I'm not sure how to query bazel from inside a test running under bazel, though.

ixdy · 2018-01-26T22:46:06Z

I have a somewhat clever way of making dep work well under bazel run, similar to the approach gazelle used. Will open a PR against repo-infra. (Still not sure about making the verify-deps script run well under bazel.)

stevekuznetsov · 2018-01-26T23:26:34Z

So are we generally expecting PR reviewers to notice a huge diff in vendor/ and enforce that?

stevekuznetsov · 2018-01-26T23:27:47Z

/meow space

k8s-ci-robot · 2018-01-26T23:27:47Z

@stevekuznetsov:

In response to this:

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

BenTheElder · 2018-01-29T21:20:21Z

Any updates on this? imho we should get this finished and in and follow-up with a presubmit later.

ixdy · 2018-01-29T21:21:45Z

waiting on final approval for kubernetes/repo-infra#60.

BenTheElder · 2018-01-29T21:26:23Z

Seems I was only looking for confirmation on nits. Maybe we should merge than and nits can be addressed later?

ixdy · 2018-01-29T22:18:42Z

OK, this PR is officially ginormous.

I've added several additional commits:

added a dependency on kubernetes/repo-infra for workspace_binary, which means you can now bazel run //:dep -- and get the right version
updated the docs around dependencies to mention version, how to use bazel run, and the need to run hack/update-deps.sh too
created a simple prow job (copied from verify-config) which runs hack/verify-deps.sh, but only when anything deps-related has changed

PTAL

BenTheElder · 2018-01-29T22:19:32Z

OK, this PR is officially ginormous.

We need a new size label? 😂

k8s-ci-robot · 2018-01-30T06:34:09Z

@BenTheElder:

In response to this:

scripts, bazel, dep config, presubmit LGTM, vendor/ == 🤷‍♂️

/lgtm
/hold
/shrug
/meow space
/woof
/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2018-01-30T06:34:09Z

@BenTheElder: Do you want a brief explanation of what an acorn is? In a nutshell, it's an oak tree.

In response to this:

scripts, bazel, dep config, presubmit LGTM, vendor/ == 🤷‍♂️

/lgtm
/hold
/shrug
/meow space
/woof
/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

BenTheElder

scripts, bazel, dep config, presubmit LGTM. vendor/ == 🤷‍♂️

/lgtm
/hold
/shrug
/meow space
/woof
/joke

k8s-ci-robot · 2018-01-30T06:34:10Z

@BenTheElder:

In response to this:

scripts, bazel, dep config, presubmit LGTM, vendor/ == 🤷‍♂️

/lgtm
/hold
/shrug
/meow space
/woof
/joke

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2018-01-30T06:34:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, fejta, ixdy

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [BenTheElder,fejta,ixdy]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

BenTheElder · 2018-01-30T06:34:56Z

better yet since this is all about vendor and sandboxing:
/meow boxes

k8s-ci-robot · 2018-01-30T06:34:57Z

@BenTheElder:

In response to this:

better yet since this is all about vendor and sandboxing:
/meow boxes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ixdy · 2018-01-30T19:12:11Z

ok, I'm reasonably sure this works.

/unhold

ixdy · 2018-01-30T19:17:35Z

I'll note that the verify job I added here is only running hack/verify-deps.sh, which doesn't call dep, just the hack/prune-libraries.sh script. I'm not sure if we want to add a dep ensure step to this check, much like the kubernetes/kubernetes verify-deps.sh check.

k8s-ci-robot · 2018-01-30T19:20:11Z

@ixdy: I updated Prow config for you!

In response to this:

It does seem like the new release of dep does a better job of removing unused libraries.

In this PR, I enabled the new prune options (I turned off pruning non-go files, because Bazel).

I then ran dep ensure followed by hack/update-bazel.sh - I didn't run any of @fejta's other dep scripts in hack/. It seems like it removed some dependencies that the scripts missed, but potentially added some it had removed?

Amazingly, bazel build ... && bazel test ... still works.

fixes #5488

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ixdy · 2018-01-30T19:30:52Z

The verify job appears to be basically doing the right thing - it at least verifies that you've run hack/prune-libraries.sh, hopefully through hack/update-deps.sh.

ixdy force-pushed the godep-prune-options branch from dd7db8b to 25eaf88 Compare January 25, 2018 18:53

fejta reviewed Jan 26, 2018

View reviewed changes

k8s-ci-robot assigned fejta Jan 26, 2018

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

ixdy added 5 commits January 25, 2018 18:35

Add prune options to Gopkg.toml

beb62c4

Simplify hack/update-deps.sh

ae8ff3a

Run hack/update-deps.sh

5076bf8

Add constraint on dep

490bd4f

Run hack/update-deps.sh

01fa5e6

ixdy force-pushed the godep-prune-options branch from 585cd95 to 64d6866 Compare January 26, 2018 02:45

ixdy force-pushed the godep-prune-options branch from 64d6866 to fd73b49 Compare January 26, 2018 02:47

Make hack/update-deps.sh use vendored dep through bazel run

ecb0383

ixdy force-pushed the godep-prune-options branch from fd73b49 to ecb0383 Compare January 26, 2018 02:48

ixdy changed the title ~~Use new prune options in dep 0.4.1~~ Use dep v0.4.1 in kubernetes/test-infra Jan 26, 2018

ixdy added 2 commits January 29, 2018 14:04

Add a workspace_binary rule for dep

130e82d

Update dep documentation

1e0ab9f

Add pull-test-infra-verify-deps prow job

73883f7

ixdy requested review from krzyzacy and michelle192837 as code owners January 29, 2018 22:19

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Jan 30, 2018

BenTheElder approved these changes Jan 30, 2018

View reviewed changes

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

k8s-ci-robot merged commit f03c297 into kubernetes:master Jan 30, 2018

ixdy deleted the godep-prune-options branch May 15, 2018 23:52

		hack/update-bazel.sh
		drop-dep vendor/golang.org/x/text/language vendor/golang.org/x/text/internal

Use dep v0.4.1 in kubernetes/test-infra #6443

Use dep v0.4.1 in kubernetes/test-infra #6443

Conversation

ixdy commented Jan 25, 2018 • edited Loading

fejta commented Jan 25, 2018

ixdy commented Jan 25, 2018 • edited Loading

ixdy commented Jan 25, 2018

stevekuznetsov commented Jan 25, 2018

BenTheElder commented Jan 26, 2018

krzyzacy commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

BenTheElder commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

fejta left a comment

Choose a reason for hiding this comment

fejta Jan 26, 2018

Choose a reason for hiding this comment

ixdy Jan 26, 2018

Choose a reason for hiding this comment

ixdy commented Jan 26, 2018

ixdy commented Jan 26, 2018

BenTheElder commented Jan 26, 2018

ixdy commented Jan 26, 2018

ixdy commented Jan 26, 2018

stevekuznetsov commented Jan 26, 2018

stevekuznetsov commented Jan 26, 2018

k8s-ci-robot commented Jan 26, 2018

BenTheElder commented Jan 29, 2018

ixdy commented Jan 29, 2018

BenTheElder commented Jan 29, 2018

ixdy commented Jan 29, 2018

BenTheElder commented Jan 29, 2018

k8s-ci-robot commented Jan 30, 2018

k8s-ci-robot commented Jan 30, 2018

BenTheElder left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 30, 2018

k8s-ci-robot commented Jan 30, 2018

BenTheElder commented Jan 30, 2018

k8s-ci-robot commented Jan 30, 2018

ixdy commented Jan 30, 2018

ixdy commented Jan 30, 2018

k8s-ci-robot commented Jan 30, 2018

ixdy commented Jan 30, 2018

ixdy commented Jan 25, 2018 •

edited

Loading

ixdy commented Jan 25, 2018 •

edited

Loading

BenTheElder left a comment •

edited

Loading