From 781650cc0913728906160ae778918252e86a008d Mon Sep 17 00:00:00 2001 From: Emil Kristoffer Feet Date: Thu, 10 Oct 2024 22:12:08 +0200 Subject: [PATCH] fix: Handle single-file module pypi-deps in py_pex_binary (#392) Addresses the bug in https://github.com/aspect-build/rules_py/issues/391. Single-file modules such as `six` and `typing-extensions` does not work with the current `py_pex_binary` rule, since it's assumed that there exist a sub-directory within `site-packages` that is not dist-info. An example of what a single-file module pypi-dep looks like: ``` ls bazel-reppro_pex_err/external/rules_python~~pip~pypi_311_six/site-packages/ __init__.py six-1.16.0.dist-info six.py ``` Since `Distribution.load(..)` takes in the `site-packages` directory, we only emit this part of the path, and let `uniquify=True` handle deduplication after `_map_srcs` is applied. --- ### Changes are visible to end-users: no ### Test plan - Manual testing; please provide instructions so we can reproduce: Add `six` to requirements and `"@pypi_six//:pkg"` as a dep to the py_pex_binary example; then import `six` in py_pex_binary's `say.py`. Printing the module or `cowsay.cow(f"{six}")` shows that the previous example and single-files modules now also work. --- gazelle_python.yaml | 4 ++- py/private/py_pex_binary.bzl | 23 +++++------------ py/tests/py-pex-binary/BUILD.bazel | 34 +++++++++++++++++++++++++ py/tests/py-pex-binary/data.txt | 1 + py/tests/py-pex-binary/print_modules.py | 15 +++++++++++ py/tools/pex/main.py | 2 +- requirements.in | 1 + requirements.txt | 1 + 8 files changed, 62 insertions(+), 19 deletions(-) create mode 100644 py/tests/py-pex-binary/BUILD.bazel create mode 100644 py/tests/py-pex-binary/data.txt create mode 100644 py/tests/py-pex-binary/print_modules.py diff --git a/gazelle_python.yaml b/gazelle_python.yaml index e6546504..cc68df4c 100644 --- a/gazelle_python.yaml +++ b/gazelle_python.yaml @@ -2161,6 +2161,7 @@ manifest: numpy.lib.ufunclike: numpy numpy.lib.user_array: numpy numpy.lib.utils: numpy + numpy.libs.libopenblas64_p-r0-0cf96a72: numpy numpy.linalg: numpy numpy.linalg.lapack_lite: numpy numpy.linalg.linalg: numpy @@ -3716,6 +3717,7 @@ manifest: past.types.olddict: future past.types.oldstr: future past.utils: future + pillow.libs.libopenjp2-05423b53: pillow pluggy: pluggy proxytypes: jsonref psutil: psutil @@ -3986,4 +3988,4 @@ manifest: yaml.tokens: PyYAML pip_repository: name: pypi -integrity: e0cc13413ec04d597469a3a5baa75d320d7c614017bbccd34aa3e356675dd984 +integrity: 2e0cbc780621ce75951013e6968f66d1aadd8d1ecb8b7c470883c998b169e733 diff --git a/py/private/py_pex_binary.bzl b/py/private/py_pex_binary.bzl index 90fcf114..9ee01fee 100644 --- a/py/private/py_pex_binary.bzl +++ b/py/private/py_pex_binary.bzl @@ -56,26 +56,15 @@ def _map_srcs(f, workspace): site_packages_i = f.path.find("site-packages") - # if path contains `site-packages` and there is only two path segments - # after it, it will be treated as third party dep. - # Here are some examples of path we expect and use and ones we ignore. - # - # Match: `external/rules_python~~pip~pypi_39_rtoml/site-packages/rtoml-0.11.0.dist-info/INSTALLER` - # Reason: It has two `/` after first `site-packages` substring. - # - # No Match: `external/rules_python~~pip~pypi_39_rtoml/site-packages/rtoml-0.11.0/src/mod/parse.py` - # Reason: It has three `/` after first `site-packages` substring. - if site_packages_i != -1 and f.path.count("/", site_packages_i) == 2: - if f.path.find("dist-info", site_packages_i) != -1: + # If the path contains 'site-packages', treat it as a third party dep + if site_packages_i != -1: + if f.path.find("dist-info", site_packages_i) != -1 and f.path.count("/", site_packages_i) == 2: return ["--distinfo={}".format(f.dirname)] - return ["--dep={}".format(f.dirname)] - elif site_packages_i == -1: - # If the path does not have a `site-packages` in it, then put it into - # the standard runfiles tree. - return ["--source={}={}".format(f.path, dest_path)] + return ["--dep={}".format(f.path[:site_packages_i + len("site-packages")])] - return [] + # If the path does not have a `site-packages` in it, then put it into the standard runfiles tree. + return ["--source={}={}".format(f.path, dest_path)] def _py_python_pex_impl(ctx): py_toolchain = _py_semantics.resolve_toolchain(ctx) diff --git a/py/tests/py-pex-binary/BUILD.bazel b/py/tests/py-pex-binary/BUILD.bazel new file mode 100644 index 00000000..3107a127 --- /dev/null +++ b/py/tests/py-pex-binary/BUILD.bazel @@ -0,0 +1,34 @@ +load("@aspect_bazel_lib//lib:testing.bzl", "assert_contains") +load("//py:defs.bzl", "py_binary", "py_pex_binary") + +# Test that both single-file modules (six) and multi-file modules (cowsay) work with py_pex_binary. +py_binary( + name = "print_modules_bin", + srcs = ["print_modules.py"], + data = ["data.txt"], + deps = [ + "@bazel_tools//tools/python/runfiles", + "@pypi_cowsay//:pkg", + "@pypi_six//:pkg", + ], +) + +py_pex_binary( + name = "print_modules_pex", + binary = ":print_modules_bin", + python_interpreter_constraints = [], +) + +# PEX_ROOT is set to avoid warning on default user PEX_ROOT not being writable +genrule( + name = "run_print_modules_pex", + outs = ["print_modules_pex.out"], + cmd = "PEX_ROOT=.pex $(execpath print_modules_pex) >$@", + tools = ["print_modules_pex"], +) + +assert_contains( + name = "test__print_modules_pex", + actual = "print_modules_pex.out", + expected = "Mooo!,cowsay-6.1/cowsay/__init__.py,six-1.16.0/six.py", +) diff --git a/py/tests/py-pex-binary/data.txt b/py/tests/py-pex-binary/data.txt new file mode 100644 index 00000000..44f77409 --- /dev/null +++ b/py/tests/py-pex-binary/data.txt @@ -0,0 +1 @@ +Mooo! \ No newline at end of file diff --git a/py/tests/py-pex-binary/print_modules.py b/py/tests/py-pex-binary/print_modules.py new file mode 100644 index 00000000..e434a7c4 --- /dev/null +++ b/py/tests/py-pex-binary/print_modules.py @@ -0,0 +1,15 @@ +import sys +import cowsay +import six +from bazel_tools.tools.python.runfiles import runfiles + + +r = runfiles.Create() +data_path = r.Rlocation("aspect_rules_py/py/tests/py-pex-binary/data.txt") + +# strings on one line to test presence for all +print(open(data_path).read() + + "," + + "/".join(cowsay.__file__.split("/")[-3:]) + + "," + + "/".join(six.__file__.split("/")[-2:])) diff --git a/py/tools/pex/main.py b/py/tools/pex/main.py index 45981091..ef2b8469 100644 --- a/py/tools/pex/main.py +++ b/py/tools/pex/main.py @@ -159,7 +159,7 @@ def __call__(self, parser, namespace, value, option_str=None): ] for dep in options.dependencies: - dist = Distribution.load(dep + "/../") + dist = Distribution.load(dep) # TODO: explain which level of inferno is this! key = "%s-%s" % (dist.key, dist.version) diff --git a/requirements.in b/requirements.in index 706c61da..29cbd745 100644 --- a/requirements.in +++ b/requirements.in @@ -5,3 +5,4 @@ pytest cowsay ftfy==6.2.0 neptune==1.10.2 +six diff --git a/requirements.txt b/requirements.txt index 321cb256..e6f9c903 100644 --- a/requirements.txt +++ b/requirements.txt @@ -770,6 +770,7 @@ six==1.16.0 \ --hash=sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926 \ --hash=sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254 # via + # -r requirements.in # bravado # bravado-core # neptune