Version the CSI protobuf #74

akutz · 2017-08-10T18:39:36Z

This patch versions the generated CSI protobuf and provides a Travis-CI build step that verifies the protobuf is up-to-date before allowing any changes to be merged.

Additionally this patch also adds minimal Go language bindings in order to verify the specification. Finally, scaffolding was put into place in order to support Cxx language bindings in the future as well.

jdef · 2017-08-10T20:38:29Z

This patch also removes golang stub generation support. Other issues/PRs are trying to standardize proto stub generation: #68 #69
At some point in the near future I want the CI to test that the .proto file actually compiles -- losing that capability as part of this patch is unfortunate.

akutz · 2017-08-10T20:42:29Z

Hi @jdef,

You were invited but unable to join the call with @saad-ali, @jieyu, and myself yesterday. There is a plan to handle your concerns, and in fact I even mentioned your specific concern in our group chat in Slack a few hours back:

@jieyu summarized yesterday's meeting in this comment, but it looks like you were not tagged.

I hope this helps assuage any concerns you may have.

jdef · 2017-08-10T20:49:50Z

@akutz thanks for the summary. Looking forward to the PR that adds back .proto validation. If it doesn't happen over the next couple of days then I'd like to track the need via an issue.

akutz · 2017-08-10T20:52:44Z

Hi @jdef,

To be clear, it won't be validation but Go language binding generation. The validation @jieyu, @saad-ali, and I discussed includes using a bare-minimum Cxx and Go client to replicate the understood workflows against a mock plug-in, similar to CNI's methodology.

jdef · 2017-08-10T20:56:25Z

That's fine. The lost capability that I'm looking to get back is validation of the syntax of the .proto file, which we'll gain as soon as *any* bindings are generated in CI from the .proto file

…

On Thu, Aug 10, 2017 at 4:52 PM, Schley Andrew Kutz < ***@***.***> wrote: Hi @jdef <https://github.com/jdef>, To be clear, it won't be validation but Go language binding generation. The validation @jieyu <https://github.com/jieyu>, @saad-ali <https://github.com/saad-ali>, and I discussed includes using a bare-minimum Cxx and Go client to replicate the understood workflows against a mock plug-in, similar to CNI's methodology. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLHfDSxvtb_TTV2vZxFrZDgrjuOduks5sW22dgaJpZM4Oz0dV> .

jieyu

Can you update CONTRIBUTING.md about the flow of updating protobuf in the spec:

update the protobuf in spec.md
run make locally
commit the change and send a PR.

jieyu · 2017-08-10T19:06:16Z

Makefile


-build: $(CSI_PROTO) $(CSI_GOSRC)
+# the target for building the temporary csi protobuf file.


nits: Can you use full sentences for the comment. In other word, Capitalize the first word of a sentence and uses a period in the end. Ditto everywhere else in this file.

Hi @jieyu,

As I was the first to create comments in the Makefiles for this project, is this your personal preference or part of the project's style guidelines? What you see is my preference, but I will change it in accordance with a posted style manual. Thanks!

jieyu · 2017-08-10T21:06:22Z

README.md

-| `CSI_PROTO_DIR` | The path of the directory in which the protobuf and Go source files will be generated. If this directory does not exist it will be created. | `.` |
-| `CSI_PROTO_ADD` | A list of additional protobuf files used when building the Go source file | |
-| `CSI_IMPORT_PATH` | The package of the generated Go source | `csi` |
+This project contains the CSI [specification](spec.md) and


Please follow the markdown style guid here:
https://github.com/container-storage-interface/spec/blob/master/CONTRIBUTING.md#markdown-style

One sentence per line.

Hi @jieyu,

It is a single sentence. Do you mean I should not hard-wrap it? Sure, if that's this project's style. I prefer hard-wrapping for the sake of readability, but I will gladly adhere to the project's style guidelines.

I mean this is a single sentence, you should not wrap it. see the rationale here:
https://github.com/container-storage-interface/spec/blob/master/CONTRIBUTING.md#markdown-style

jieyu · 2017-08-10T21:07:33Z

Makefile


 clean:
-	rm -f $(CSI_PROTO) $(CSI_GOSRC)
+	rm -f $(CSI_PROTO)


Remove the tmp file as well?

Hi @jieyu,

Good catch.

jieyu · 2017-08-10T21:10:32Z

.travis.yml

-  - printf "%s" "$GITHUB_API_TOKEN" > "${HOME}/.gist"
-  - gist -d "https://travis-ci.org/container-storage-interface/spec/jobs/${TRAVIS_BUILD_ID}" *.{proto,go}
+# run make
+script: make


