mobile: setup Python package distribution to PyPI#43793
mobile: setup Python package distribution to PyPI#43793paul-r-gall wants to merge 13 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
abeyad
left a comment
There was a problem hiding this comment.
This is great, thanks @paul-r-gall !
Just a couple comments, and also mobile format is failing.
| name: python_wheel | ||
| path: dist | ||
| - name: Publish package to PyPI | ||
| uses: pypa/gh-action-pypi-publish@v1.8.14 |
There was a problem hiding this comment.
Should we add some instructions on what a developer should do with the Python .whl file if they want to use Envoy Client in their Python application? e.g. what commands should they run and what set up do they need in their build system so Envoy Client can be used?
There was a problem hiding this comment.
Yeah I'll add that once I figure out how to do it myself.
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
Signed-off-by: Paul Ogilby <pgal@google.com>
|
/gemini review |
|
flagging that this isnt being tested in ci - probably we should test it in staging first |
There was a problem hiding this comment.
Code Review
This pull request sets up the infrastructure to build and distribute the envoy-mobile Python package to PyPI. It introduces Bazel rules for creating a Python wheel that includes the native C++ extension, configures package metadata in pyproject.toml, and updates CI workflows for building and deploying. My review focuses on improving the correctness and maintainability of the Bazel packaging rules. I've identified a few issues: an incorrect package name in py_package, a non-portable genrule for the native library, and improper handling of pyproject.toml which leads to incorrect package metadata. Addressing these will ensure the Python package is built correctly and is easier to maintain.
|
|
||
| py_package( | ||
| name = "envoy_mobile_package", | ||
| packages = ["library.python.envoy_mobile"], |
There was a problem hiding this comment.
The packages attribute for py_package appears to be incorrect. It's set to ["library.python.envoy_mobile"], but the package being created is envoy_mobile, as evidenced by the new import statements in test files (from envoy_mobile import ...).
The py_package rule uses this to find the package's __init__.py. With imports = ["."] in the py_library dependency, the search path is mobile/library/python. The current setting would cause Bazel to look for a non-existent path. This should be corrected to point to the actual package name.
| packages = ["library.python.envoy_mobile"], | |
| packages = ["envoy_mobile"], |
| py_wheel( | ||
| name = "envoy_mobile_wheel", | ||
| abi = ":abi_tag", | ||
| distribution = "envoy-mobile", | ||
| platform = select({ | ||
| ":is_linux_x86_64": "manylinux2014_x86_64", | ||
| ":is_macos_x86_64": "macosx_10_15_x86_64", | ||
| ":is_macos_arm64": "macosx_11_0_arm64", | ||
| "//conditions:default": "any", | ||
| }), | ||
| python_tag = ":python_tag", | ||
| strip_path_prefixes = [ | ||
| "mobile/library/python", | ||
| "library/python", | ||
| ], | ||
| version = "0.5.0", | ||
| visibility = ["//visibility:public"], | ||
| deps = [":envoy_mobile_package"], | ||
| ) |
There was a problem hiding this comment.
The pyproject.toml file is currently being packaged as a data file within the envoy_mobile package (defined in envoy_mobile_lib on line 91), which is likely not the intended behavior. It should be used to define the package metadata for the wheel itself.
To fix this, you should use the pyproject_toml attribute on py_wheel. This will also allow you to remove redundant attributes like distribution and version, as rules_python will infer them from the TOML file. You will also need to remove //:pyproject.toml from the data attribute of the envoy_mobile_lib py_library target.
py_wheel(
name = "envoy_mobile_wheel",
pyproject_toml = "//:pyproject.toml",
abi = ":abi_tag",
platform = select({
":is_linux_x86_64": "manylinux2014_x86_64",
":is_macos_x86_64": "macosx_10_15_x86_64",
":is_macos_arm64": "macosx_11_0_arm64",
"//conditions:default": "any",
}),
python_tag = ":python_tag",
strip_path_prefixes = [
"mobile/library/python",
"library/python",
],
visibility = ["//visibility:public"],
deps = [":envoy_mobile_package"],
)
| genrule( | ||
| name = "envoy_engine_so", | ||
| srcs = [":envoy_engine.so"], | ||
| outs = ["envoy_mobile/envoy_engine.so"], | ||
| cmd = "cp $< $@", |
There was a problem hiding this comment.
This genrule is not portable to Windows. The comment for pybind_extension mentions (or .pyd on Windows), which implies Windows support is a consideration. However, this rule hardcodes the .so extension in srcs and outs, and uses the cp command which is not standard on Windows.
For better cross-platform compatibility, consider using select to handle different file extensions and commands for different operating systems, or refactoring this into a custom Starlark rule that uses ctx.actions.copy.
Signed-off-by: Paul Ogilby <pgal@google.com>
Commit Message: mobile: setup Python package distribution.
AI Usage: This PR was almost 100% generated by Gemini CLI.
__init__.py: Established the public API by re-exporting key classes from the native extension (Engine, EngineBuilder, Stream, etc.).