search

openconfig / gribi

A gRPC Interface to a Network Element RIB.
Apache License 2.0
57 stars 14 forks source link

Update generated protos and Go modules. #5

Closed robshakir closed 3 years ago

robshakir commented 3 years ago 
* (M) fakeserver/...
   - Fix import paths.
 * (A) go.mod
 * (A) go.sum
   - Update module support for girib.
 * (M) proto/gribi_aft
   - Add go_package annotation and regenerate protobufs.
 * (M) proto/service/...
   - Generate gRPC bindings separately to Go files.
   - Add annotation for go_package.
 * (M) scripts/update-schema.sh
   - Ensure that schema generation uses the updated proto_generator.\
sbezverk commented 3 years ago

@robshakir you still rely on the fact that the $GOPATH/src is used, but for example in my case all code is outside of GOPATH and it is not even set. Would you be able to address it and make it non gopath dependent?

robshakir commented 3 years ago

Good question -- looking at where we reference $GOPATH here:

proto/gribi_aft/gen.go://go:generate sh -c "cd $GOPATH/src && protoc --proto_path=. --go_out=:. github.com/openconfig/gribi/proto/gribi_aft/gribi_aft.proto"
proto/gribi_aft/enums/gen.go://go:generate sh -c "cd $GOPATH/src && protoc --proto_path=. --go_out=:. github.com/openconfig/gribi/proto/gribi_aft/enums/enums.proto"
proto/service/gen.go://go:generate sh -c "cd $GOPATH/src && protoc --proto_path=. --go_out=. --go-grpc_out=. github.com/openconfig/gribi/proto/service/gribi.proto"
scripts/update-schema.sh:$GOPATH/bin/proto_generator \
scripts/update-schema.sh:$GOPATH/bin/generator \

The last two are trivial to fix since this is $GOPATH/bin and we could rely on the fact that this is within your $PATH.

For the others, I think we might be able to rely on understanding the cwd used when go generate is used -- let me figure that out.

sbezverk commented 3 years ago

@robshakir what do you think about using go mod vendor approach without committing into the repo but to add its population with go mod tidy? I think it could provide a nice and universal solution..

robshakir commented 3 years ago

I'd prefer not to use go mod vendor since I'm not sure we're tied to very specific versions of packages in this repository right now. My view is without checking in the generated packages there it's not clear how we'd actually be able to have other projects reference the generated .pb.go. The blocker right now (AIUI, please correct me if you see differently) is being able to have the .pb.go files generated without needing to reference GOPATH. I think the best way to do this might just be to remove the go generate commands and have make generate do this itself.

sbezverk commented 3 years ago

@robshakir for me any solution which is not relying on presence of GOPATH is good.

robshakir commented 3 years ago

@sbezverk PTAL -- this is a little hacky, since we vendor the protos manually during the make generate command (and currently just tie to master, but I think this is OK) but it makes sure that there is no dependency on the GOPATH environment variable during the build process.

I think the less kludgy solution is to move to using bazel or similar, but this feels a little overkill at the current time.

sbezverk commented 3 years ago

It is working, but man it is a bit weird to see user's home directory in a public repo :)

robshakir commented 3 years ago

For sure -- pushed a version that fixes this by just find/replacing the paths. The references to the homedir are just part of ensuring that we have a record of what input files were used to generate the code, so as long as we have some record it seems fine.

PTAL and check that this still works for you and I can commit it.

sbezverk commented 3 years ago

@robshakir it seems there is not used import in enums.proto

proto/gribi_aft/enums/enums.proto:13:1: warning: Import github.com/openconfig/ygot/proto/ywrapper/ywrapper.proto is unused.

Probably worth to remove it since it is not used...

sbezverk commented 3 years ago

@robshakir any plans to merge it?

robshakir commented 3 years ago

Apologies for the delay here. The unused imports are something that we need to track in ygot, since this is a generated protobuf. I filed https://github.com/openconfig/ygot/issues/432 a while ago for this, but it hasn't bubbled up the priority stack. It's in the work that we're tracking for ygot's 1.0.0 release though.

robshakir commented 3 years ago

@nandanarista - yes, this is just bringing the latest update here. We're making the import paths relative so that we're not tied to the exact GitHub directory structure, protoc can be called with the import path directly to this repo rather than GOPATH which allows the build to be the same.

On the YANG files, they're just vendored from elsewhere.