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

Making pb2 auto-gen script also copy over .proto files. #1317

Merged
merged 1 commit into from
Jan 14, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Dec 22, 2015

@tseaver As you'll notice, this is quite nasty. I'm open to suggestions for making this better. Some simple options:

  • We are content that this is "good enough"
  • I become competent using bash
  • I rewrite this script in Python with judicious use of some system calls

FWIW, this is in advance of v1beta3 for datastore, which will re-use some of these components (e.g. timestamp) so we need this pipeline to be a bit more polished.

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Dec 22, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 22, 2015
GRPC_PLUGIN=grpc_python_plugin
PROTOC_CMD=protoc

This comment was marked as spam.

@dhermes dhermes mentioned this pull request Dec 22, 2015
49 tasks
@tseaver
Copy link
Contributor

tseaver commented Dec 22, 2015

It seems a little weird that this is a makefile, given that the logic is all imperative. It seems like there should be a way to use pattern-based dependency rules to do the compilation / copying.

@dhermes
Copy link
Contributor Author

dhermes commented Dec 22, 2015

I agree, I hate it. Should I just take a stab at re-writing this in Python (where I can actually use a for loop)?


I must say I don't know much about the protoc tool.

  • There is a way to specify --proto_path to look for dependencies, but we end up having lots of paths
  • There is no way in protoc to accomplish the goal of this PR (copying over the .proto files while also renaming them with an _ in front)
  • There is a flag that may be useful that is in the updated protoc but not in the one in the Debian package repo
--dependency_out=FILE       Write a dependency output file in the format
                            expected by make. This writes the transitive
                            set of input file paths to FILE
  • Since the generated files all live in the google/... file structure and the libraries don't use a namespace package, we have to move them, so again we have to manually locate the path
  • It seems that the following is supported
protoc --proto_path=src --python_out=build/gen src/foo.proto src/bar/baz.proto

which produces build/gen/foo_pb2.py and build/gen/bar/baz_pb2.py

@dhermes dhermes force-pushed the makefile-bring-over-dot-proto branch from bde00aa to 7a09564 Compare December 22, 2015 23:06
@dhermes
Copy link
Contributor Author

dhermes commented Dec 22, 2015

@tseaver PTAL. This is (hopefully) much easier to read and for now can hold off implementing in Python (and maybe for always).

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

Rather than treating the makefile like a bash script, could we make the generated _pb2.py files in the right locations be actual make targets, and then write pattern-based rules for producing them (in the two or three steps needed) from the original .proto files?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

Want to take a stab at it?

Fully revamping the script to be more read-able, split
into sections:

- Compile `.proto` to `_pb2.py` files
- Move over `_pb2.py` files into our library
- Clear out old `.proto` files in library
- Move over the newly compiled `.proto` files into library
  and remove the executable bits and prepend them with `_`
@dhermes dhermes force-pushed the makefile-bring-over-dot-proto branch from 5be7afc to d97004a Compare January 14, 2016 21:31
@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

@tseaver can we punt on the Makefile optimizations and try to handle it if/when the service specific protos get their own repo?

The point of this PR was to prepare for building v1beta3 in a more systematic way but it is blocking other PRs.

If you'd like I could also pull the relevant content apart from PRs that use this as a diff base

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

@dhermes If datastore v1beta3 is the driver, I'm not sure why we're mucking about with generating protobuf stuff: wouldn't we be better off just importing from googleapis-common-protos?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

That will only contain "common" protos (see #1353 for which protos that means). This doesn't help for service specific protos (Bigtable, datastore and soon pub/sub and logging)

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

Weird. The googleapis repository actually contains the datastore v1beta3 protos. It doesn't have any of the code for making the Python releases, however.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

@tbetbetbe never told me how they generated the code but the point of common is the set of protos that are utilities used by services but aren't defined within a specific service (e.g. google.api.duration)

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

If we could get the googleapis team to either a) make PyPI releases for the service-specific protobuf wrappers (`googleapis-datastore-protos', etc.), or b) publish how they are generating the one they made already, that would be helpful. I'm currently hacking on how to use their Makefile to generate the Python code.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

@tseaver I filed #1384 to track this and cc-ed the lead of the team in charge PyPI package for common. Can we merge this and discuss the path forward in the issue?

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

Yup. LGTM

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

FWIW I've suggested your a) in recent conversations with Tim.

dhermes added a commit that referenced this pull request Jan 14, 2016
Making pb2 auto-gen script also copy over .proto files.
@dhermes dhermes merged commit 533b3c6 into googleapis:master Jan 14, 2016
@dhermes dhermes deleted the makefile-bring-over-dot-proto branch January 14, 2016 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants