Dep Support #113

akutz · 2017-09-20T21:46:38Z

This patch introduces support for the Golang dependency management program dep. The changes include using a predetermined version of the gRPC plug-in for Go, protoc-gen-go.

cc @gpaul

akutz · 2017-09-20T21:49:40Z

Hi @gpaul,

It occurs to me I should probably add the following at the top of Gopkg.toml:

# The following packages are required because they're either build
# tools or are dependencies only if the "csi" package is already
# generated. However, if the "csi" package is removed (a valid
# condition), a "dep ensure" will not fetch the dependencies it
# no longer sees in the project's dependency graph. Including the
# dependencies in the "required" list ensures they're always
# downloaded.
required = [
  "github.com/golang/protobuf/protoc-gen-go",
  "golang.org/x/net/context",
  "google.golang.org/grpc"
]

If that's not present and someone removes the csi package and then performs a dep ensure everything will break. This is because the dependencies will no longer be detected during the update and dep will update the lock file as a zero-length file (aside from a comment).

The Makefile accounts for this by doing the following:

vendor: | $(DEP)
	$(DEP) ensure -v -vendor-only

The -vendor-only flag tells dep not to update the Gopkg.lock file -- just download what's in there. Thus if people use the make workflow there's nothing to fear. However, I should probably be careful and add the requires statement. I hate doing that as it will require those imports even if the project actually does stop using them.

Thoughts?

akutz · 2017-09-20T21:53:30Z

lib/go/Makefile

 ########################################################################
 ##                             PROTOC                                 ##
 ########################################################################

-# Only set PROTOC_VER if it has an empty value.


Hi @jieyu ,

I've always err'd on the side of caution and opted for the use of ifeq to determine if a variable is already set instead of ?=. I've always assumed my method is more performant since ?= is subject to recursive expansion (and still is in the case of the value as a result of a $(shell ...) command).

I made this change because it occurs to me if the value contains no variables or use of the $(shell ...) command then ?= probably doesn't incur the penalty of the would-be recursive expansion and is simpler and as effective as the previous method.

Do you know if this is true? Is ?= without variables in the value still subject to a check for recursive expansion everything the variable is referenced?

Hi @gpaul,

Do you have any advice or guidance related to the above question?

gnu make will short-circuit the recursive expand if it doesn't find a '$' in the value:
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n230

The stack trace as I read it would be:
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n194
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n230
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n415
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n457
http://git.savannah.gnu.org/cgit/make.git/tree/expand.c#n557
http://git.savannah.gnu.org/cgit/make.git/tree/variable.c#n1090

where v.recursive = recursive due to
http://git.savannah.gnu.org/cgit/make.git/tree/variable.c#n263
where recursive = 1 due to
http://git.savannah.gnu.org/cgit/make.git/tree/variable.c#n1420
and specifically
http://git.savannah.gnu.org/cgit/make.git/tree/variable.c#n1203
as the ?= case is handled by
http://git.savannah.gnu.org/cgit/make.git/tree/variable.c#n1195

Cool, thanks!

But seriously @gpaul, thank you for tracking this down. I know there are hundreds of locations where I've used the other pattern to ensure that things are more performant. Knowing this information is truly helpful.

akutz · 2017-09-20T22:51:13Z

This should address the issue encountered by @gpaul in #108.

gpaul · 2017-09-21T10:19:29Z

In what workflow will the csi package be removed? Is the idea to account for a Go project including the spec repo but generating their own csi.pb.go using for example gogoprotobuf?

If they're removing the csi package they might as well remove the lib/go package, too?

gpaul · 2017-09-21T10:25:15Z

lib/go/Makefile

+dep: $(DEP)
+endif
+
+vendor: | $(DEP)


Shouldn't $(DEP) be a hard dependency (ie., without the |) ?

Ah just realized that this would download dep anew every time as it's a PHONY dependency. If we let make do its job and not mark dep as PHONY then it shouldn't need to re-run that dependency every time.

Again, this is to handle the case where dep is a phony dependency to point to an existing version of the binary.

Also, it's an order only dependency because I don't care so much if the version of dep changes. It just needs to exist. That and vendor is a directory and you don't really want to compare a dir's mtime as Make does funny things with that. Order only dependencies are the best approach when depending on or being a dependency of a directory.

gpaul · 2017-09-21T10:33:21Z

lib/go/Makefile

+	chmod 0755 "$$DEP_BIN" && \
+	mv -f "$$DEP_BIN" "$@"
+ifneq (./dep,$(DEP))
+dep: $(DEP)


I'm not sure I follow. This would expand to

dep: ./dep

And if DEP=dep is given, this will expand to

dep: dep

which will likely break.

I wonder if we shouldn't do

DEP ?= dep ... .PHONY: $(DEP) $(DEP): ... ineq (dep, $(DEP)) .PHONY: dep dep: $(DEP) endif vendor: dep ...

And then, looking at that, I wonder how valuable it is to have DEP configurable?

If we drop the configurability this could be

... remove 'DEP ?= ./dep' line ... dep: ... current contents of $(DEP) vendor: dep dep ...

This is really nit-picky and I'm more than happy if this is merged as is and we revisit this with a use case if it breaks at some point.

The point is to allow you to specify an absolute path to an existing dep binary. You would never specify just "dep" as it would be the same binary that gets downloaded.

akutz · 2017-09-21T14:36:59Z

Hi @gpaul,

The Makefile allows the csi directory to be complete removed as its contents are wholly generated. The process should account for that.

gpaul · 2017-09-21T14:47:33Z

@akutz Thanks but I'm still quite confused. Removed from where? By whom and under what circumstances? What is "the process"?

akutz · 2017-09-21T15:12:49Z

Hi @gpaul,

The process is to run make, but if someone accidentally or on purpose nuked the csi package knowing it is generated l, the make process wouldn't put it back since dep will, by default, remove any dependencies it no longer sees in the graph.

akutz · 2017-09-21T15:15:02Z

By the way @gpaul, check out #93. There's a reason for the empty, top level package. Otherwise the process of importing the package registers the gRPC types automatically, which may not be desired.

gpaul · 2017-09-21T15:18:28Z

Thanks @akutz

the make process wouldn't put it back

I understand perfectly now.

akutz · 2017-09-21T16:41:45Z

Hi @gpaul,

FWIW, this all stems from the fact that dep doesn't consider solely the lock file like Glide. Dep considers the project's graph to be the lock file and a real-time examination of the project's dependencies (via the same tools used by go list). I actually filed golang/dep#1104 regarding a result of this behavior:

$ dep ensure -update my.package/import
Gopkg.toml and Gopkg.lock are out of sync. Run a plain dep ensure to resync them before attempting to -update

The above command fails with the above error even if the TOML and Lock files have not changed. The reason is described in the aforementioned issue -- this is the actual dep model:

This behavior has changed at my request as it's sometimes desirable to edit the TOML file manually before running dep ensure -update or allowing the project imports to change. Now a warning is displayed.

However, the crucial point here is that if the csi package is missing, running dep ensure will look at both the lock file and the project's imports to determine what packages need to be fetched. Even if the lock file lists all those packages pulled in through the now absent csi package, the project will no longer require them and dep not only doesn't pull them, but updates the lock file to remove them.

Hence the Makefile doing dep ensure -vendor-only. This updates the project from the lock file only and does not update the lock file with the new project scan result.

akutz · 2017-09-21T16:43:27Z

Hi @gpaul,

I've just rebased this PR to remove this from the Makefile:

ifneq (./dep,$(DEP))
dep: $(DEP)
endif

It occurred to me that I only had this when I used to use the dep target as a dependency for other targets. I no longer do that, so this phony target is no longer needed.

gpaul · 2017-09-21T17:04:41Z

FWIW

Worth a lot (to me at least)!

I'll have to give it some more thought, but my immediate thought is that your initial suggestion re: required sounds pretty good.

cpuguy83 · 2017-10-03T13:34:55Z

lib/go/Gopkg.toml

+]
+
+[[constraint]]
+  branch = "master"


Should we target a specific revision? These can be updated periodically if needed.

cpuguy83 · 2017-10-03T13:35:03Z

lib/go/Gopkg.toml

+  name = "github.com/golang/protobuf"
+
+[[constraint]]
+  branch = "master"


cpuguy83 · 2017-10-03T13:45:18Z

lib/go/Makefile

+DEP_BIN := dep-$$GOHOSTOS-$$GOHOSTARCH
+DEP_URL := https://github.com/golang/dep/releases/download/v$(DEP_VER)/$$DEP_BIN
+
+$(DEP):


I don't spend much time designing makefiles, but a little iffy in automatically installing a binary for people. We can add how to get dep in the README or in a contributing guide.

akutz · 2017-10-03T14:22:49Z

Hi @cpuguy83,

Thank you very much for your comments and thorough examination of this PR! A few things:

The Gopkg.toml file should not target a specific revision unless you want to override something. That is what the lock file is for. The branch constraint is not sliding -- not unless you update the lock file. I recommend reading up on the dep tool and its TOML and lock files. I am a Glide power-user, and I have to say, the Dep tool is the natural successor to Glide.
We're not installing a binary. We're placing a binary in the local directory to use. It's the dep tool. Makefiles do this all the time.

Thank you again!

cpuguy83 · 2017-10-03T14:34:34Z

I'm still iffy on the binary. We are taking something from the internet and executing it without confirmation. It wouldn't be so bad if we had a checksum to work against, but even then I think it's best to have the developer install dep.

akutz · 2017-10-03T14:38:50Z

Hi @cpuguy83,

I understand your concern. However, when you go get something you’re also often installing a binary from source. The Makefile is such that it won’t install dep if you define its location ahead of time. That’s what the ?= means.

akutz · 2017-10-23T17:53:14Z

Hi @jdef / @saad-ali / @jieyu,

Please indicate why this PR is not yet merged so the issue(s) may be addressed. Thank you.

cpuguy83 · 2017-10-23T18:33:43Z

I am majorly 👎 on automatically downloading a binary and executing it. I know it only does it if dep is missing, but this is really kinda gross.

I see we are also doing this with protoc, so... 🤷‍♂️
Everything else looks ok.

akutz · 2017-10-23T18:59:48Z

Hi @cpuguy83,

I understand your concern, but we’ve already been doing this and it’s the only way to ensure a consistent and deterministic result for the generated code. Otherwise it’s up to those modifying things to configure the required versions.

cpuguy83 · 2017-10-23T20:17:24Z

Wouldn't CI be able to check for this?

akutz · 2017-10-23T20:20:03Z

Hi @cpuguy83,

Absolutely. And it does. The purpose though is to make it easy for people modifying the spec to do just that. Look, I agree with your concerns. I do. In a lot of my own projects I manage the build dependencies in the .travis.yml file, and I expect my colleagues to know how to install things locally. However, we're trying to be as friendly as possible in this case.

jdef · 2017-10-27T06:46:36Z

lib/go/Makefile

+	chmod 0755 "$$DEP_BIN" && \
+	mv -f "$$DEP_BIN" "$@"
+
+vendor: | $(DEP)


maybe a compromise would be a vendor: check-dep dependency, where check-dep tests for the presence of dep in the $PATH and if it's missing exits the build?

if we want to stay friendly, we could also have a target dep: | $(DEP) so that a user that's missing dep could simply invoke make dep to pull it down.

Or we merge as is and handle it later since this adds value without detracting from any. This needs to be deterministic, period. Or bindings could be off since the tool used to grab the eps may influence the eps.

This patch introduces support for the Golang dependency management program `dep`. The changes include using a predetermined version of the gRPC plug-in for Go, `protoc-gen-go`.

akutz · 2018-02-09T07:27:42Z

Ping.

This was opened in September of last year. Please either merge or close this. Thank you.

cpuguy83 · 2018-02-09T18:09:30Z

@akutz I don't want to have an implicit pull down of a binary.
Having a make dep and a make check-dep and output the correct error messages when dep is not available would fix my concerns.

akutz · 2018-02-09T18:14:34Z

Hi @cpuguy83,

I understand your concern, but this is not a new behavior, and your reluctance is blocking a needed fix. We've already encountered multiple cases where the bindings are not generated correctly due to incompatible dependencies, such as dep, protoc, etc. The Makefile has to provide the dependencies at the correct version or it's not useful for local devs or CI. If you will not consent to this then I will close the PR. I or someone else can refactor the build process to no longer use Travis-CI's containerized infrastructure and instead use their slower, VM infrastructure that supports Docker and rely on the latter for generating bindings.

cpuguy83 · 2018-02-09T20:06:30Z

@akutz Breaking it more because it's already broken is not the best argument.
This is adding a new thing to download dep and execute it.

If we want to make sure that the correct protos are generated then we need to verify it in CI.
I'm not sure I understand why any of this requires docker support.

akutz · 2018-02-09T20:19:16Z

Hi @cpuguy83,

Breaking it more because it's already broken

I disagree with the idea it's broken at all.

If we want to make sure that the correct protos are generated then we need to verify it in CI.

We do today Brian, that's the point. It's troublesome, it seems, for people generating them locally when they don't have the correct dependencies in place. This PR ensures all moving parts are removed.

I'm not sure I understand why any of this requires docker support.

Travis-CI containerized builds do not allow Docker. They're much faster than Travis-CI VM builds that do allow Docker. See #181 where I created a Docker image that can be used to generate the bindings locally or on Travis. The issue is that it would mean switching Travis-CI to use the slower VM builds.

cpuguy83 · 2018-02-09T20:30:22Z

I understand the issues with sudo mode in travis, but my question is why would we need sudo mode?

akutz · 2018-02-09T20:37:03Z

Hi @cpuguy83,

Because it makes Docker available to the build and thus Docker could be used to generate the language bindings with no dependencies. Does that make sense? If you see #181, the Docker image akutz/csi-golang can be used to generate a proto file or Go language bindings with no host dependencies (other than Docker). But it would require VM builds on Travis.

cpuguy83 · 2018-02-09T20:44:33Z

@akutz I think we can get the correct versions for CI without necessitating that CI runs a docker image.

akutz · 2018-02-09T20:48:24Z

Hi @cpuguy83,

Perhaps I'm not clear, but the goal is for devs to generate the bindings locally and then commit it and push with the PR. The CI environment then validates the generated bindings. Because we've encountered problems where devs do not have the correct dependencies locally, the Makefile ensures that they do by removing any possibility of mismatched versions. Transitioning to using Docker to generate the bindings would produce a similarly deterministic means of generating the bindings both locally and on the CI platform. Today that deterministic method is achieved via the Makefile.

cpuguy83 · 2018-07-18T19:05:02Z

Of course I said it seemed like the go community was moving to dep and the go team releases vgo.

saad-ali · 2018-11-07T23:35:26Z

We plan to use go modules in the future, so this is not needed anymore.

akutz commented Sep 20, 2017

View reviewed changes

gpaul reviewed Sep 21, 2017

View reviewed changes

gpaul mentioned this pull request Sep 21, 2017

spec: Rename handle to volume_handle in VolumeInfo. #114

Closed

akutz force-pushed the feature/dep branch from 38d3f5e to 59f8b79 Compare September 21, 2017 16:42

akutz force-pushed the feature/dep branch from 59f8b79 to 417bfd6 Compare September 21, 2017 16:44

cpuguy83 reviewed Oct 3, 2017

View reviewed changes

jdef reviewed Oct 27, 2017

View reviewed changes

akutz mentioned this pull request Jan 31, 2018

Generating Go Bindings with Docker #180

Closed

Dep Support

439e6ab

This patch introduces support for the Golang dependency management program `dep`. The changes include using a predetermined version of the gRPC plug-in for Go, `protoc-gen-go`.

akutz force-pushed the feature/dep branch from 417bfd6 to 439e6ab Compare February 9, 2018 07:26

saad-ali closed this Nov 7, 2018

Dep Support #113

Dep Support #113

Conversation

akutz commented Sep 20, 2017

akutz commented Sep 20, 2017

akutz Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpaul Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Sep 20, 2017

gpaul commented Sep 21, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Sep 21, 2017

gpaul commented Sep 21, 2017

akutz commented Sep 21, 2017

akutz commented Sep 21, 2017

gpaul commented Sep 21, 2017

akutz commented Sep 21, 2017

akutz commented Sep 21, 2017

gpaul commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Oct 3, 2017

cpuguy83 commented Oct 3, 2017

akutz commented Oct 3, 2017

akutz commented Oct 23, 2017

cpuguy83 commented Oct 23, 2017

akutz commented Oct 23, 2017

cpuguy83 commented Oct 23, 2017

akutz commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Feb 9, 2018

cpuguy83 commented Feb 9, 2018

akutz commented Feb 9, 2018 • edited Loading

cpuguy83 commented Feb 9, 2018

akutz commented Feb 9, 2018

cpuguy83 commented Feb 9, 2018

akutz commented Feb 9, 2018

cpuguy83 commented Feb 9, 2018

akutz commented Feb 9, 2018

cpuguy83 commented Jul 18, 2018

saad-ali commented Nov 7, 2018

akutz Sep 20, 2017 •

edited

Loading

gpaul Sep 21, 2017 •

edited

Loading

gpaul commented Sep 21, 2017 •

edited

Loading

akutz commented Feb 9, 2018 •

edited

Loading