For travis CI for now (until we have validation tooling checked in), can we try to call protoc to generate the go file to capture any protobuf syntax errors?

Hi @jieyu,

That's not necessary. The other PR is almost on deck.

ok, but I do want to merge this first. The other might take a while to review. Sounds like not too much work here. Just call protoc in travis yaml?

Hi @jieyu,

You can then review them in tandem if you wish or I'll put them into the same PR. Your call. I don't want to reintroduce a lot of what I already pulled out.

Sounds good. Let's pull the other one into the same PR.

akutz · 2017-08-10T21:19:52Z

Hi @jdef,

FWIW it's already in progress with successful, forked builds. But the second PR will be dependent upon this PR. I will rebase the latter once this one is accepted.

jdef · 2017-08-11T13:33:14Z

lib/go/Makefile

+ifeq (,$(strip $(GOPATH)))
+GOPATH := $(HOME)/go
+else
+# If GOPATH is arleady set then update GOPATH to be its own


s/arleady/already/

akutz · 2017-08-11T17:41:52Z

Hi @jieyu,

Would you please mark this as approved? I've address all your suggested changes/fixes.

jieyu

Can you split this big patch into logically separate pieces. For instance, adding guideline for commit style should be in its own commit. See why we need to do this in the link below (probably add this to CONTRIBUTING.md as well).
https://kernelnewbies.org/PatchPhilosophy

You can separate this patch into the following logic changes:

Remove tailing white spaces in the spec
Adding guideline for commit message
Build system refactor

jieyu · 2017-08-11T18:14:05Z

protoc.mk

+PROTOC_ARCH := x86_32
+endif
+
+PROTOC := ./protoc


I don't like the way we build protoc all the time. For C++ projects like Mesos/grpc, it might already bundle protoc and probably do not want to build protoc again. I'd suggest we allow downstream project to pass in a path to protoc (pre-existed at some location).

If you do need a *.mk file for modularization, please put that under build/make subdirectory. I might be interested in adding CMake support under build/cmake

Hi @jieyu,

I do like it. It ensures a specific version.

I only made proto.mk external from the Go process in the case that you might want to re-use it. If that's not the case I'll move it into lib/go/Makefile directly. Thank you for the feedback.

jieyu · 2017-08-11T18:15:08Z

protoc.mk

+PROTOC_OS := osx
+endif
+
+PROTOC_ARCH := $(shell uname -m)


Instead of that, let's allow users to pass in the value directory. uname does not work on windows.

Hi @jieyu,

CSI does not support Windows today, so it is irrelevant.

jieyu · 2017-08-11T18:18:58Z

Makefile

+	diff "$@" "$?"
+else
+	diff "$@" "$?" > /dev/null 2>&1 || cp -f "$?" "$@"
+.PHONY: $(CSI_PROTO).tmp


Can you explain why this is a PHONY target?

Hi @jieyu,

Because it ensures that a local, stale temp file won't cause the protobuf from not being rebuilt. We discussed this yesterday on Slack.

jieyu · 2017-08-11T18:22:55Z

Makefile

+
+# If this is not running on Travis-CI then for sake of convenience
+# go ahead and update the language bindings as well.
+ifneq (true,$(TRAVIS))


Why not build library as well in CI? We need to capture library build error on CI as well, right?

Hi @jieyu,

Because I don't want the default target in the root Makefile to do recursive Make.

jieyu · 2017-08-11T18:29:02Z

lib/go/Makefile

+	go get $(@:$(GOPATH)/src/%=%)
+
+PROTOC_GO := $(PROTOC_GO_PKGS) $(PROTOC_GEN_GO)
+protoc-go: $(PROTOC_GO)


this is a PHONY target?

Hi @jieyu,

Yes and no. It's an alias.

jieyu · 2017-08-11T18:32:59Z

lib/go/Makefile

+ifeq (true,$(TRAVIS))
+	diff "$@" "$?"
+else
+	diff "$@" "$?" > /dev/null 2>&1 || cp -f "$?" "$@"


So this output will depends on the version of protoc as well as the plugins i think? I am not sure if there will be any compatibility issue or not. For instance, what if a downstream wants to use a specific version of protoc because of some other dependencies?

Hi @jieyu,

Then they can use that version. Please note that the version variable allows itself to be specified externally.

PROTOC_VER ?= 3.3.0

As for the plug-ins, I'm thinking I will vendor the Go pkg dependencies in lib/go and build the plug-in out of that to ensure a specific version. I'll use Glide to record to actual dependency manifest. Good call.

jieyu · 2017-08-11T18:33:25Z

lib/go/Makefile

+	go clean -i -v
+
+clobber:
+	rm -f $(CSI_GO) $(CSI_GO_TMP)


rm .build directory as well?

Hi @jieyu,

That's a good catch, thanks.

akutz · 2017-08-11T18:41:11Z

Hi @jieyu,

This patch was originally separate commits, but I collapsed them at your request. In fact both you and @jdef objected to losing the Go code generation. I do not think it is prudent to divide them up again.

jieyu · 2017-08-11T18:44:41Z

@akutz The merge was meant for the makefile refactor. Merging top level makefile change with golang binding changes. I am fine with that. But the commit style change and whitespace fix should be in separate commits.

jdef · 2017-08-11T18:44:50Z

This patch was originally separate commits...

@akutz I think Jie would like all the changes to remain as part of this PR, but that this PR itself should be broken up into logically distinct commits (that won't be squashed at merge time). @jieyu correct me if I'm wrong

akutz · 2017-08-11T18:48:28Z

Hi @jdef and @jieyu,

Guys, I am not going to spend time breaking this back up. Please feel free to do so. I wouldn't have even touched the spec.md file if not for adding two lines to the spec in order to specify the package. I did this because @jieyu did the same in a now-closed PR. I thought it was a good idea. I cannot edit that file without it updating the WS. As I pointed out in Slack. We had a whole conversation. I even cleared this with @jdef and @saad-ali. @jdef, please feel free to add that text via your editor and I'll reset my copy of spec.md.

Just... sigh Yeah. :)

akutz · 2017-08-11T18:51:16Z

And @jieyu, the merge was meant for a use case, not a single file refactor. I get that you're being careful with the repo, but please don't forget there is a purpose to provide a feature/use case here, not simply the refactor of a single file.

jieyu · 2017-08-11T18:52:16Z

@akutz at the very least, the style guide change should be a separate commit. Make sense?

akutz · 2017-08-11T18:54:03Z

@jieyu, why? What purpose does that serve? The update to the contribution file regarding proper procedure for updating the spec is irrelevant without the rest of the changes. Commits should be atomic features, and the remark in the contribution doc is pointless on its own.

akutz · 2017-08-11T18:55:24Z

Hi @jieyu,

Also, you keep referring to CONTRIBUTING.md as a style guide when it's far more than just that. You should break it up if you want to keep process separate from style. I recommend you look at http://rexray.readthedocs.io/en/stable/ and how I provide separate documentation for style, process, and build reference. Perhaps we could do something like that here?

akutz · 2017-08-11T18:58:18Z

Hi @jdef,

Could you please file a PR where you add the following to spec.md?

Container Storage Interface

This section describes the interface between COs and Plugins.
syntax = "proto3";
package csi;

It's around line 212. That way I can reset spec.md in this PR once I rebase off of your merged commit. It will remove all the whitespace changes. Thanks!

jdef · 2017-08-11T19:11:12Z

#78

…

On Fri, Aug 11, 2017 at 2:58 PM, Schley Andrew Kutz < ***@***.***> wrote: Hi @jdef <https://github.com/jdef>, Could you please file a PR where you add the following to spec.md? Container Storage Interface This section describes the interface between COs and Plugins. syntax = "proto3";package csi; It's around line 212. That way I can reset spec.md in this PR once I rebase off of your merged commit. It will remove all the whitespace changes. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#74 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLEOUwYA2Do7n333sQ9PUDbHGs3Eaks5sXKRLgaJpZM4Oz0dV> .

saad-ali · 2017-08-11T19:12:32Z

We need to ensure that this community is fosters an environment of collaboration while maintaining a high quality bar. I know we all have good intentions, and we'll sort this out. To that end, I sent out an invite for a meeting @ 1:30 PM PT today to see if we can resolve the open questions around this PR. Message me on slack (saad-ali) if you want an invite.

akutz · 2017-08-11T19:13:00Z

Hi @jdef,

It is going to, and is, fail(ing) because the existing build process places that text at the top of the file. You'd have to modify the existing code generation process.

jdef · 2017-08-11T19:21:12Z

thanks @akutz, fixed

akutz · 2017-08-11T20:14:01Z

Hi @jieyu,

I removed spec.md from the PR after rebasing off of master with @jdef's merged changes.

saad-ali · 2017-08-11T21:11:14Z

We had a productive meeting. Follow up items we agreed on:

Add comment to about what PHONY target is
Add a README to root of lib/
We are ok using go get for now, and not vendoring go dependencies.
Will leave make file for C as a hook for travis, and Jie can modify it in follow up PR.
uname --> switch to go version
proto buf should be major version (can be a follow up, @jdef)

akutz · 2017-08-11T22:24:07Z

Hi @saad-ali,

