No more dep in makefile/build - long live dep through vendor/ by ldemailly · Pull Request #3678 · istio/istio

ldemailly · 2018-02-22T05:17:22Z

Follow up on having vendor/ checked in (as submodule)

We don't need to run a bunch of dep commands anymore, or build/find dep for that matter

This should shave some time from the build, not counting the simpler Makefile and script

We can later make new targets to automate
https://github.com/istio/istio/wiki/Vendor-FAQ#how-do-i-add--change-a-dependency
(for instance we can follow up with #3679)

Follow up on having vendor/ checked in (as submodule) We can later make new targets to automate https://github.com/istio/istio/wiki/Vendor-FAQ#how-do-i-add--change-a-de pendency

ldemailly · 2018-02-22T05:28:32Z

Makefile

-	unset GOOS && CGO_ENABLED=1 go get -u github.com/golang/dep/cmd/dep
-
 ${GOLINT}:
 	unset GOOS && CGO_ENABLED=1 go get -u github.com/golang/lint/golint


btw we should probably not get golint that way either - I think bin/linter.sh uses an image ...

There were some other linter targets that I think have since been removed so this target (and the GOLINT variable) might now be unused (though I didn't search the other .mk files that are imported by this one).

indeed

ldemailly-macbookpro:istio ldemailly$ git grep GOLINT Makefile:GOLINT := $(shell which golint 2>/dev/null || echo "${ISTIO_BIN}/golint" ) Makefile:${GOLINT}:

will drop it but I want to see all builds green first before touching the pr again and cancelling the inflight ones

ldemailly · 2018-02-22T05:46:45Z

circleci builds and the part of prow that would need dependencies seem happy with this pr 🎉
wtb "/lgtm"

ldemailly · 2018-02-22T05:53:25Z

before (on master, dependencies circleci build) : 1min50s init step

after: 0min02sec !

costinm · 2018-02-22T05:58:07Z

Makefile

-	$(Q) ${DEP} ensure -update
-
-# Generates the current status of the dependencies. Does not update the deps.
-depend.status:


Please keep those targets, maybe redirect the output to the OUT directory. It's something we may want to include somewhere in the release or artifacts.

i'd prefer to not keep all the DEP variable and build magic just to have (even if useful) graph target (one need to install graphviz anyway, then it's just 1 command dep status -dot | dot -T png > dep.png ?)

I think that should go in follow ups like #3679 but if you prefer I can add it back as a raw target:

depend.status: dep status -dot | dot -T png > dep.png

lmk

I'm adding it back because when I tried it complained about input-digest mismatch
so it's useful for that

costinm

Great !!! Thanks, I forgot about dep.

Can you make sure that Gopkg.lock is still saved somewhere - and included in the release ? It is very useful to also have the version and list of each dep, even if the source is preserved.

ldemailly · 2018-02-22T06:06:18Z

Gopkg.lock is checked in, that's not changing (though #3679 would be good to have to make sure it doesn't go stale/out of sync with vendor-istio)

also ideally there would be some sort of commit hook / alert on changes to that file and vendor/

mattdelco

As a change for deleting dead/dated/unused code it seems okay to me.

On a broader scale it'd be nice to get rid of either "depend" or "init" since they're just aliases for each other and tend to complicate things and add confusion.

I did tend to turn a blind eye to the state of the dependencies. E.g. "default"'s first dependency is "depend", but its second dependency ("build") also has a dependency on "depend" so "default" doesn't need to cite "depend".

