Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock gogofaster to a fixed v1.2.0 #50

Merged
merged 1 commit into from
Dec 17, 2018

Conversation

dpordomingo
Copy link
Contributor

@dpordomingo dpordomingo commented Dec 12, 2018

This PR would fix the issue with code generation in go, caused by a new release of gogo/protobuf-v1.2.0.
Because of that, Travis is failing: https://travis-ci.org/src-d/lookout-sdk/jobs/465002703

-	IncludeLanguages []string `protobuf:"bytes,8,rep,name=include_languages,json=includeLanguages" json:"include_languages,omitempty"`
+	IncludeLanguages []string `protobuf:"bytes,8,rep,name=include_languages,json=includeLanguages,proto3" json:"include_languages,omitempty"`

This PR will lock our code generation to the new gogofaster v1.2.0.

Implementation

It builds protoc-gen-gogofaster from release gogo/protobuf-v1.2.0 and uses it without messing user binaries nor sources.

Caveats

  • we should update lookout to depend on the new protobuf v1.2.0 (it currently depends on v1.1.1)

@dpordomingo dpordomingo added the bug Something isn't working label Dec 12, 2018
@dpordomingo dpordomingo self-assigned this Dec 12, 2018
@smacker
Copy link
Contributor

smacker commented Dec 12, 2018

CI:

unzip -aoq .ci/protobuf-07eab6a8298cf32fac45cceaac59424f98421bbc.zip -d .ci
mkdir -p /github.com/gogo
mkdir: cannot create directory ‘/github.com’: Permission denied
Makefile:30: recipe for target 'install-protoc-gen-gogofaster' failed
make: *** [install-protoc-gen-gogofaster] Error 1
The command "make protogen" exited with 2.

@dpordomingo
Copy link
Contributor Author

yes... I'll fix the CI problem ;)

but it worked in my machine :P

@dpordomingo dpordomingo force-pushed the lock-googofaster branch 2 times, most recently from e6db36f to dc6cb5b Compare December 12, 2018 13:25
@dpordomingo dpordomingo changed the title Lock gogofaster to a fixed version Lock gogofaster to a fixed v1.2.0 Dec 12, 2018
@dpordomingo
Copy link
Contributor Author

✔️ Travis approved

Makefile Outdated Show resolved Hide resolved
@carlosms
Copy link
Contributor

Are there any alternatives to avoid changing the contents of $GOPATH/src?

@dpordomingo
Copy link
Contributor Author

We were currently doing that when using go get.
To have a proper gogofaster installed is an initial requirement from the very beginning as defined by protogen_golang.sh#L4

Assumes 'protoc' and 'protoc-gen-gogofaster' binaries are installed

We could:

  • check if protoc can use a gogofaster binary not being globally installed;
  • backup existent one, and restore it after the generation; or
  • generate the code in an isolated env.

Anyway I'd merge this (since it does not change the current behavior) and create a new issue to plan a better way to generate our go code, considering the three alternatives above, or any other that we could find.

@smacker
Copy link
Contributor

smacker commented Dec 13, 2018

check if protoc can use a gogofaster binary not being globally installed;

just run it with different $PATH.

@carlosms
Copy link
Contributor

What if we do something like this in a .sh, will this work?

mkdir -p /tmp/tmpgopath
export GOPATH=/tmp/tmpgopath

go get github.com/gogo/protobuf/protoc-gen-gofast
cd $GOPATH/src/github.com/gogo/protobuf
git checkout v1.2.0

go install github.com/gogo/protobuf/protoc-gen-gogofaster
mv $GOPATH/bin/protoc-gen-gogofaster path/our/tools

cd -
rm -rf /tmp/tmpgopath

@dpordomingo
Copy link
Contributor Author

dpordomingo commented Dec 14, 2018

Compiling protoc-gen-gogofaster into path/our/tools as @carlosms proposed at #issuecomment-446931766 (it can be done without installing nor changing $GOPATH) does not work because as said at golang/protobuf installation guide

The compiler plugin, protoc-gen-gofaster, [...] must be in your $PATH for the protocol compiler, protoc, to find it.

If protoc-gen-gogofaster is not under $PATH the generation fails:

$ ./_tools/protogen_golang.sh
protoc-gen-gogofaster: program not found or is not executable
--gogofaster_out: protoc-gen-gogofaster: Plugin failed with status code 1.
Failed to run protoc on proto/lookout/sdk

Changing $PATH as proposed by @smacker at #issuecomment-446909715 makes the ./_tools/protogen_golang.sh fail because mv command is not find (because the shell rely in $PATH to locate it.

Conclusion:

  • ✔️ we can build our own protoc-gen-gofaster without messing user $GOPATH/src,
  • ❎ I found no way to a protoc-gen-gofaster version out of user $PATH,

Alternatives to keep user $PATH/protoc-gen-gofaster:
a) backup -> install ours -> generate -> restore,
b) use a container to generate the protos.

b could be the best, more general and secure one... but I think it would be overengineering the solution; so I'd vote for a.

@smacker
Copy link
Contributor

smacker commented Dec 14, 2018

David:

export PATH=<new path with protoc-gen-gofaster>:$PATH

even if there is another gofaster somewhere in the user's path the first one will be used.

@dpordomingo
Copy link
Contributor Author

I'm already working with that option, but thanks ;)

@dpordomingo
Copy link
Contributor Author

what I'm trying now (and locally working, is something applying both: building outside of user $GOPATH,
and using the PATH=/custom/path;$PATH)

@dpordomingo
Copy link
Contributor Author

Done by 9804eac
It builds protoc-gen-gogofaster from release gogo/protobuf-v1.2.0 and uses it without messing user binaries nor sources.

Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe we could clean up the downloads and the src folder, but up to you.

Copy link
Contributor

@smacker smacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's fix CI already :)

@dpordomingo
Copy link
Contributor Author

@carlosms your suggestion was partially addressed:

  • ✔️ clean up the src folder,
  • ❌ clean up the downloads → used for local cache, to avoid downloading it was already dowloaded

Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>
@dpordomingo dpordomingo merged commit d7f9318 into src-d:master Dec 17, 2018
@dpordomingo dpordomingo deleted the lock-googofaster branch December 17, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants