Update makefile for proto related rules#169
Conversation
Makefile.core.mk
Outdated
| proto_iscp: | ||
| protoc -I=${GOPATH}/src -I./pkg/apis/istio/v1alpha2/ --proto_path=pkg/apis/istio/v1alpha2/ --gofast_out=pkg/apis/istio/v1alpha2/ pkg/apis/istio/v1alpha2/istiocontrolplane_types.proto | ||
| proto_iscp: get_dep_proto | ||
| protoc -I=/tmp/src -I=${GOPATH}/src/ -I=/usr/include/protobuf --gogofast_out=${GOPATH}/src ${GOPATH}/src/istio.io/operator/pkg/apis/istio/v1alpha2/istiocontrolplane_types.proto |
There was a problem hiding this comment.
the first part looks good. the ${GOPATH}/src - understand what your after, I 'll need to think a little bit about how we can generalize this. I just woke up so I am not super sharp atm, but I think we need a system host bindmount on either ${GOPATH} or /.${GOPATH}may work for our use case with operator repo, but may not work for every use case. THe bindmount should be rshared - so will need to test if that will work as I expect.
I guess we can start with that, and expand.
Cheers
-steve
Makefile.core.mk
Outdated
| # get imported protos to $GOPATH | ||
| get_dep_proto: | ||
| GO111MODULE=off go get k8s.io/api/core/v1 k8s.io/api/autoscaling/v2beta1 k8s.io/apimachinery/pkg/apis/meta/v1/ github.com/gogo/protobuf/... | ||
| GO111MODULE=off GOPATH=/tmp go get k8s.io/api/core/v1 k8s.io/api/autoscaling/v2beta1 k8s.io/apimachinery/pkg/apis/meta/v1/ github.com/gogo/protobuf/... |
There was a problem hiding this comment.
interesting solutoin, although these deps are pulled automatically by for example make downloador are already likely in the image . Protobuf itself is in the expected systemwide location.
There was a problem hiding this comment.
The k8s deps are not available in the common image now. Actually pulling in those deps to /tmp using go get does not require changes to existing proto import paths.
There was a problem hiding this comment.
The common image does include parts of gogo/protobuf: https://github.com/istio/tools/blob/master/docker/build-tools/Dockerfile#L46-L49
if parts of gogo/protbuf are not present, perhaps you could improve thecommon-tools image to include what is needed? We really want to pin any dependencies needed using either go.mod or build-tools.
The dependencies are in go.mod referenced as k8s.io/api and k8s.io/apimachinery. Do you mean these are not pulled into the go package cache when using the repository? Or just that using -I is not desireable with protoc?
There was a problem hiding this comment.
the protoc include path does not handle go package cache properly, that's why I have to pull those deps to temp directory
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
|
@sdake Can you help please take another look? |
sdake
left a comment
There was a problem hiding this comment.
Thanks for the work!
I'm not sure this PR is quite right, but it removes a bunch of cruft, so approving - and we can iterate towards a better solution that works with CONTAINER_BUILD=1.
Cheers
-steve
| Currently there are two proto files defined temporarily in istio/operator: istiocontrolplane_types.proto and values.types.proto, which would be moved to istio/api after 1.3 | ||
|
|
||
| There are some type of fields in the original proto files, e.g. []map[string]interface{}, cannot be handled properly by protoc, | ||
| so we comment out this fields and patch them back in the generated pb.go files |
Thanks for checking. Now it works with both container and local. |
* Update makefile for proto related rules to work for new system * Address comments * Add doc gen * Upload updated pb.go files * Update common * Address comment
* Eliminate vfsgen again. (#200) * Fix build commmand for installing go-bindata (#202) * Update makefile for proto related rules (#169) * Update makefile for proto related rules to work for new system * Address comments * Add doc gen * Upload updated pb.go files * Update common * Address comment * Merge two install sections into one (#203) * Merge two install sections into one * Change section title * Update to 2019-08-16 commonfiles (#207) Enable MacOS based container builds. Depends-On: istio/common-files#30 Depends-On: istio/tools#274 * support resource type for k8s mapping. (#206) * support resource type for k8s mapping. * apply comments. * Fix reverse translation issue (#205) * Fix reverse translation issue * Address hpa spec issues * Refactor * Second version of refactor * Address comment * lint * Changes to API for mesh config integration (#210) * Changes to API for mesh config integration * Review comments, add strategy * Add strategy to k8s settings * Fix unit tests * update doc. (#212) * update doc. * apply comments. * update link with recent source. * Clean up values (#214) * remove invalid and duplicated fields. (#217)
Uh oh!
There was an error while loading. Please reload this page.