My current mindset is that anything that's cited as a dependency should be an actual file/path (i.e., nothing .PHONY) but if this was fully adopted then nothing would rebuild when there's a source code change (and vendor/ wouldn't get updated). Now that go 1.10 is out I don't know if we're at a point where we can try letting make defer to go on what needs to be rebuilt (and optionally git on whether to update vendor).

bin/init.sh is now just doing a download and a few file copies so it's a good candidate to port into the Makefile. If handling wget vs curl is really an issue then another option is to just compile a simple go app to do the download.

Anyway, I'm not expecting you or this change to do all that, but I figured I might as well comment on what came to mind as I tried to look at things holistically.

mattdelco · 2018-02-22T06:05:06Z

Makefile

-
-# Target to update the Gopkg.lock with latest versions.
-# Should be run when adding any new dependency and periodically.
-depend.update: ${DEP} ; $(info $(H) ensuring dependencies are up to date...)


A few lines above (where github won't let me add a comment, sigh), there's a reference to "must manually call dep.update" that probably meant "depend.update", which is now deleted in this change.

updating that part

ldemailly · 2018-02-22T06:53:35Z

For the record I was trying to add a working "depend.update" because of some other pr issue in #oncall but was having random errors again like:

$ dep ensure
Solving failure: No versions of gopkg.in/validator.v2 met constraints:
	v2: Could not introduce gopkg.in/validator.v2@v2, as its subpackage gopkg.in/validator.v2 does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)
	v2: Could not introduce gopkg.in/validator.v2@v2, as its subpackage gopkg.in/validator.v2 does not contain usable Go code (*build.NoGoError).. (Package is required by (root).)

I fixed them by putting deletion of the .lock in that target and removed a bunch of files in my go path it took a very long time but it worked. definitely something we should do on a nightly or weekly build rather than by hand

guptasu · 2018-02-22T07:04:42Z

/lgtm

istio-merge-robot · 2018-02-22T07:05:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: guptasu
We suggest the following additional approver: kyessenov

Assign the PR to them by writing /assign @kyessenov in a comment when ready.

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

Details

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

OWNERS

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

ldemailly · 2018-02-22T07:16:08Z

matt, yes I am trying to do only minor surgery in this PR on our makefile but I agree with you.

istio-merge-robot · 2018-02-22T07:25:15Z

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

nmittler · 2018-04-24T00:50:38Z

@ldemailly I was just chatting with @costinm and he's thinking that this PR incorrectly removed the --update from the depend.update target. I'm going to put a PR together to revert that target.

ldemailly · 2018-04-24T01:19:19Z

it wasn't actually used before by people and this and subsequent PRs (adding DEPARGS) let one choose what happens, it's gradual from just dep ensure which is the minimally invasive command, to picking a specific package to update to recreating the whole lock (depend.update.full)

We have several depend.* targets that to various things, wrapping dep ensure. This has caused confusion and has also led to the breakage of the depend.update target. PR istio#3678 changed the behavior of `depend.update` to remove the `--update` option when running `dep ensure`. This PR removes all of the depend.* targets and replaces them with the raw commands in the CircleCI yaml.

We have several depend.* targets that to various things, wrapping dep ensure. This has caused confusion and has also led to the breakage of the depend.update target. PR #3678 changed the behavior of `depend.update` to remove the `--update` option when running `dep ensure`. This PR removes all of the depend.* targets and replaces them with the raw commands in the CircleCI yaml.

We have several depend.* targets that to various things, wrapping dep ensure. This has caused confusion and has also led to the breakage of the depend.update target. PR istio#3678 changed the behavior of `depend.update` to remove the `--update` option when running `dep ensure`. This PR removes all of the depend.* targets and replaces them with the raw commands in the CircleCI yaml.

* wip: write status to Segments * attempt to workaround test flake caused by fake client bug * nolint: lll for gen code * rbac * apply gen

No more dep in makefile/build - long live dep through vendor/

2ddf6fc

Follow up on having vendor/ checked in (as submodule) We can later make new targets to automate https://github.com/istio/istio/wiki/Vendor-FAQ#how-do-i-add--change-a-de pendency

ldemailly requested review from a team February 22, 2018 05:17

googlebot added the cla: yes label Feb 22, 2018

ldemailly requested review from costinm and mattdelco February 22, 2018 05:17

Removed a couple more targets

c7e9c9c

ldemailly commented Feb 22, 2018

View reviewed changes

costinm reviewed Feb 22, 2018

View reviewed changes

costinm approved these changes Feb 22, 2018

View reviewed changes

Remove moar

185eec8

mattdelco reviewed Feb 22, 2018

View reviewed changes

Code review + dep update as input hash were out of sync

bd73545

ldemailly mentioned this pull request Feb 22, 2018

vendor associated with change to toml file https://github.com/istio/i… istio/old_vendor-istio_repo#5

Merged

Split make depend.update into .full and regular

9b6d271

istio-testing assigned guptasu Feb 22, 2018

istio-testing added the lgtm label Feb 22, 2018

ldemailly added the approved label Feb 22, 2018

ldemailly merged commit 088dfd7 into master Feb 22, 2018

ldemailly deleted the dep_clean branch February 22, 2018 07:34

ldemailly mentioned this pull request Feb 22, 2018

auto link toml/lock file with vendor-istio to check consistency #3679

Closed

ldemailly mentioned this pull request Feb 22, 2018

RFC: Commit vendor #2691

Closed

nmittler mentioned this pull request Apr 24, 2018

Remove depend.* targets #5148

Merged

PetrMc pushed a commit to PetrMc/istio-petrmc-upstream-fork that referenced this pull request Jan 14, 2026

segments: report status (istio#3678)

ea23dd4

* wip: write status to Segments * attempt to workaround test flake caused by fake client bug * nolint: lll for gen code * rbac * apply gen

Conversation

ldemailly commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattdelco Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldemailly commented Feb 22, 2018

Uh oh!

costinm Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 22, 2018

Uh oh!

mattdelco left a comment

Choose a reason for hiding this comment

Uh oh!

mattdelco Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Feb 22, 2018

Uh oh!

guptasu commented Feb 22, 2018

Uh oh!

istio-merge-robot commented Feb 22, 2018

Uh oh!

ldemailly commented Feb 22, 2018

Uh oh!

istio-merge-robot commented Feb 22, 2018

Uh oh!

nmittler commented Apr 24, 2018

Uh oh!

ldemailly commented Apr 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ldemailly commented Feb 22, 2018 •

edited

Loading

ldemailly Feb 22, 2018 •

edited

Loading

ldemailly commented Feb 22, 2018 •

edited

Loading

ldemailly Feb 22, 2018 •

edited

Loading