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(bzlmod): only expose common packages via the requirements constants #1955

Merged
merged 2 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,20 @@ A brief description of the categories of changes:
where the runtime for the exec tools toolchain is.

### Fixed
* (bzlmod): When using `experimental_index_url` the `all_requirements`,
`all_whl_requirements` and `all_data_requirements` will now only include
common packages that are available on all target platforms. This is to ensure
that packages that are only present for some platforms are pulled only via
the `deps` of the materialized `py_library`. If you would like to include
platform specific packages, using a `select` statement with references to the
specific package will still work (e.g.
```
my_attr = all_requirements + select(
{
"@platforms//os:linux": ["@pypi//foo_available_only_on_linux"],
"//conditions:default": [],
}
)`.
* (bzlmod): Targets in `all_requirements` now use the same form as targets returned by the `requirement` macro.
* (rules) Auto exec groups are enabled. This allows actions run by the rules,
such as precompiling, to pick an execution platform separately from what
Expand Down
10 changes: 8 additions & 2 deletions python/private/pypi/bzlmod.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ You cannot use both the additive_build_content and additive_build_content_file a
whl_mods = whl_mods,
)

def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, simpleapi_cache):
def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, simpleapi_cache, exposed_packages):
logger = repo_utils.logger(module_ctx)
python_interpreter_target = pip_attr.python_interpreter_target
is_hub_reproducible = True
Expand Down Expand Up @@ -245,7 +245,9 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
if get_index_urls:
# TODO @aignas 2024-05-26: move to a separate function
found_something = False
is_exposed = False
for requirement in requirements:
is_exposed = is_exposed or requirement.is_exposed
for distribution in requirement.whls + [requirement.sdist]:
if not distribution:
# sdist may be None
Expand Down Expand Up @@ -290,6 +292,8 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
)

if found_something:
if is_exposed:
exposed_packages.setdefault(hub_name, {})[whl_name] = None
continue

requirement = select_requirement(
Expand Down Expand Up @@ -431,6 +435,7 @@ def _pip_impl(module_ctx):
# Where hub, whl, and pip are the repo names
hub_whl_map = {}
hub_group_map = {}
exposed_packages = {}

simpleapi_cache = {}
is_extension_reproducible = True
Expand Down Expand Up @@ -470,7 +475,7 @@ def _pip_impl(module_ctx):
else:
pip_hub_map[pip_attr.hub_name].python_versions.append(pip_attr.python_version)

is_hub_reproducible = _create_whl_repos(module_ctx, pip_attr, hub_whl_map, whl_overrides, hub_group_map, simpleapi_cache)
is_hub_reproducible = _create_whl_repos(module_ctx, pip_attr, hub_whl_map, whl_overrides, hub_group_map, simpleapi_cache, exposed_packages)
is_extension_reproducible = is_extension_reproducible and is_hub_reproducible

for hub_name, whl_map in hub_whl_map.items():
Expand All @@ -482,6 +487,7 @@ def _pip_impl(module_ctx):
for key, value in whl_map.items()
},
default_version = _major_minor_version(DEFAULT_PYTHON_VERSION),
packages = sorted(exposed_packages.get(hub_name, {})),
groups = hub_group_map.get(hub_name),
)

Expand Down
8 changes: 7 additions & 1 deletion python/private/pypi/hub_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ exports_files(["requirements.bzl"])
"""

def _impl(rctx):
bzl_packages = rctx.attr.whl_map.keys()
bzl_packages = rctx.attr.packages or rctx.attr.whl_map.keys()
aliases = render_multiplatform_pkg_aliases(
aliases = {
key: [whl_alias(**v) for v in json.decode(values)]
Expand Down Expand Up @@ -77,6 +77,12 @@ setting.""",
"groups": attr.string_list_dict(
mandatory = False,
),
"packages": attr.string_list(
mandatory = False,
doc = """\
The list of packages that will be exposed via all_*requirements macros. Defaults to whl_map keys.
""",
),
"repo_name": attr.string(
mandatory = True,
doc = "The apparent name of the repo. This is needed because in bzlmod, the name attribute becomes the canonical name.",
Expand Down
18 changes: 18 additions & 0 deletions python/private/pypi/parse_requirements.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ def parse_requirements(
* srcs: The Simple API downloadable source list.
* requirement_line: The original requirement line.
* target_platforms: The list of target platforms that this package is for.
* is_exposed: A boolean if the package should be exposed via the hub
repository.

The second element is extra_pip_args should be passed to `whl_library`.
"""
Expand Down Expand Up @@ -266,6 +268,8 @@ def parse_requirements(
for p in DEFAULT_PLATFORMS
if p not in configured_platforms
]
for p in plats:
configured_platforms[p] = file

contents = ctx.read(file)

Expand Down Expand Up @@ -344,6 +348,19 @@ def parse_requirements(

ret = {}
for whl_name, reqs in requirements_by_platform.items():
requirement_target_platforms = {}
for r in reqs.values():
for p in r.target_platforms:
requirement_target_platforms[p] = None

is_exposed = len(requirement_target_platforms) == len(configured_platforms)
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(
whl_name,
sorted(requirement_target_platforms),
sorted(configured_platforms),
))

for r in sorted(reqs.values(), key = lambda r: r.requirement_line):
whls, sdist = _add_dists(
r,
Expand All @@ -362,6 +379,7 @@ def parse_requirements(
download = r.download,
whls = whls,
sdist = sdist,
is_exposed = is_exposed,
),
)

Expand Down
10 changes: 10 additions & 0 deletions tests/pypi/parse_requirements/parse_requirements_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def _test_simple(env):
],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down Expand Up @@ -145,6 +146,7 @@ def _test_platform_markers_with_python_version(env):
],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down Expand Up @@ -181,6 +183,7 @@ def _test_dupe_requirements(env):
],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down Expand Up @@ -220,6 +223,7 @@ def _test_multi_os(env):
target_platforms = ["windows_x86_64"],
whls = [],
sdist = None,
is_exposed = False,
),
],
"foo": [
Expand All @@ -244,6 +248,7 @@ def _test_multi_os(env):
],
whls = [],
sdist = None,
is_exposed = True,
),
struct(
distribution = "foo",
Expand All @@ -258,6 +263,7 @@ def _test_multi_os(env):
target_platforms = ["windows_x86_64"],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down Expand Up @@ -317,6 +323,7 @@ def _test_multi_os_download_only_platform(env):
target_platforms = ["linux_x86_64"],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down Expand Up @@ -371,6 +378,7 @@ def _test_os_arch_requirements_with_default(env):
target_platforms = ["linux_aarch64", "linux_x86_64"],
whls = [],
sdist = None,
is_exposed = True,
),
struct(
distribution = "foo",
Expand All @@ -385,6 +393,7 @@ def _test_os_arch_requirements_with_default(env):
target_platforms = ["linux_super_exotic"],
whls = [],
sdist = None,
is_exposed = True,
),
struct(
distribution = "foo",
Expand All @@ -406,6 +415,7 @@ def _test_os_arch_requirements_with_default(env):
],
whls = [],
sdist = None,
is_exposed = True,
),
],
})
Expand Down