build: temporary unbreak for make proto#5632
Conversation
Due to an import pathing restriction in protoc, we still need the source tree to represent the import names in the packages. This workaround makes the build work for those who have their github branch checked out this way. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
33b972f to
ad080b8
Compare
| $(PROTOC_COMMAND) -Iproto $< --python_out=py/vtproto --grpc_python_out=py/vtproto | ||
|
|
||
| # TODO(sougou): find a better way around this temp hack. | ||
| VTTOP=$(VTROOT)/../../.. |
There was a problem hiding this comment.
In the contributor's guide, we are asking people to checkout to ~/vitess. This will break anyone who follows that.
What error are you getting from make proto?
There was a problem hiding this comment.
Run as is, you'll get this error:
~/...vitess> make proto
Tue Dec 31 12:30:45 PST 2019: Compiling proto definitions
/bin/bash: line 1: cd: /home/sougou/dev/src/vitess.io/vitess/src: No such file or directory
[more of the same]
If you fix the cd command, then you'll get something like this:
~/...vitess> make proto
Tue Dec 31 12:32:38 PST 2019: Compiling proto definitions
vitess.io/vitess/proto: warning: directory does not exist.
vitess.io/vitess/proto/tabletmanagerservice.proto: No such file or directory
The .proto files, there is this line:
option go_package = "vitess.io/vitess/go/vt/proto/tabletmanagerdata";
This makes protoc want to generate the file at vitess.io/vitess/proto relative to the current path. We need to figure out a way to make it generate the file under vt/proto instead. I haven't found a way yet.
If we change the go_package to go/vt/proto/tabletmanagerdata, then it generates the wrong package names in the generated files (without thevitess.io/vitess prefix).
Essentially, protos won't build with the current Makefile at all. This change at least allows you to build it if you have vitess checked out under src/vitess.io/vitess.
There was a problem hiding this comment.
Could we potentially use a symlink from vitess.io/vitess/proto to vt/proto instead? .. at least until we find a better way to do this.
|
Tried the symlink. Didn't help: |
The protoc generated code was not matching the goimports standard. I've added a goimports step after the build. This caused another breakage because time.proto did not match its package name of vttime. I've fixed that also. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
|
I've pushed an extra commit to fix the codegen of protoc, which was no compliant with goimports. |
morgo
left a comment
There was a problem hiding this comment.
LGTM. This is temporary, but at least it is working again. We can spend some more time figuring out a more permanent fix.
Due to an import pathing restriction in protoc, we still need the
source tree to represent the import names in the packages.
This workaround makes the build work for those who have their
github branch checked out this way.
Signed-off-by: Sugu Sougoumarane ssougou@gmail.com