Add Makefile and CI to run make build #11

iamniting · 2021-02-18T12:10:05Z

Signed-off-by: Nitin Goyal [email protected]

Makefile

.github/workflows/protoc.yaml

.gitignore

Makefile

Madhu-1 · 2021-02-23T11:14:39Z

.gitignore

@@ -0,0 +1,11 @@
+# binary files
+protoc


these are installed inside the folder name called google right?

no, they are directly inside our repo

ew, better not do that

.gitignore

Madhu-1 · 2021-02-23T11:16:14Z

Makefile

+install-deps:
+ifndef PROTOC
+	# download protoc
+	curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip


Suggested change

curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip

curl -OL https://github.com/protocolbuffers/protobuf/releases/download/${PROTOC_VERSION}/protoc-3.14.0-linux-x86_64.zip

for all the tools we installed the version should be defined as a variable above and we should use the same here.

Madhu-1 · 2021-02-23T11:18:13Z

Makefile

+
+build: install-deps
+	# generate libs
+	./protoc --go_out=lib/go/replication --go_opt=paths=source_relative --plugin=./protoc-gen-go *.proto


Suggested change

./protoc --go_out=lib/go/replication --go_opt=paths=source_relative --plugin=./protoc-gen-go *.proto

./protoc --go_out=lib/go/replication --go_opt=paths=source_relative --plugin=./protoc-gen-go replication.proto

Changed the file name but can not change the indentation. It gives an error

Madhu-1 · 2021-02-23T11:18:52Z

Makefile

+	rm -rf protoc \
+		protoc-gen-go \
+		protoc-gen-go-grpc \
+		protoc-3.14.0-linux-x86_64.zip \
+		protoc-gen-go.v1.25.0.linux.386.tar.gz \
+		protoc-gen-go-grpc.v1.1.0.linux.386.tar.gz \
+		google/protobuf/*.proto


indentation? and also its good to install these packages to a folder and remove the folder

nixpanic

just some nits to consider for improvement

nixpanic · 2021-02-24T08:42:40Z

Makefile

+install-deps:
+ifndef PROTOC
+	# download protoc
+	curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip


Better download in a sub-directory and not clutter the root of the project with unneeded files.
Same counts for the extraction, run it from a different directory.

nixpanic · 2021-02-24T08:44:21Z

Makefile

+endif
+
+clean:
+	rm -rf protoc \


why pass -r for recursive? I do not think any directories are removed here?

It probably is not a good idea to remove the downloaded dependencies with make clean. A next make will need to download them again, that does not seem very useful. make clean should remove compiled/generated files, I recommend to introduce a different target for cleaning up everything, maybe make dep-clean or something like it.

nixpanic · 2021-02-24T08:53:40Z

Makefile

+PROTOC_GEN_GO_REQ := "protoc-gen-go v1.25.0"
+PROTOC_GEN_GO_GRPC_REQ := "protoc-gen-go-grpc 1.1.0"
+
+PROTOC := $(shell ([ "`$(PWD)/protoc --version 2> /dev/null`" == $(PROTOC_REQ) ] && echo true))


passing && echo true is a little strange to read, you could rewrite it to something easier to understand like:

PROTOC_VERSION := $(shell ./protoc --version 2>/dev/null) ifeq($(PROTOC_VERSION),$(PROTOC_REQ) HAVE_PROTOC := "yes" endif

After that, you can use ifdef HAVE_PROTOC to check if it is available. This reads more similar to how other projects use detection of features/settings/...

nixpanic · 2021-02-24T09:00:03Z

Makefile

+		protoc-gen-go \
+		protoc-gen-go-grpc \
+		protoc-3.14.0-linux-x86_64.zip \
+		protoc-gen-go.v1.25.0.linux.386.tar.gz \


really, it would be so much cleaner to place downloaded files in ./dist/ and executables in ./bin/

nixpanic · 2021-02-24T09:00:31Z

.gitignore

@@ -0,0 +1,11 @@
+# binary files
+protoc


ew, better not do that

nixpanic · 2021-02-24T09:02:37Z

lib/go/replication/replication.pb.go

@@ -1,20 +1,16 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.


Explain in the commit how these files are generated. You added a make target, so the command in the commit should be make ....

iamniting · 2021-02-24T10:58:49Z

@nixpanic Can you pls take a look again at fresh as the whole structure is changed now?

nixpanic

Looks much cleaner to me, way easier to understand what is done and where non-repository files are located.

Thanks!

nixpanic · 2021-02-24T12:47:51Z

Makefile

+PROTOC_GEN_GO_GRPC_FOUND := $(shell ./bin/protoc-gen-go-grpc --version 2> /dev/null)
+
+
+ifeq ("$(PROTOC_FOUND)","libprotoc ${PROTOC_VERSION}")


have you tested this? there is a mix of $(...) (make) and ${...} (bash) notation

Yes, I did both are working similarly for me. Anyway, I have changed it to curly.

Madhu-1

LGTM. one minor comment to check *.go files after make build in CI

Madhu-1 · 2021-03-01T09:55:51Z

.github/workflows/protoc.yaml

+
+    - name: generate go libs using protoc
+      run: |
+        make build


can we check that pushed*.go files are not having any changes once we do make the build here?

Signed-off-by: Nitin Goyal <[email protected]>

raghavendra-talur reviewed Feb 18, 2021

View reviewed changes

Makefile Show resolved Hide resolved

This was referenced Feb 18, 2021

Add updated generated files #6

Closed

Add Makefile #5

Closed

Madhu-1 reviewed Feb 19, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

.github/workflows/protoc.yaml Outdated Show resolved Hide resolved

iamniting force-pushed the protoc branch 4 times, most recently from 996b147 to 6d5a608 Compare February 23, 2021 05:47

sp98 reviewed Feb 23, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

iamniting force-pushed the protoc branch from 6d5a608 to f08ca82 Compare February 23, 2021 11:09

Madhu-1 reviewed Feb 23, 2021

View reviewed changes

nixpanic reviewed Feb 24, 2021

View reviewed changes

iamniting force-pushed the protoc branch from f08ca82 to 0c2b5e4 Compare February 24, 2021 10:38

iamniting force-pushed the protoc branch from 0c2b5e4 to c4bb2ab Compare February 24, 2021 12:44

nixpanic approved these changes Feb 24, 2021

View reviewed changes

iamniting force-pushed the protoc branch from c4bb2ab to 48b0603 Compare February 24, 2021 13:12

Madhu-1 reviewed Mar 1, 2021

View reviewed changes

iamniting added 4 commits March 1, 2021 20:15

Add Makefile

b15a85e

Signed-off-by: Nitin Goyal <[email protected]>

Add .gitignore file

400ed3c

Signed-off-by: Nitin Goyal <[email protected]>

Add protoc CI

b5e7e0f

Signed-off-by: Nitin Goyal <[email protected]>

Add generated go libs via running 'make build'

04aee13

Signed-off-by: Nitin Goyal <[email protected]>

iamniting force-pushed the protoc branch from 48b0603 to 04aee13 Compare March 1, 2021 14:47

Madhu-1 approved these changes Mar 2, 2021

View reviewed changes

Madhu-1 merged commit ad3b144 into csi-addons:main Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Makefile and CI to run make build #11

Add Makefile and CI to run make build #11

iamniting commented Feb 18, 2021

Madhu-1 Feb 23, 2021

iamniting Feb 24, 2021

nixpanic Feb 24, 2021

Madhu-1 Feb 23, 2021

Madhu-1 Feb 23, 2021

Madhu-1 Feb 23, 2021

iamniting Feb 24, 2021 •

edited

Loading

Madhu-1 Feb 23, 2021

nixpanic left a comment

nixpanic Feb 24, 2021

nixpanic Feb 24, 2021

nixpanic Feb 24, 2021

nixpanic Feb 24, 2021

nixpanic Feb 24, 2021

nixpanic Feb 24, 2021

iamniting commented Feb 24, 2021

nixpanic left a comment

nixpanic Feb 24, 2021

iamniting Feb 24, 2021

Madhu-1 left a comment

Madhu-1 Mar 1, 2021

iamniting Mar 1, 2021

	curl -OL https://github.com/protocolbuffers/protobuf/releases/download/v3.14.0/protoc-3.14.0-linux-x86_64.zip
	curl -OL https://github.com/protocolbuffers/protobuf/releases/download/${PROTOC_VERSION}/protoc-3.14.0-linux-x86_64.zip

	./protoc --go_out=lib/go/replication --go_opt=paths=source_relative --plugin=./protoc-gen-go *.proto
	./protoc --go_out=lib/go/replication --go_opt=paths=source_relative --plugin=./protoc-gen-go replication.proto

		@@ -1,20 +1,16 @@
		// Code generated by protoc-gen-go. DO NOT EDIT.

		PROTOC_GEN_GO_GRPC_FOUND := $(shell ./bin/protoc-gen-go-grpc --version 2> /dev/null)


		ifeq ("$(PROTOC_FOUND)","libprotoc ${PROTOC_VERSION}")

Add Makefile and CI to run make build #11

Add Makefile and CI to run make build #11

Conversation

iamniting commented Feb 18, 2021

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

iamniting Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic 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

iamniting commented Feb 24, 2021

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting Feb 24, 2021 •

edited

Loading