Skip to content

Conversation

@josephrubin
Copy link

@josephrubin josephrubin commented Dec 5, 2024

Since protoc supports pyi_out, allow py_proto_library to generate the pyi files for editor support (otherwise, most editors cannot autocomplete with proto python output).

References: #1293

Testing:

There are no tests specifically for py_proto_library, but when I run:

~/protobuf/examples$ bazel build //:addressbook_py_pb2

I get:

INFO: Analyzed target //:addressbook_py_pb2 (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:addressbook_py_pb2 up-to-date:
  bazel-bin/external/protobuf~/src/google/protobuf/_virtual_imports/timestamp_proto/google/protobuf/timestamp_pb2.py
  bazel-bin/external/protobuf~/src/google/protobuf/_virtual_imports/timestamp_proto/google/protobuf/timestamp_pb2.pyi
  bazel-bin/addressbook_pb2.py
  bazel-bin/addressbook_pb2.pyi

Since protoc supports `pyi_out`, allow py_proto_library to generate the pyi
files for editor support (otherwise, most editors cannot autocomplete with
proto python output).

References: protocolbuffers#1293

Test:

There are no tests specifically for py_proto_library, but when I run:

`~/protobuf/examples$ bazel build //:addressbook_py_pb2`

I get:

```
INFO: Analyzed target //:addressbook_py_pb2 (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:addressbook_py_pb2 up-to-date:
  bazel-bin/external/protobuf~/src/google/protobuf/_virtual_imports/timestamp_proto/google/protobuf/timestamp_pb2.py
  bazel-bin/external/protobuf~/src/google/protobuf/_virtual_imports/timestamp_proto/google/protobuf/timestamp_pb2.pyi
  bazel-bin/addressbook_pb2.py
  bazel-bin/addressbook_pb2.pyi
```
if plugin_output:
args.add(plugin_output, format = proto_lang_toolchain_info.out_replacement_format_flag)
if proto_lang_toolchain_info.out_replacement_format_flag.startswith("--python"):
args.add(plugin_output, format = "--pyi_out=%s")
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure the best way to do this. I would prefer to just add the --pyi_out to python's proto_lang_toolchain instantiation but the command_line = "--python_out=%s", can only contain one %s string.

This is my first time in this code base so I would appreciate any suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have you define another toolchain here for pyi:

proto_lang_toolchain(
name = "python_toolchain",
command_line = "--python_out=%s",
progress_message = "Generating Python proto_library %{label}",
runtime = ":protobuf_python",
# NOTE: This isn't *actually* public. It's an implicit dependency of py_proto_library,
# so must be public so user usages of the rule can reference it.
visibility = ["//visibility:public"],
)

Then it can be another dependency to the rule, and we can invoke proto_common.compile() twice, once for each toolchain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should not require changing proto_common.bzl. That is common code used by all proto rules, and shouldn't know anything about .pyi.

@josephrubin josephrubin changed the title Adds generated pyi support to py_proto_library feat(python_pyi): add generated pyi to py_proto_library Dec 5, 2024
@josephrubin josephrubin marked this pull request as ready for review December 5, 2024 05:18
@shaod2
Copy link
Member

shaod2 commented Dec 9, 2024

Why relying on the output of py_proto_library is required? I would think that it should be consumed by other bazel rules. Could you just run protoc to get the desired result?

@josephrubin
Copy link
Author

josephrubin commented Dec 9, 2024

I think that bazel build should be the canonical way to generate build artifacts in a bazel repo. You could run protoc directly but bazel in theory has a better understanding of the packaging, build directory, and so forth for a Python project (as well as uses the hermetic protoc).

The resulting pyi files could be used as input to, for example, another rule that generates documentation based on build output.

But I also understand where you are coming from. These pyi files are not needed at runtime.

@shaod2 shaod2 requested a review from anandolee December 9, 2024 20:23
@tonyliaoss tonyliaoss added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 19, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Feb 19, 2025
@haberman
Copy link
Member

Sorry for the slow review. I've just added some suggestions for how to do this without changing proto_common.bzl.

@mkruskal-google
Copy link
Member

#21567 seems to do this the way Josh was suggesting, closing this to follow up on that PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants