Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 84351d4

Browse filesBrowse files
ewiandaaignas
ewianda
and
aignas
authored
fix: Resolve incorrect platform specific dependency (bazel-contrib#2766)
This change addresses a bug where `pip.parse` selects the wrong requirement entry when multiple extras are listed with platform-specific markers. #### 🔍 Problem: In a `requirements.txt` generated by tools like `uv` or `poetry`, it's valid to have multiple entries for the same package, each with different extras and `sys_platform` markers, for example: ```ini optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin' optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux' ``` The current implementation in [`[parse_requirements.bzl](https://github.com/bazel-contrib/rules_python/blob/032f6aa738a673b13b605dabf55465c6fc1a56eb/python/private/pypi/parse_requirements.bzl#L114-L126)`](https://github.com/bazel-contrib/rules_python/blob/032f6aa738a673b13b605dabf55465c6fc1a56eb/python/private/pypi/parse_requirements.bzl#L114-L126) uses a sort-by-length heuristic to select the “best” requirement when there are multiple entries with the same base name. This works well in legacy `requirements.txt` files where: ``` my_dep my_dep[foo] my_dep[foo,bar] ``` ...would indicate an intent to select the **most specific subset of extras** (i.e. the longest name). However, this heuristic **breaks** in the presence of **platform markers**, where extras are **not subsets**, but distinct variants. In the example above, Bazel mistakenly selects `optimum[onnxruntime-gpu]` on macOS because it's a longer match, even though it is guarded by a Linux-only marker. #### ✅ Fix: This PR modifies the behavior to: 1. **Add the requirement marker** as part of the sorting key. 2. **Then apply the longest-match logic** to drop duplicate requirements with different extras but the same markers. This ensures that only applicable requirements are considered during resolution, preserving correctness in multi-platform environments. #### 🧪 Before: On macOS, the following entry is incorrectly selected: ``` optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux' ``` #### ✅ After: Correct entry is selected: ``` optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin' ``` close bazel-contrib#2690 --------- Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
1 parent 6e2d493 commit 84351d4
Copy full SHA for 84351d4

File tree

Expand file treeCollapse file tree

5 files changed

+119
-39
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+119
-39
lines changed

‎CHANGELOG.md

Copy file name to clipboardExpand all lines: CHANGELOG.md
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ Unreleased changes template.
8181

8282
{#v0-0-0-fixed}
8383
### Fixed
84+
* (pypi) Platform specific extras are now correctly handled when using
85+
universal lock files with environment markers. Fixes [#2690](https://github.com/bazel-contrib/rules_python/pull/2690).
8486
* (runfiles) ({obj}`--bootstrap_impl=script`) Follow symlinks when searching for runfiles.
8587
* (toolchains) Do not try to run `chmod` when downloading non-windows hermetic toolchain
8688
repositories on Windows. Fixes

‎python/private/pypi/parse_requirements.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/parse_requirements.bzl
+16-28Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -30,22 +30,9 @@ load("//python/private:normalize_name.bzl", "normalize_name")
3030
load("//python/private:repo_utils.bzl", "repo_utils")
3131
load(":index_sources.bzl", "index_sources")
3232
load(":parse_requirements_txt.bzl", "parse_requirements_txt")
33+
load(":pep508_requirement.bzl", "requirement")
3334
load(":whl_target_platforms.bzl", "select_whls")
3435

35-
def _extract_version(entry):
36-
"""Extract the version part from the requirement string.
37-
38-
39-
Args:
40-
entry: {type}`str` The requirement string.
41-
"""
42-
version_start = entry.find("==")
43-
if version_start != -1:
44-
# Extract everything after '==' until the next space or end of the string
45-
version, _, _ = entry[version_start + 2:].partition(" ")
46-
return version
47-
return None
48-
4936
def parse_requirements(
5037
ctx,
5138
*,
@@ -111,19 +98,20 @@ def parse_requirements(
11198
# The requirement lines might have duplicate names because lines for extras
11299
# are returned as just the base package name. e.g., `foo[bar]` results
113100
# in an entry like `("foo", "foo[bar] == 1.0 ...")`.
114-
requirements_dict = {
115-
(normalize_name(entry[0]), _extract_version(entry[1])): entry
116-
for entry in sorted(
117-
parse_result.requirements,
118-
# Get the longest match and fallback to original WORKSPACE sorting,
119-
# which should get us the entry with most extras.
120-
#
121-
# FIXME @aignas 2024-05-13: The correct behaviour might be to get an
122-
# entry with all aggregated extras, but it is unclear if we
123-
# should do this now.
124-
key = lambda x: (len(x[1].partition("==")[0]), x),
125-
)
126-
}.values()
101+
# Lines with different markers are not condidered duplicates.
102+
requirements_dict = {}
103+
for entry in sorted(
104+
parse_result.requirements,
105+
# Get the longest match and fallback to original WORKSPACE sorting,
106+
# which should get us the entry with most extras.
107+
#
108+
# FIXME @aignas 2024-05-13: The correct behaviour might be to get an
109+
# entry with all aggregated extras, but it is unclear if we
110+
# should do this now.
111+
key = lambda x: (len(x[1].partition("==")[0]), x),
112+
):
113+
req = requirement(entry[1])
114+
requirements_dict[(req.name, req.version, req.marker)] = entry
127115

128116
tokenized_options = []
129117
for opt in parse_result.options:
@@ -132,7 +120,7 @@ def parse_requirements(
132120

133121
pip_args = tokenized_options + extra_pip_args
134122
for plat in plats:
135-
requirements[plat] = requirements_dict
123+
requirements[plat] = requirements_dict.values()
136124
options[plat] = pip_args
137125

138126
requirements_by_platform = {}

‎python/private/pypi/pep508_requirement.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/pep508_requirement.bzl
+11Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,16 @@ def requirement(spec):
3030
"""
3131
spec = spec.strip()
3232
requires, _, maybe_hashes = spec.partition(";")
33+
34+
version_start = requires.find("==")
35+
version = None
36+
if version_start != -1:
37+
# Extract everything after '==' until the next space or end of the string
38+
version, _, _ = requires[version_start + 2:].partition(" ")
39+
40+
# Remove any trailing characters from the version string
41+
version = version.strip(" ")
42+
3343
marker, _, _ = maybe_hashes.partition("--hash")
3444
requires, _, extras_unparsed = requires.partition("[")
3545
extras_unparsed, _, _ = extras_unparsed.partition("]")
@@ -42,4 +52,5 @@ def requirement(spec):
4252
name = normalize_name(name).replace("_", "-"),
4353
marker = marker.strip(" "),
4454
extras = extras,
55+
version = version,
4556
)

‎tests/pypi/extension/extension_tests.bzl

Copy file name to clipboardExpand all lines: tests/pypi/extension/extension_tests.bzl
+78Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -856,6 +856,84 @@ git_dep @ git+https://git.server/repo/project@deadbeefdeadbeef
856856

857857
_tests.append(_test_simple_get_index)
858858

859+
def _test_optimum_sys_platform_extra(env):
860+
pypi = _parse_modules(
861+
env,
862+
module_ctx = _mock_mctx(
863+
_mod(
864+
name = "rules_python",
865+
parse = [
866+
_parse(
867+
hub_name = "pypi",
868+
python_version = "3.15",
869+
requirements_lock = "universal.txt",
870+
),
871+
],
872+
),
873+
read = lambda x: {
874+
"universal.txt": """\
875+
optimum[onnxruntime]==1.17.1 ; sys_platform == 'darwin'
876+
optimum[onnxruntime-gpu]==1.17.1 ; sys_platform == 'linux'
877+
""",
878+
}[x],
879+
),
880+
available_interpreters = {
881+
"python_3_15_host": "unit_test_interpreter_target",
882+
},
883+
)
884+
885+
pypi.exposed_packages().contains_exactly({"pypi": []})
886+
pypi.hub_group_map().contains_exactly({"pypi": {}})
887+
pypi.hub_whl_map().contains_exactly({
888+
"pypi": {
889+
"optimum": {
890+
"pypi_315_optimum_linux_aarch64_linux_arm_linux_ppc_linux_s390x_linux_x86_64": [
891+
whl_config_setting(
892+
version = "3.15",
893+
target_platforms = [
894+
"cp315_linux_aarch64",
895+
"cp315_linux_arm",
896+
"cp315_linux_ppc",
897+
"cp315_linux_s390x",
898+
"cp315_linux_x86_64",
899+
],
900+
config_setting = None,
901+
filename = None,
902+
),
903+
],
904+
"pypi_315_optimum_osx_aarch64_osx_x86_64": [
905+
whl_config_setting(
906+
version = "3.15",
907+
target_platforms = [
908+
"cp315_osx_aarch64",
909+
"cp315_osx_x86_64",
910+
],
911+
config_setting = None,
912+
filename = None,
913+
),
914+
],
915+
},
916+
},
917+
})
918+
919+
pypi.whl_libraries().contains_exactly({
920+
"pypi_315_optimum_linux_aarch64_linux_arm_linux_ppc_linux_s390x_linux_x86_64": {
921+
"dep_template": "@pypi//{name}:{target}",
922+
"python_interpreter_target": "unit_test_interpreter_target",
923+
"repo": "pypi_315",
924+
"requirement": "optimum[onnxruntime-gpu]==1.17.1",
925+
},
926+
"pypi_315_optimum_osx_aarch64_osx_x86_64": {
927+
"dep_template": "@pypi//{name}:{target}",
928+
"python_interpreter_target": "unit_test_interpreter_target",
929+
"repo": "pypi_315",
930+
"requirement": "optimum[onnxruntime]==1.17.1",
931+
},
932+
})
933+
pypi.whl_mods().contains_exactly({})
934+
935+
_tests.append(_test_optimum_sys_platform_extra)
936+
859937
def extension_test_suite(name):
860938
"""Create the test suite.
861939

‎tests/pypi/pep508/requirement_tests.bzl

Copy file name to clipboardExpand all lines: tests/pypi/pep508/requirement_tests.bzl
+12-11Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,20 +20,21 @@ _tests = []
2020

2121
def _test_requirement_line_parsing(env):
2222
want = {
23-
" name1[ foo ] ": ("name1", ["foo"]),
24-
"Name[foo]": ("name", ["foo"]),
25-
"name [fred,bar] @ http://foo.com ; python_version=='2.7'": ("name", ["fred", "bar"]),
26-
"name; (os_name=='a' or os_name=='b') and os_name=='c'": ("name", [""]),
27-
"name@http://foo.com": ("name", [""]),
28-
"name[ Foo123 ]": ("name", ["Foo123"]),
29-
"name[extra]@http://foo.com": ("name", ["extra"]),
30-
"name[foo]": ("name", ["foo"]),
31-
"name[quux, strange];python_version<'2.7' and platform_version=='2'": ("name", ["quux", "strange"]),
32-
"name_foo[bar]": ("name-foo", ["bar"]),
23+
" name1[ foo ] ": ("name1", ["foo"], None, ""),
24+
"Name[foo]": ("name", ["foo"], None, ""),
25+
"name [fred,bar] @ http://foo.com ; python_version=='2.7'": ("name", ["fred", "bar"], None, "python_version=='2.7'"),
26+
"name; (os_name=='a' or os_name=='b') and os_name=='c'": ("name", [""], None, "(os_name=='a' or os_name=='b') and os_name=='c'"),
27+
"name@http://foo.com": ("name", [""], None, ""),
28+
"name[ Foo123 ]": ("name", ["Foo123"], None, ""),
29+
"name[extra]@http://foo.com": ("name", ["extra"], None, ""),
30+
"name[foo]": ("name", ["foo"], None, ""),
31+
"name[quux, strange];python_version<'2.7' and platform_version=='2'": ("name", ["quux", "strange"], None, "python_version<'2.7' and platform_version=='2'"),
32+
"name_foo[bar]": ("name-foo", ["bar"], None, ""),
33+
"name_foo[bar]==0.25": ("name-foo", ["bar"], "0.25", ""),
3334
}
3435

3536
got = {
36-
i: (parsed.name, parsed.extras)
37+
i: (parsed.name, parsed.extras, parsed.version, parsed.marker)
3738
for i, parsed in {case: requirement(case) for case in want}.items()
3839
}
3940
env.expect.that_dict(got).contains_exactly(want)

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.