src-d / lookout-sdk

SDK for lookout analyzers
Apache License 2.0
4 stars 11 forks source link

Lock gogofaster to a fixed v1.2.0 #50

Closed dpordomingo closed 5 years ago

dpordomingo commented 5 years ago

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

smacker commented 5 years ago

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 commented 5 years ago

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

but it worked in my machine :P

dpordomingo commented 5 years ago

:heavy_check_mark: Travis approved

carlosms commented 5 years ago

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

dpordomingo commented 5 years ago

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:

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 commented 5 years ago

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

just run it with different $PATH.

carlosms commented 5 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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

dpordomingo commented 5 years ago

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 commented 5 years ago

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

dpordomingo commented 5 years ago

@carlosms your suggestion was partially addressed: