-
Notifications
You must be signed in to change notification settings - Fork 3
Add Makefile to generate python files based on idl definition #3
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
Conversation
Makefile
Outdated
# Generate Python files for API protos | ||
$(DST_DIR)/uber/cadence/api/v1/%_pb2.py: $(PROTO_ROOT)/api/v1/%.proto | $(DST_DIR)/uber/cadence/api/v1/ | ||
@echo "Generating Python file for $(PROTO_ROOT)/api/v1/$*.proto" | ||
protoc --proto_path=$(SRC_DIR) --python_out=$(DST_DIR) $(PROTO_ROOT)/api/v1/$*.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to copy the protoc download-and-init-in-temp-build-dir thing from the other cadence repos :\ protoc's code is far from stable across versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protoc v3.14.0 does not have pyi support, should I just a separate version for the pyi files? I think .pyi files are quite essential for dev to understand type definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no need for it to be the same version as go/java, we can just pick a newer one. they're all wire-binary-compatible with each other, and we have no code-API backwards compatibility needs that would force us on an older version (unlike go/java)
Makefile
Outdated
show: | ||
@echo "Proto files to process:" | ||
@echo "API:" | ||
@for file in $(API_PROTO_FILES); do echo " $$file"; done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible alternative to these, for makefile-debugging purposes:
$(info $(API_PROTO_FILES))
^ basically anywhere, but generally at parse-time (outside any target).
(tbh I think these "help understand the makefile" targets are completely fine while things are simple, so this LGTM as-is. just offering an alternative for future debug-sessions. remake
is also truly excellent and highly recommended)
copying from side-chat:
also we might want to pull in the submodule-check from the server repo, since this silently does nothing if the submodule isn't initialized. |
Python uses pyproject. Please refer to https://github.com/temporalio/sdk-python/blob/main/pyproject.toml on how to add build-system. |
Summary
Adds a Makefile that automates generation of Python protobuf files from Cadence's IDL definitions. Processes both API and Admin protobuf files from
idls/proto/uber/cadence/
and generates corresponding Python_pb2.py
files in.gen/
directory.Changes
Makefile
- Build automation with targets:all
(default),clean
,help
,show
.gen/uber/cadence/api/v1/
and.gen/uber/cadence/admin/v1/
Usage
Prerequisites
protoc
(Protocol Buffers compiler) in PATH.proto
files inidls/proto/uber/cadence/