Fixed make proto #747

jojohappy · 2019-01-17T11:11:39Z

Signed-off-by: jojohappy sarahdj0917@gmail.com

Changes

We have a shortcut of make proto to generates golang files from Thanos proto files. But if people have not installed the standard protocol buffer implementation from https://github.com/google/protobuf, the command will be failed.

This PR will fix the bug for downloading the protoc binary and pinning the version at 3.4.0 we specified.

Verification

To run make proto locally.

bwplotka

LGTM, just super minor nits! Thanks!

bwplotka · 2019-01-17T11:49:28Z

Makefile

+PROMU_VERSION       ?= 264dc36af9ea3103255063497636bd5713e3e9c1
+PROTOC              ?= $(BIN_DIR)/protoc-$(PROTOC_VERSION)
+PROTOC_VERSION      ?= 3.4.0
+PROTOC_PACKAGE      ?= protoc-$(PROTOC_VERSION)-linux-x86_64.zip


I would put it to golang BIN_DIR if possible (: What do you think?

em, actually it's possible in theory, but the package has sub bin dir, I should extract the binary only. That is why I put the package at the top of temporary dir.

what do you mean? You can use PATH to file.. it's even better

It's actually important, otherwise users can accidently commit this file

I guess you make a mistake, I will put the package (protoc-$(PROTOC_VERSION)-linux-x86_64.zip) into the TMP_GOPATH dir.
cd -- $(TMP_GOPATH) && curl -OLSs $(PROTOC_DOWNLOAD_URL); and then move the binary protoc into golang BIN_DIR.

but I will also change the destination of package downloaded. Thanks for your suggestion.

oh.. Yes I totally missed that! no... TMP is totally fine for protoc package..! sorry

bwplotka · 2019-01-17T11:50:25Z

Makefile

-	@go install ./vendor/github.com/gogo/protobuf/protoc-gen-gogofast
-	@./scripts/genproto.sh
+proto: deps $(GOIMPORTS) $(PROTOC)
+	@GOIMPORTS_BIN="$(GOIMPORTS)" PROTOC_BIN="$(PROTOC)" ./scripts/genproto.sh


but maybe positional arguments would work better? Not a blocker

Sorry, I can't catch your suggestion.

bwplotka · 2019-01-17T11:53:48Z

Makefile

+	@echo ">> installing protoc@${PROTOC_VERSION}"
+	@mv -- "$(TMP_GOPATH)/bin/protoc" "$(BIN_DIR)/protoc-$(PROTOC_VERSION)"
+	@echo ">> produced $(BIN_DIR)/protoc-$(PROTOC_VERSION)"
+	@go install ./vendor/github.com/gogo/protobuf/protoc-gen-gogofast


Let's run thins in very beginning. Otherwise when you run protoc and it will get into to the 214 line and we abort this, protoc will be installed so running make proto will not install protoc, but protoc-gen-gogofast won't be there.

You are right, my bad.

bwplotka

One thing and LGTM

bwplotka · 2019-01-17T22:44:12Z

Makefile

+PROMU_VERSION       ?= 264dc36af9ea3103255063497636bd5713e3e9c1
+PROTOC              ?= $(BIN_DIR)/protoc-$(PROTOC_VERSION)
+PROTOC_VERSION      ?= 3.4.0
+PROTOC_PACKAGE      ?= protoc-$(PROTOC_VERSION)-linux-x86_64.zip


It's actually important, otherwise users can accidently commit this file

bwplotka · 2019-01-18T12:48:36Z

Makefile

+
+$(PROTOC):
+	@echo ">> fetching protoc@${PROTOC_VERSION}"
+	@if [ ! -x '$(BIN_DIR)/protoc' ]; then \


Why is this? makefile already checks if command in variable $(PROTOC) exists. Let's remove it to fetch it always, I think to simplify.

bwplotka · 2019-01-18T12:49:24Z

Makefile

+$(PROTOC):
+	@echo ">> fetching protoc@${PROTOC_VERSION}"
+	@if [ ! -x '$(BIN_DIR)/protoc' ]; then \
+		curl -LSs $(PROTOC_DOWNLOAD_URL) -o $(BIN_DIR)/$(PROTOC_PACKAGE); \


I messed up... tmp packages in bin might be not ok... ): Can we move it back to tmp.
What I meant with my above comments is for $(PROTOC) itself - it should be in BIN_DIR as you already did it.

Sorry for confusion!

It's Ok, I'll rollback the code.

bwplotka

Perfect, once CI is read let's merge it.

I will checkout this and test on my machine as well.

bwplotka · 2019-01-18T14:26:29Z

And it does not work:

curl: (23) Failed writing body (0 != 16360)
Makefile:209: recipe for target '/home/bartek/Repos/thanosGo/bin/protoc-3.4.0' failed
make: *** [/home/bartek/Repos/thanosGo/bin/protoc-3.4.0] Error 2

;p

jojohappy · 2019-01-18T14:51:40Z

Because the temp dir does not exist... I'll fix it.

bwplotka · 2019-01-18T15:21:08Z

We need mkdir -p $(TMP_GOPATH) in proto before doing curl and all works then 👍 @jojohappy

bwplotka

Nice, thanks! Will merge on green.

adrien-f · 2019-01-18T19:46:32Z

What about people on OSX ?

bwplotka · 2019-01-18T20:58:40Z

What exactly blocker you see for osx?

…

On Fri, Jan 18, 2019, 19:46 Adrien F ***@***.***> wrote: What about people on OSX ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#747 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGoNu9acu4s1tJhKodZ0u8DzpDZ_yExBks5vEiSagaJpZM4aE-Qy> .

adrien-f · 2019-01-18T21:56:15Z

The whole PROTOC_PACKAGE ?= protoc-$(PROTOC_VERSION)-linux-x86_64.zip line. That would mean downloading a Linux binary for OSX ?

jojohappy · 2019-01-19T11:57:25Z

Thanks @adrien-f , maybe we should download the protoc package base on the arch of system. I will try to catch the correct value of arch.

jojohappy · 2019-01-19T12:16:55Z

I find that we can use the golang env $GOOS and $GOARCH to combine the full download url of protoc package.

The valid combinations of $GOOS and $GOARCH are from the section

Since the protoc packages provide three operation systems only, linux, darwin and windows. maybe we can support those systems only and drop errors for others, such as bsd, plan9, solaris etc...

what do you think? @adrien-f @bwplotka

adrien-f · 2019-01-19T15:20:15Z

Seems much more sensible to me ! TIL about $GOOS 👍

bwplotka · 2019-01-21T13:07:51Z

I feel like limiting dev to OSX and Linux for development is ok. Worth to mention this in CONTRIBUTING.md

FUSAKLA · 2019-01-21T20:31:13Z

It's 👍 from me but the architecture detection would be good. (tested on linux and it's working as expected)

bwplotka

So @jojohappy do we wait on you to do OS split for protoc installation now in this PR, right?

jojohappy · 2019-01-22T22:29:20Z

Yes, it is WIP, I will update soon.

jojohappy · 2019-01-23T09:13:06Z

@FUSAKLA @adrien-f @bwplotka I have updated the PR, PTAL, thanks!

adrien-f

Awesome !

adrien-f · 2019-01-23T10:14:54Z

scripts/installprotoc.sh

+    esac
+    true
+}
+adjust_arch() {


New Line maybe?

adrien-f · 2019-01-23T10:15:48Z

scripts/installprotoc.sh

+
+is_supported_platform "$PLATFORM"
+if [[ $? -eq 1 ]]; then
+    echo "platform $PLATFORM is not supported."


Maybe echo a link to protoc's download page ?

adrien-f · 2019-01-23T10:23:36Z

CONTRIBUTING.md

@@ -9,8 +9,9 @@ Please follow the [code of conduct](CODE_OF_CONDUCT.md) in all your interactions
 ## Pull Request Process

 1. Read [getting started docs](docs/getting_started.md) and prepare Thanos.
-2. Familarize yourself with [Makefile](Makefile) commands like `format`, `build`, `proto` and `test`.
-3. Fork improbable-eng/thanos.git and start development from your own fork. Here are sample steps to setup your development environment:
+2. It is strongly recommended that you can use MACOSX or popular Linux distributions systems e.g., ubuntu, redhat, and opensuse for development.


It is strongly recommended that you ~~can~~ use OS X

Maybe also capitalize distributions names

FUSAKLA · 2019-01-23T17:22:08Z

Tested and still ok 👍

bwplotka

Oh no...more bash ^^

But if the outcome is to have reproducible proto generation.. I am in! LGTM

bwplotka · 2019-01-23T18:27:14Z

CONTRIBUTING.md

@@ -9,8 +9,9 @@ Please follow the [code of conduct](CODE_OF_CONDUCT.md) in all your interactions
 ## Pull Request Process

 1. Read [getting started docs](docs/getting_started.md) and prepare Thanos.
-2. Familarize yourself with [Makefile](Makefile) commands like `format`, `build`, `proto` and `test`.
-3. Fork improbable-eng/thanos.git and start development from your own fork. Here are sample steps to setup your development environment:
+2. It is strongly recommended that you use OSX or popular Linux distributions systems e.g., Ubuntu, Redhat, and OpenSUSE for development.


Suggested change

2. It is strongly recommended that you use OSX or popular Linux distributions systems e.g., Ubuntu, Redhat, and OpenSUSE for development.

2. It is strongly recommended that you use OSX or popular Linux distributions systems e.g. Ubuntu, Redhat, or OpenSUSE for development.

Can we move it outside of Pull Request Process in some Prerequisites section?

bwplotka · 2019-01-23T18:29:46Z

Just small doc nit and we can merge. Thanks @jojohappy (:

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

bwplotka · 2019-01-24T10:11:36Z

I guess it's now time to hook it in CI to test if proto was generated, right? (:

jojohappy · 2019-01-24T14:58:49Z

@bwplotka Yes, it is.

Could we keep working for #733? Or still broken by waiting for moving dependency to go mod?

FUSAKLA · 2019-01-24T18:01:28Z

Ugh we agreed with @bwplotka to leave that as it is right now and focus on more important things so I wouldn't wait for it probably and add it separately.

bwplotka approved these changes Jan 17, 2019

View reviewed changes

bwplotka reviewed Jan 18, 2019

View reviewed changes

bwplotka approved these changes Jan 18, 2019

View reviewed changes

bwplotka reviewed Jan 22, 2019

View reviewed changes

adrien-f reviewed Jan 23, 2019

View reviewed changes

jojohappy force-pushed the fix_make_proto branch 2 times, most recently from 5845b2f to 040cb96 Compare January 23, 2019 14:03

bwplotka approved these changes Jan 23, 2019

View reviewed changes

jojohappy added 5 commits January 24, 2019 08:30

Fixed make proto

0e9527a

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

Address review comments

eecf67e

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

Change the download destination of protoc package

d00602f

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

Rollback && simplify the code

eb6dd4d

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

Fixed temporary dir doesn't exist

8f13b90

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

jojohappy added 2 commits January 24, 2019 08:30

To install protoc command only suuportting linux or macosx

097675f

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

Update docs

cb96d88

Signed-off-by: jojohappy <sarahdj0917@gmail.com>

jojohappy force-pushed the fix_make_proto branch from 040cb96 to cb96d88 Compare January 24, 2019 00:38

bwplotka approved these changes Jan 24, 2019

View reviewed changes

bwplotka merged commit 71f4e25 into thanos-io:master Jan 24, 2019

jojohappy deleted the fix_make_proto branch January 24, 2019 14:50

	2. It is strongly recommended that you use OSX or popular Linux distributions systems e.g., Ubuntu, Redhat, and OpenSUSE for development.
	2. It is strongly recommended that you use OSX or popular Linux distributions systems e.g. Ubuntu, Redhat, or OpenSUSE for development.

Fixed make proto #747

Fixed make proto #747

Conversation

jojohappy commented Jan 17, 2019

Changes

Verification

bwplotka 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

jojohappy Jan 18, 2019 • edited

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

bwplotka 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

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Jan 18, 2019

jojohappy commented Jan 18, 2019

bwplotka commented Jan 18, 2019

bwplotka left a comment

Choose a reason for hiding this comment

adrien-f commented Jan 18, 2019

bwplotka commented Jan 18, 2019 via email

adrien-f commented Jan 18, 2019

jojohappy commented Jan 19, 2019

jojohappy commented Jan 19, 2019

adrien-f commented Jan 19, 2019

bwplotka commented Jan 21, 2019

FUSAKLA commented Jan 21, 2019

bwplotka left a comment • edited

Choose a reason for hiding this comment

jojohappy commented Jan 22, 2019

jojohappy commented Jan 23, 2019

adrien-f 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

FUSAKLA commented Jan 23, 2019

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Jan 23, 2019

bwplotka commented Jan 24, 2019 • edited

jojohappy commented Jan 24, 2019

FUSAKLA commented Jan 24, 2019

jojohappy Jan 18, 2019 •

edited

bwplotka left a comment •

edited

bwplotka commented Jan 24, 2019 •

edited