One slight change - I'm sticking with uname as CSI does not support Windows, so it doesn't matter if uname isn't a Windows command. Additionally, @jieyu may have honed in on that one command, but the fact is the Makefile uses dozens more GNU commands unavailable on Windows as well.

In other words -- supporting Windows is more than just replacing the use of the uname command.

jieyu · 2017-08-11T22:25:49Z

uname for now sounds good. Consider adding a TODO there.

akutz · 2017-08-11T22:28:24Z

Hi @jieyu,

Thank you for your suggestion, but I'm going to leave it as is. FWIW, I had actually made the change to go env and decided to revert it. I understand you zeroed in on uname, but the fact is it's a larger issue. Go look at how protoc is downloaded or any other number of commands. Supporting Windows is a much broader task than just one command.

jieyu

LGTM! Thanks @akutz !

jieyu · 2017-08-11T22:41:03Z

Makefile

+	diff "$@" "$?"
+else
+	diff "$@" "$?" > /dev/null 2>&1 || cp -f "$?" "$@"
+.PHONY: $(CSI_PROTO).tmp


Can you remove the PHONY target here for now as we discussed offline?

This patch versions the generated CSI protobuf and provides a Travis-CI build step that verifies the protobuf is up-to-date before allowing any changes to be merged. Additionally this patch also adds minimal Go language bindings in order to verify the specification. Finally, scaffolding was put into place in order to support Cxx language bindings in the future as well.

jdef approved these changes Aug 10, 2017

View reviewed changes

jieyu requested changes Aug 10, 2017

View reviewed changes

akutz force-pushed the feature/csi.proto branch from 72ab19f to 459e38f Compare August 10, 2017 21:21

akutz mentioned this pull request Aug 10, 2017

Language Bindings Skeleton #75

Closed

akutz force-pushed the feature/csi.proto branch 5 times, most recently from a98cf4a to d9cd7d3 Compare August 10, 2017 22:08

akutz mentioned this pull request Aug 11, 2017

Add build support for CXX consumers #69

Closed

jdef reviewed Aug 11, 2017

View reviewed changes

akutz force-pushed the feature/csi.proto branch from d9cd7d3 to 81353a7 Compare August 11, 2017 17:40

akutz force-pushed the feature/csi.proto branch 2 times, most recently from 865871b to 79c961f Compare August 11, 2017 17:46

jieyu requested changes Aug 11, 2017

View reviewed changes

akutz force-pushed the feature/csi.proto branch from 79c961f to 8b58152 Compare August 11, 2017 18:52

akutz force-pushed the feature/csi.proto branch from 8b58152 to f636064 Compare August 11, 2017 19:42

akutz force-pushed the feature/csi.proto branch 6 times, most recently from 861bdb8 to 6f1ce3a Compare August 11, 2017 22:41

jieyu approved these changes Aug 11, 2017

View reviewed changes

akutz force-pushed the feature/csi.proto branch from 6f1ce3a to 15419ea Compare August 11, 2017 22:49

jieyu merged commit 6606242 into container-storage-interface:master Aug 11, 2017

jieyu mentioned this pull request Aug 11, 2017

golang code generation #68

Closed


		build: $(CSI_PROTO) $(CSI_GOSRC)
		# the target for building the temporary csi protobuf file.

Version the CSI protobuf #74

Version the CSI protobuf #74

Conversation

akutz commented Aug 10, 2017 • edited Loading

jdef commented Aug 10, 2017

akutz commented Aug 10, 2017 • edited Loading

jdef commented Aug 10, 2017

akutz commented Aug 10, 2017

jdef commented Aug 10, 2017 via email

jieyu left a 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 Aug 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Aug 11, 2017

jieyu left a 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

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 Aug 11, 2017

jieyu commented Aug 11, 2017

jdef commented Aug 11, 2017

akutz commented Aug 11, 2017 • edited Loading

akutz commented Aug 11, 2017

jieyu commented Aug 11, 2017

akutz commented Aug 11, 2017

akutz commented Aug 11, 2017 • edited Loading

akutz commented Aug 11, 2017

Container Storage Interface

jdef commented Aug 11, 2017 via email

saad-ali commented Aug 11, 2017

akutz commented Aug 11, 2017

jdef commented Aug 11, 2017

akutz commented Aug 11, 2017

saad-ali commented Aug 11, 2017

akutz commented Aug 11, 2017

jieyu commented Aug 11, 2017

akutz commented Aug 11, 2017

jieyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Aug 10, 2017 •

edited

Loading

akutz commented Aug 10, 2017 •

edited

Loading

akutz Aug 10, 2017 •

edited

Loading

akutz commented Aug 11, 2017 •

edited

Loading

akutz commented Aug 11, 2017 •

edited

Loading