Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(whl_library): fix the dependency generation for multi-python depenency closures #1875

Merged
merged 4 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
# (Note, we cannot use `common --deleted_packages` because the bazel version command doesn't support it)
# To update these lines, execute
# `bazel run @rules_bazel_integration_test//tools:update_deleted_packages`
build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered
query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered
build --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered
query --deleted_packages=examples/build_file_generation,examples/build_file_generation/random_number_generator,examples/bzlmod,examples/bzlmod_build_file_generation,examples/bzlmod_build_file_generation/other_module/other_module/pkg,examples/bzlmod_build_file_generation/runfiles,examples/bzlmod/entry_points,examples/bzlmod/entry_points/tests,examples/bzlmod/libs/my_lib,examples/bzlmod/other_module,examples/bzlmod/other_module/other_module/pkg,examples/bzlmod/patches,examples/bzlmod/py_proto_library,examples/bzlmod/py_proto_library/example.com/another_proto,examples/bzlmod/py_proto_library/example.com/proto,examples/bzlmod/runfiles,examples/bzlmod/tests,examples/bzlmod/tests/dupe_requirements,examples/bzlmod/tests/other_module,examples/bzlmod/whl_mods,examples/multi_python_versions/libs/my_lib,examples/multi_python_versions/requirements,examples/multi_python_versions/tests,examples/pip_parse,examples/pip_parse_vendored,examples/pip_repository_annotations,examples/py_proto_library,examples/py_proto_library/example.com/another_proto,examples/py_proto_library/example.com/proto,gazelle,gazelle/manifest,gazelle/manifest/generate,gazelle/manifest/hasher,gazelle/manifest/test,gazelle/modules_mapping,gazelle/python,gazelle/pythonconfig,tests/integration/compile_pip_requirements,tests/integration/compile_pip_requirements_test_from_external_repo,tests/integration/ignore_root_user_error,tests/integration/ignore_root_user_error/submodule,tests/integration/pip_parse,tests/integration/pip_parse/empty,tests/integration/pip_repository_entry_points,tests/integration/py_cc_toolchain_registered

test --test_output=errors

Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ A brief description of the categories of changes:
be automatically deleted correctly. For example, if `python_generation_mode`
is set to package, when `__init__.py` is deleted, the `py_library` generated
for this package before will be deleted automatically.
* (whl_library): Use `is_python_config_setting` to correctly handle multi-python
version dependency select statements when the `experimental_target_platforms`
includes the Python ABI. The default python version case within the select is
also now handled correctly, stabilizing the implementation.

### Added

Expand Down
6 changes: 4 additions & 2 deletions examples/bzlmod/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,14 @@ pip.parse(
# to generate the dependency graph for.
experimental_target_platforms = [
# Specifying the target platforms explicitly
"cp39_linux_x86_64",
"cp39_linux_*",
"cp39_*",
"cp310_*",
"cp311_*",
"cp312_*",
],
hub_name = "pip",
python_version = "3.9",
quiet = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking: debug code?

requirements_lock = "//:requirements_lock_3_9.txt",
requirements_windows = "//:requirements_windows_3_9.txt",
# These modifications were created above and we
Expand Down
89 changes: 33 additions & 56 deletions python/pip_install/private/generate_whl_library_build_bazel.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,14 @@ py_library(
"""

def _plat_label(plat):
if plat.endswith("default"):
return plat
if plat.startswith("@//"):
return "@@" + str(Label("//:BUILD.bazel")).partition("//")[0].strip("@") + plat.strip("@")
elif plat.startswith("@"):
return str(Label(plat))
else:
return ":is_" + plat
return ":is_" + plat.replace("cp3", "python_3.")

def _render_list_and_select(deps, deps_by_platform, tmpl):
deps = render.list([tmpl.format(d) for d in sorted(deps)])
Expand All @@ -115,14 +117,7 @@ def _render_list_and_select(deps, deps_by_platform, tmpl):

# Add the default, which means that we will be just using the dependencies in
# `deps` for platforms that are not handled in a special way by the packages
#
# FIXME @aignas 2024-01-24: This currently works as expected only if the default
# value of the @rules_python//python/config_settings:python_version is set in
# the `.bazelrc`. If it is unset, then the we don't get the expected behaviour
# in cases where we are using a simple `py_binary` using the default toolchain
# without forcing any transitions. If the `python_version` config setting is set
# via .bazelrc, then everything works correctly.
deps_by_platform["//conditions:default"] = []
deps_by_platform.setdefault("//conditions:default", [])
deps_by_platform = render.select(deps_by_platform, value_repr = render.list)

if deps == "[]":
Expand All @@ -131,81 +126,63 @@ def _render_list_and_select(deps, deps_by_platform, tmpl):
return "{} + {}".format(deps, deps_by_platform)

def _render_config_settings(dependencies_by_platform):
py_version_by_os_arch = {}
loads = []
additional_content = []
for p in dependencies_by_platform:
# p can be one of the following formats:
# * //conditions:default
# * @platforms//os:{value}
# * @platforms//cpu:{value}
# * @//python/config_settings:is_python_3.{minor_version}
# * {os}_{cpu}
# * cp3{minor_version}_{os}_{cpu}
if p.startswith("@"):
if p.startswith("@") or p.endswith("default"):
continue

abi, _, tail = p.partition("_")
if not abi.startswith("cp"):
tail = p
abi = ""

os, _, arch = tail.partition("_")
os = "" if os == "anyos" else os
arch = "" if arch == "anyarch" else arch

py_version_by_os_arch.setdefault((os, arch), []).append(abi)

if not py_version_by_os_arch:
return None, None

loads = []
additional_content = []
for (os, arch), abis in py_version_by_os_arch.items():
constraint_values = []
if os:
constraint_values.append("@platforms//os:{}".format(os))
if arch:
constraint_values.append("@platforms//cpu:{}".format(arch))
if os:
constraint_values.append("@platforms//os:{}".format(os))

os_arch = (os or "anyos") + "_" + (arch or "anyarch")
additional_content.append(
"""\
config_setting(
constraint_values_str = render.indent(render.list(constraint_values)).lstrip()

if abi:
if not loads:
loads.append("""load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting")""")

additional_content.append(
"""\
is_python_config_setting(
name = "is_{name}",
constraint_values = {values},
python_version = "3.{minor_version}",
constraint_values = {constraint_values},
visibility = ["//visibility:private"],
)""".format(
name = os_arch,
values = render.indent(render.list(sorted([str(Label(c)) for c in constraint_values]))).strip(),
),
)

if abis == [""]:
if not os or not arch:
fail("BUG: both os and arch should be set in this case")
continue

for abi in abis:
if not loads:
loads.append("""load("@bazel_skylib//lib:selects.bzl", "selects")""")
minor_version = int(abi[len("cp3"):])
setting = "@@{rules_python}//python/config_settings:is_python_3.{version}".format(
rules_python = str(Label("//:BUILD.bazel")).partition("//")[0].strip("@"),
version = minor_version,
name = p.replace("cp3", "python_3."),
minor_version = abi[len("cp3"):],
constraint_values = constraint_values_str,
),
)
settings = [
":is_" + os_arch,
setting,
]

plat = "{}_{}".format(abi, os_arch)

else:
additional_content.append(
"""\
selects.config_setting_group(
name = "{name}",
match_all = {values},
config_setting(
name = "is_{name}",
constraint_values = {constraint_values},
visibility = ["//visibility:private"],
)""".format(
name = _plat_label(plat).lstrip(":"),
values = render.indent(render.list(sorted(settings))).strip(),
name = p.replace("cp3", "python_3."),
constraint_values = constraint_values_str,
),
)

Expand Down Expand Up @@ -379,7 +356,7 @@ def generate_whl_library_build_bazel(
contents = "\n".join(
[
_BUILD_TEMPLATE.format(
loads = "\n".join(loads),
loads = "\n".join(sorted(loads)),
py_library_label = py_library_label,
dependencies = render.indent(lib_dependencies, " " * 4).lstrip(),
whl_file_deps = render.indent(whl_file_deps, " " * 4).lstrip(),
Expand Down
1 change: 0 additions & 1 deletion python/pip_install/tools/wheel_installer/arguments_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ def test_platform_aggregation(self) -> None:
parser = arguments.parser()
args = parser.parse_args(
args=[
"--platform=host",
"--platform=linux_*",
"--platform=osx_*",
"--platform=windows_*",
Expand Down
39 changes: 26 additions & 13 deletions python/pip_install/tools/wheel_installer/wheel.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class Arch(Enum):
ppc64le = ppc

@classmethod
def interpreter(cls) -> "OS":
def interpreter(cls) -> "Arch":
"Return the currently running interpreter architecture."
# FIXME @aignas 2023-12-13: Hermetic toolchain on Windows 3.11.6
# is returning an empty string here, so lets default to x86_64
Expand Down Expand Up @@ -94,12 +94,6 @@ class Platform:
arch: Optional[Arch] = None
minor_version: Optional[int] = None

def __post_init__(self):
if not self.os and not self.arch and not self.minor_version:
raise ValueError(
"At least one of os, arch, minor_version must be specified"
)

@classmethod
def all(
cls,
Expand All @@ -125,7 +119,13 @@ def host(cls) -> List["Platform"]:
A list of parsed values which makes the signature the same as
`Platform.all` and `Platform.from_string`.
"""
return [cls(os=OS.interpreter(), arch=Arch.interpreter())]
return [
Platform(
os=OS.interpreter(),
arch=Arch.interpreter(),
minor_version=host_interpreter_minor_version(),
)
]

def all_specializations(self) -> Iterator["Platform"]:
"""Return the platform itself and all its unambiguous specializations.
Expand Down Expand Up @@ -160,9 +160,9 @@ def __lt__(self, other: Any) -> bool:

def __str__(self) -> str:
if self.minor_version is None:
assert (
self.os is not None
), f"if minor_version is None, OS must be specified, got {repr(self)}"
if self.os is None and self.arch is None:
return "//conditions:default"

if self.arch is None:
return f"@platforms//os:{self.os}"
else:
Expand Down Expand Up @@ -207,6 +207,7 @@ def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
minor_version=minor_version,
)
)

else:
ret.update(
cls.all(
Expand Down Expand Up @@ -252,6 +253,8 @@ def platform_system(self) -> str:
return "Darwin"
elif self.os == OS.windows:
return "Windows"
else:
return ""

# derived from OS and Arch
@property
Expand Down Expand Up @@ -416,7 +419,9 @@ def _maybe_add_common_dep(self, dep):
if len(self._target_versions) < 2:
return

platforms = [Platform(minor_version=v) for v in self._target_versions]
platforms = [Platform()] + [
Platform(minor_version=v) for v in self._target_versions
]

# If the dep is targeting all target python versions, lets add it to
# the common dependency list to simplify the select statements.
Expand Down Expand Up @@ -534,14 +539,22 @@ def _add_req(self, req: Requirement, extras: Set[str]) -> None:
):
continue

if match_arch:
if match_arch and self._add_version_select:
self._add(req.name, plat)
if plat.minor_version == host_interpreter_minor_version():
self._add(req.name, Platform(plat.os, plat.arch))
elif match_arch:
self._add(req.name, plat)
elif match_os and self._add_version_select:
self._add(req.name, Platform(plat.os, minor_version=plat.minor_version))
if plat.minor_version == host_interpreter_minor_version():
self._add(req.name, Platform(plat.os))
elif match_os:
self._add(req.name, Platform(plat.os))
elif match_version and self._add_version_select:
self._add(req.name, Platform(minor_version=plat.minor_version))
if plat.minor_version == host_interpreter_minor_version():
self._add(req.name, Platform())
elif match_version:
self._add(req.name, None)

Expand Down