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.

Sign up for GitHub

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

Jump to bottom

refactor(pypi): split out more utils and start passing 'abi_os_arch' around #2069

Merged
merged 1 commit into from
Jul 17, 2024
Merged

refactor(pypi): split out more utils and start passing 'abi_os_arch' around #2069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions python/private/pypi/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ bzl_library(
deps = [
":index_sources_bzl",
":parse_requirements_txt_bzl",
":pypi_repo_utils_bzl",
":requirements_files_by_platform_bzl",
":whl_target_platforms_bzl",
"//python/private:normalize_name_bzl",
],
Expand Down Expand Up @@ -227,6 +229,15 @@ bzl_library(
srcs = ["pip_repository_attrs.bzl"],
)

bzl_library(
name = "pypi_repo_utils_bzl",
srcs = ["pypi_repo_utils.bzl"],
deps = [
"//python:versions_bzl",
"//python/private:toolchains_repo_bzl",
],
)

bzl_library(
name = "render_pkg_aliases_bzl",
srcs = ["render_pkg_aliases.bzl"],
Expand All @@ -240,6 +251,14 @@ bzl_library(
],
)

bzl_library(
name = "requirements_files_by_platform_bzl",
srcs = ["requirements_files_by_platform.bzl"],
deps = [
":whl_target_platforms_bzl",
],
)

bzl_library(
name = "simpleapi_download_bzl",
srcs = ["simpleapi_download.bzl"],
Expand Down Expand Up @@ -270,13 +289,12 @@ bzl_library(
":generate_whl_library_build_bazel_bzl",
":parse_whl_name_bzl",
":patch_whl_bzl",
":pypi_repo_utils_bzl",
":whl_target_platforms_bzl",
"//python:repositories_bzl",
"//python:versions_bzl",
"//python/private:auth_bzl",
"//python/private:envsubst_bzl",
"//python/private:repo_utils_bzl",
"//python/private:toolchains_repo_bzl",
],
)

Expand Down
19 changes: 12 additions & 7 deletions python/private/pypi/extension.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ load(":parse_requirements.bzl", "host_platform", "parse_requirements", "select_r
load(":parse_whl_name.bzl", "parse_whl_name")
load(":pip_repository_attrs.bzl", "ATTRS")
load(":render_pkg_aliases.bzl", "whl_alias")
load(":requirements_files_by_platform.bzl", "requirements_files_by_platform")
load(":simpleapi_download.bzl", "simpleapi_download")
load(":whl_library.bzl", "whl_library")
load(":whl_repo_name.bzl", "whl_repo_name")
Expand Down Expand Up @@ -183,12 +184,16 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s

requirements_by_platform = parse_requirements(
module_ctx,
requirements_by_platform = pip_attr.requirements_by_platform,
requirements_linux = pip_attr.requirements_linux,
requirements_lock = pip_attr.requirements_lock,
requirements_osx = pip_attr.requirements_darwin,
requirements_windows = pip_attr.requirements_windows,
extra_pip_args = pip_attr.extra_pip_args,
requirements_by_platform = requirements_files_by_platform(
requirements_by_platform = pip_attr.requirements_by_platform,
requirements_linux = pip_attr.requirements_linux,
requirements_lock = pip_attr.requirements_lock,
requirements_osx = pip_attr.requirements_darwin,
requirements_windows = pip_attr.requirements_windows,
extra_pip_args = pip_attr.extra_pip_args,
python_version = major_minor,
logger = logger,
),
Comment on lines -186 to +196
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main idea is to normalize the pip_attr.requirements* attributes to a dict[Label, list[str]]. This makes the downstream code much simpler.

get_index_urls = get_index_urls,
python_version = major_minor,
logger = logger,
Expand Down Expand Up @@ -298,7 +303,7 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s

requirement = select_requirement(
requirements,
platform = repository_platform,
platform = None if pip_attr.download_only else repository_platform,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If pip_attr.download_only == True then we want to just select the first requirement out of requirements array. Previously the code was burried and we were carrying to much data around. The simplification here means that select_requirement does not need to care about whe download_only value anywhere.

)
if not requirement:
# Sometimes the package is not present for host platform if there
Expand Down
178 changes: 17 additions & 161 deletions python/private/pypi/parse_requirements.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ behavior.
load("//python/private:normalize_name.bzl", "normalize_name")
load(":index_sources.bzl", "index_sources")
load(":parse_requirements_txt.bzl", "parse_requirements_txt")
load(":whl_target_platforms.bzl", "select_whls", "whl_target_platforms")
load(":whl_target_platforms.bzl", "select_whls")

# This includes the vendored _translate_cpu and _translate_os from
# @platforms//host:extension.bzl at version 0.0.9 so that we don't
Expand Down Expand Up @@ -80,72 +80,10 @@ DEFAULT_PLATFORMS = [
"windows_x86_64",
]

def _default_platforms(*, filter):
if not filter:
fail("Must specific a filter string, got: {}".format(filter))

if filter.startswith("cp3"):
# TODO @aignas 2024-05-23: properly handle python versions in the filter.
# For now we are just dropping it to ensure that we don't fail.
_, _, filter = filter.partition("_")

sanitized = filter.replace("*", "").replace("_", "")
if sanitized and not sanitized.isalnum():
fail("The platform filter can only contain '*', '_' and alphanumerics")

if "*" in filter:
prefix = filter.rstrip("*")
if "*" in prefix:
fail("The filter can only contain '*' at the end of it")

if not prefix:
return DEFAULT_PLATFORMS

return [p for p in DEFAULT_PLATFORMS if p.startswith(prefix)]
else:
return [p for p in DEFAULT_PLATFORMS if filter in p]

def _platforms_from_args(extra_pip_args):
platform_values = []

for arg in extra_pip_args:
if platform_values and platform_values[-1] == "":
platform_values[-1] = arg
continue

if arg == "--platform":
platform_values.append("")
continue

if not arg.startswith("--platform"):
continue

_, _, plat = arg.partition("=")
if not plat:
_, _, plat = arg.partition(" ")
if plat:
platform_values.append(plat)
else:
platform_values.append("")

if not platform_values:
return []

platforms = {
p.target_platform: None
for arg in platform_values
for p in whl_target_platforms(arg)
}
return list(platforms.keys())

def parse_requirements(
ctx,
*,
requirements_by_platform = {},
requirements_osx = None,
requirements_linux = None,
requirements_lock = None,
requirements_windows = None,
extra_pip_args = [],
get_index_urls = None,
python_version = None,
Expand All @@ -158,10 +96,6 @@ def parse_requirements(
requirements_by_platform (label_keyed_string_dict): a way to have
different package versions (or different packages) for different
os, arch combinations.
requirements_osx (label): The requirements file for the osx OS.
requirements_linux (label): The requirements file for the linux OS.
requirements_lock (label): The requirements file for all OSes, or used as a fallback.
requirements_windows (label): The requirements file for windows OS.
extra_pip_args (string list): Extra pip arguments to perform extra validations and to
be joined with args fined in files.
get_index_urls: Callable[[ctx, list[str]], dict], a callable to get all
Expand All @@ -186,91 +120,11 @@ def parse_requirements(

The second element is extra_pip_args should be passed to `whl_library`.
"""
if not (
requirements_lock or
requirements_linux or
requirements_osx or
requirements_windows or
requirements_by_platform
):
fail_fn(
"A 'requirements_lock' attribute must be specified, a platform-specific lockfiles " +
"via 'requirements_by_platform' or an os-specific lockfiles must be specified " +
"via 'requirements_*' attributes",
)
return None

platforms = _platforms_from_args(extra_pip_args)

if platforms:
lock_files = [
f
for f in [
requirements_lock,
requirements_linux,
requirements_osx,
requirements_windows,
] + list(requirements_by_platform.keys())
if f
]

if len(lock_files) > 1:
# If the --platform argument is used, check that we are using
# a single `requirements_lock` file instead of the OS specific ones as that is
# the only correct way to use the API.
fail_fn("only a single 'requirements_lock' file can be used when using '--platform' pip argument, consider specifying it via 'requirements_lock' attribute")
return None

files_by_platform = [
(lock_files[0], platforms),
]
else:
files_by_platform = {
file: [
platform
for filter_or_platform in specifier.split(",")
for platform in (_default_platforms(filter = filter_or_platform) if filter_or_platform.endswith("*") else [filter_or_platform])
]
for file, specifier in requirements_by_platform.items()
}.items()

for f in [
# If the users need a greater span of the platforms, they should consider
# using the 'requirements_by_platform' attribute.
(requirements_linux, _default_platforms(filter = "linux_*")),
(requirements_osx, _default_platforms(filter = "osx_*")),
(requirements_windows, _default_platforms(filter = "windows_*")),
(requirements_lock, None),
]:
if f[0]:
files_by_platform.append(f)

configured_platforms = {}

options = {}
requirements = {}
for file, plats in files_by_platform:
if plats:
for p in plats:
if p in configured_platforms:
fail_fn(
"Expected the platform '{}' to be map only to a single requirements file, but got multiple: '{}', '{}'".format(
p,
configured_platforms[p],
file,
),
)
return None
configured_platforms[p] = file
else:
plats = [
p
for p in DEFAULT_PLATFORMS
if p not in configured_platforms
]
for p in plats:
configured_platforms[p] = file

for file, plats in requirements_by_platform.items():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code above just got moved to a different file. The tests got also moved.

if logger:
logger.debug(lambda: "Using {} for {}".format(file, plats))
contents = ctx.read(file)

# Parse the requirements file directly in starlark to get the information
Expand Down Expand Up @@ -303,9 +157,9 @@ def parse_requirements(
tokenized_options.append(p)

pip_args = tokenized_options + extra_pip_args
for p in plats:
requirements[p] = requirements_dict
options[p] = pip_args
for plat in plats:
requirements[plat] = requirements_dict
options[plat] = pip_args

requirements_by_platform = {}
for target_platform, reqs_ in requirements.items():
Expand All @@ -325,7 +179,6 @@ def parse_requirements(
requirement_line = requirement_line,
target_platforms = [],
extra_pip_args = extra_pip_args,
download = len(platforms) > 0,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This attribute got removed as it is no longer needed due to the changes in the select_requirement function.

),
)
for_req.target_platforms.append(target_platform)
Expand Down Expand Up @@ -353,12 +206,12 @@ def parse_requirements(
for p in r.target_platforms:
requirement_target_platforms[p] = None

is_exposed = len(requirement_target_platforms) == len(configured_platforms)
is_exposed = len(requirement_target_platforms) == len(requirements)
if not is_exposed and logger:
logger.debug(lambda: "Package {} will not be exposed because it is only present on a subset of platforms: {} out of {}".format(
logger.debug(lambda: "Package '{}' will not be exposed because it is only present on a subset of platforms: {} out of {}".format(
whl_name,
sorted(requirement_target_platforms),
sorted(configured_platforms),
sorted(requirements),
))

for r in sorted(reqs.values(), key = lambda r: r.requirement_line):
Expand All @@ -376,13 +229,15 @@ def parse_requirements(
requirement_line = r.requirement_line,
target_platforms = sorted(r.target_platforms),
extra_pip_args = r.extra_pip_args,
download = r.download,
whls = whls,
sdist = sdist,
is_exposed = is_exposed,
),
)

if logger:
logger.debug(lambda: "Will configure whl repos: {}".format(ret.keys()))

return ret

def select_requirement(requirements, *, platform):
Expand All @@ -391,8 +246,9 @@ def select_requirement(requirements, *, platform):
Args:
requirements (list[struct]): The list of requirements as returned by
the `parse_requirements` function above.
platform (str): The host platform. Usually an output of the
`host_platform` function.
platform (str or None): The host platform. Usually an output of the
`host_platform` function. If None, then this function will return
the first requirement it finds.

Returns:
None if not found or a struct returned as one of the values in the
Expand All @@ -402,7 +258,7 @@ def select_requirement(requirements, *, platform):
maybe_requirement = [
req
for req in requirements
if platform in req.target_platforms or req.download
if not platform or [p for p in req.target_platforms if p.endswith(platform)]
]
if not maybe_requirement:
# Sometimes the package is not present for host platform if there
Expand Down
16 changes: 10 additions & 6 deletions python/private/pypi/pip_repository.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ load("//python/private:text_util.bzl", "render")
load(":parse_requirements.bzl", "host_platform", "parse_requirements", "select_requirement")
load(":pip_repository_attrs.bzl", "ATTRS")
load(":render_pkg_aliases.bzl", "render_pkg_aliases", "whl_alias")
load(":requirements_files_by_platform.bzl", "requirements_files_by_platform")

def _get_python_interpreter_attr(rctx):
"""A helper function for getting the `python_interpreter` attribute or it's default
Expand Down Expand Up @@ -71,11 +72,14 @@ exports_files(["requirements.bzl"])
def _pip_repository_impl(rctx):
requirements_by_platform = parse_requirements(
rctx,
requirements_by_platform = rctx.attr.requirements_by_platform,
requirements_linux = rctx.attr.requirements_linux,
requirements_lock = rctx.attr.requirements_lock,
requirements_osx = rctx.attr.requirements_darwin,
requirements_windows = rctx.attr.requirements_windows,
requirements_by_platform = requirements_files_by_platform(
requirements_by_platform = rctx.attr.requirements_by_platform,
requirements_linux = rctx.attr.requirements_linux,
requirements_lock = rctx.attr.requirements_lock,
requirements_osx = rctx.attr.requirements_darwin,
requirements_windows = rctx.attr.requirements_windows,
extra_pip_args = rctx.attr.extra_pip_args,
),
extra_pip_args = rctx.attr.extra_pip_args,
)
selected_requirements = {}
Expand All @@ -84,7 +88,7 @@ def _pip_repository_impl(rctx):
for name, requirements in requirements_by_platform.items():
r = select_requirement(
requirements,
platform = repository_platform,
platform = None if rctx.attr.download_only else repository_platform,
)
if not r:
continue
Expand Down
Loading