From 08f5114ca3979964de47328f7996e2b7d762c4a3 Mon Sep 17 00:00:00 2001 From: Ignas Anikevicius <240938+aignas@users.noreply.github.com> Date: Thu, 10 Apr 2025 23:00:54 +0900 Subject: [PATCH] fix(pypi): fixes to the marker evaluation and utils These are just bugfixes to already merged code: * Fix nested bracket parsing in PEP508 marker parser. * Fix the sys_platform constants, which I noticed in #2629 but they got also pointed out in #2766. * Port some of python tests for requirement parsing and improve the implementation. Those tests will be removed in #2629. * Move the platform related code to a separate file. * Rename `pep508_req.bzl` to `pep508_requirement.bzl` to follow the convention. All of the bug fixes have added tests. Work towards #2423. --- python/private/pypi/BUILD.bazel | 15 ++++- python/private/pypi/evaluate_markers.bzl | 9 +-- python/private/pypi/pep508_env.bzl | 63 ++++++++++--------- python/private/pypi/pep508_platform.bzl | 57 +++++++++++++++++ ...{pep508_req.bzl => pep508_requirement.bzl} | 9 ++- tests/pypi/pep508/BUILD.bazel | 5 ++ tests/pypi/pep508/requirement_tests.bzl | 47 ++++++++++++++ 7 files changed, 165 insertions(+), 40 deletions(-) create mode 100644 python/private/pypi/pep508_platform.bzl rename python/private/pypi/{pep508_req.bzl => pep508_requirement.bzl} (82%) create mode 100644 tests/pypi/pep508/requirement_tests.bzl diff --git a/python/private/pypi/BUILD.bazel b/python/private/pypi/BUILD.bazel index 21e05f2895..e0a2f20c14 100644 --- a/python/private/pypi/BUILD.bazel +++ b/python/private/pypi/BUILD.bazel @@ -77,7 +77,8 @@ bzl_library( deps = [ ":pep508_env_bzl", ":pep508_evaluate_bzl", - ":pep508_req_bzl", + ":pep508_platform_bzl", + ":pep508_requirement_bzl", ], ) @@ -223,6 +224,9 @@ bzl_library( bzl_library( name = "pep508_env_bzl", srcs = ["pep508_env.bzl"], + deps = [ + ":pep508_platform_bzl", + ], ) bzl_library( @@ -235,8 +239,13 @@ bzl_library( ) bzl_library( - name = "pep508_req_bzl", - srcs = ["pep508_req.bzl"], + name = "pep508_platform_bzl", + srcs = ["pep508_platform.bzl"], +) + +bzl_library( + name = "pep508_requirement_bzl", + srcs = ["pep508_requirement.bzl"], deps = [ "//python/private:normalize_name_bzl", ], diff --git a/python/private/pypi/evaluate_markers.bzl b/python/private/pypi/evaluate_markers.bzl index 1d4c30753f..a0223abdc8 100644 --- a/python/private/pypi/evaluate_markers.bzl +++ b/python/private/pypi/evaluate_markers.bzl @@ -14,9 +14,10 @@ """A simple function that evaluates markers using a python interpreter.""" -load(":pep508_env.bzl", "env", _platform_from_str = "platform_from_str") +load(":pep508_env.bzl", "env") load(":pep508_evaluate.bzl", "evaluate") -load(":pep508_req.bzl", _req = "requirement") +load(":pep508_platform.bzl", "platform_from_str") +load(":pep508_requirement.bzl", "requirement") def evaluate_markers(requirements): """Return the list of supported platforms per requirements line. @@ -29,9 +30,9 @@ def evaluate_markers(requirements): """ ret = {} for req_string, platforms in requirements.items(): - req = _req(req_string) + req = requirement(req_string) for platform in platforms: - if evaluate(req.marker, env = env(_platform_from_str(platform, None))): + if evaluate(req.marker, env = env(platform_from_str(platform, None))): ret.setdefault(req_string, []).append(platform) return ret diff --git a/python/private/pypi/pep508_env.bzl b/python/private/pypi/pep508_env.bzl index 17d41871d1..265a8e9b99 100644 --- a/python/private/pypi/pep508_env.bzl +++ b/python/private/pypi/pep508_env.bzl @@ -15,7 +15,9 @@ """This module is for implementing PEP508 environment definition. """ -# See https://stackoverflow.com/questions/45125516/possible-values-for-uname-m +load(":pep508_platform.bzl", "platform_from_str") + +# See https://stackoverflow.com/a/45125525 _platform_machine_aliases = { # These pairs mean the same hardware, but different values may be used # on different host platforms. @@ -24,13 +26,41 @@ _platform_machine_aliases = { "i386": "x86_32", "i686": "x86_32", } + +# Platform system returns results from the `uname` call. _platform_system_values = { "linux": "Linux", "osx": "Darwin", "windows": "Windows", } + +# The copy of SO [answer](https://stackoverflow.com/a/13874620) containing +# all of the platforms: +# ┍━━━━━━━━━━━━━━━━━━━━━┯━━━━━━━━━━━━━━━━━━━━━┑ +# │ System │ Value │ +# ┝━━━━━━━━━━━━━━━━━━━━━┿━━━━━━━━━━━━━━━━━━━━━┥ +# │ Linux │ linux or linux2 (*) │ +# │ Windows │ win32 │ +# │ Windows/Cygwin │ cygwin │ +# │ Windows/MSYS2 │ msys │ +# │ Mac OS X │ darwin │ +# │ OS/2 │ os2 │ +# │ OS/2 EMX │ os2emx │ +# │ RiscOS │ riscos │ +# │ AtheOS │ atheos │ +# │ FreeBSD 7 │ freebsd7 │ +# │ FreeBSD 8 │ freebsd8 │ +# │ FreeBSD N │ freebsdN │ +# │ OpenBSD 6 │ openbsd6 │ +# │ AIX │ aix (**) │ +# ┕━━━━━━━━━━━━━━━━━━━━━┷━━━━━━━━━━━━━━━━━━━━━┙ +# +# (*) Prior to Python 3.3, the value for any Linux version is always linux2; after, it is linux. +# (**) Prior Python 3.8 could also be aix5 or aix7; use sys.platform.startswith() +# +# We are using only the subset that we actually support. _sys_platform_values = { - "linux": "posix", + "linux": "linux", "osx": "darwin", "windows": "win32", } @@ -61,6 +91,7 @@ def env(target_platform, *, extra = None): "platform_release": "", "platform_version": "", } + if type(target_platform) == type(""): target_platform = platform_from_str(target_platform, python_version = "") @@ -87,31 +118,3 @@ def env(target_platform, *, extra = None): "platform_machine": _platform_machine_aliases, }, } - -def _platform(*, abi = None, os = None, arch = None): - return struct( - abi = abi, - os = os, - arch = arch, - ) - -def platform_from_str(p, python_version): - """Return a platform from a string. - - Args: - p: {type}`str` the actual string. - python_version: {type}`str` the python version to add to platform if needed. - - Returns: - A struct that is returned by the `_platform` function. - """ - if p.startswith("cp"): - abi, _, p = p.partition("_") - elif python_version: - major, _, tail = python_version.partition(".") - abi = "cp{}{}".format(major, tail) - else: - abi = None - - os, _, arch = p.partition("_") - return _platform(abi = abi, os = os or None, arch = arch or None) diff --git a/python/private/pypi/pep508_platform.bzl b/python/private/pypi/pep508_platform.bzl new file mode 100644 index 0000000000..381a8d7a08 --- /dev/null +++ b/python/private/pypi/pep508_platform.bzl @@ -0,0 +1,57 @@ +# Copyright 2025 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""The platform abstraction +""" + +def platform(*, abi = None, os = None, arch = None): + """platform returns a struct for the platform. + + Args: + abi: {type}`str | None` the target ABI, e.g. `"cp39"`. + os: {type}`str | None` the target os, e.g. `"linux"`. + arch: {type}`str | None` the target CPU, e.g. `"aarch64"`. + + Returns: + A struct. + """ + + # Note, this is used a lot as a key in dictionaries, so it cannot contain + # methods. + return struct( + abi = abi, + os = os, + arch = arch, + ) + +def platform_from_str(p, python_version): + """Return a platform from a string. + + Args: + p: {type}`str` the actual string. + python_version: {type}`str` the python version to add to platform if needed. + + Returns: + A struct that is returned by the `_platform` function. + """ + if p.startswith("cp"): + abi, _, p = p.partition("_") + elif python_version: + major, _, tail = python_version.partition(".") + abi = "cp{}{}".format(major, tail) + else: + abi = None + + os, _, arch = p.partition("_") + return platform(abi = abi, os = os or None, arch = arch or None) diff --git a/python/private/pypi/pep508_req.bzl b/python/private/pypi/pep508_requirement.bzl similarity index 82% rename from python/private/pypi/pep508_req.bzl rename to python/private/pypi/pep508_requirement.bzl index 618ffaf17a..11f2b3e8fa 100644 --- a/python/private/pypi/pep508_req.bzl +++ b/python/private/pypi/pep508_requirement.bzl @@ -17,7 +17,7 @@ load("//python/private:normalize_name.bzl", "normalize_name") -_STRIP = ["(", " ", ">", "=", "<", "~", "!"] +_STRIP = ["(", " ", ">", "=", "<", "~", "!", "@"] def requirement(spec): """Parse a PEP508 requirement line @@ -28,15 +28,18 @@ def requirement(spec): Returns: A struct with the information. """ + spec = spec.strip() requires, _, maybe_hashes = spec.partition(";") marker, _, _ = maybe_hashes.partition("--hash") requires, _, extras_unparsed = requires.partition("[") + extras_unparsed, _, _ = extras_unparsed.partition("]") for char in _STRIP: requires, _, _ = requires.partition(char) - extras = extras_unparsed.strip("]").split(",") + extras = extras_unparsed.replace(" ", "").split(",") + name = requires.strip(" ") return struct( - name = normalize_name(requires.strip(" ")), + name = normalize_name(name).replace("_", "-"), marker = marker.strip(" "), extras = extras, ) diff --git a/tests/pypi/pep508/BUILD.bazel b/tests/pypi/pep508/BUILD.bazel index b795db0591..575f28ada6 100644 --- a/tests/pypi/pep508/BUILD.bazel +++ b/tests/pypi/pep508/BUILD.bazel @@ -1,5 +1,10 @@ load(":evaluate_tests.bzl", "evaluate_test_suite") +load(":requirement_tests.bzl", "requirement_test_suite") evaluate_test_suite( name = "evaluate_tests", ) + +requirement_test_suite( + name = "requirement_tests", +) diff --git a/tests/pypi/pep508/requirement_tests.bzl b/tests/pypi/pep508/requirement_tests.bzl new file mode 100644 index 0000000000..7c81ea50fc --- /dev/null +++ b/tests/pypi/pep508/requirement_tests.bzl @@ -0,0 +1,47 @@ +# Copyright 2025 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for parsing the requirement specifier.""" + +load("@rules_testing//lib:test_suite.bzl", "test_suite") +load("//python/private/pypi:pep508_requirement.bzl", "requirement") # buildifier: disable=bzl-visibility + +_tests = [] + +def _test_requirement_line_parsing(env): + want = { + " name1[ foo ] ": ("name1", ["foo"]), + "Name[foo]": ("name", ["foo"]), + "name [fred,bar] @ http://foo.com ; python_version=='2.7'": ("name", ["fred", "bar"]), + "name; (os_name=='a' or os_name=='b') and os_name=='c'": ("name", [""]), + "name@http://foo.com": ("name", [""]), + "name[ Foo123 ]": ("name", ["Foo123"]), + "name[extra]@http://foo.com": ("name", ["extra"]), + "name[foo]": ("name", ["foo"]), + "name[quux, strange];python_version<'2.7' and platform_version=='2'": ("name", ["quux", "strange"]), + "name_foo[bar]": ("name-foo", ["bar"]), + } + + got = { + i: (parsed.name, parsed.extras) + for i, parsed in {case: requirement(case) for case in want}.items() + } + env.expect.that_dict(got).contains_exactly(want) + +_tests.append(_test_requirement_line_parsing) + +def requirement_test_suite(name): # buildifier: disable=function-docstring + test_suite( + name = name, + basic_tests = _tests, + )