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

feat(bzlmod): cross-platform builds without experimental_index_url #2325

Merged
merged 42 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
4eb639e
feat: reuse the render_pkg_aliases for when filename is not known but…
aignas Oct 22, 2024
ab8b2f0
feat: support generating the extra config settings required
aignas Oct 22, 2024
645885c
feat: get_whl_flag_versions now generates extra args for the rules
aignas Oct 22, 2024
1d33f49
feat: make lock file generation the same irrespective of the host pla…
aignas Oct 22, 2024
9544386
test: add an extra test with multiple requirements files
aignas Oct 22, 2024
b81f86e
doc: CHANGELOG
aignas Oct 22, 2024
9b296d2
bring back code used in workspace
aignas Oct 22, 2024
77754c2
fixup the generation of the config settings
aignas Oct 22, 2024
d9a0d3b
Update python/private/pypi/extension.bzl
aignas Oct 22, 2024
fdfa1de
wip
aignas Oct 22, 2024
f7051ab
wip
aignas Oct 22, 2024
0c058a1
Update CHANGELOG.md
aignas Oct 22, 2024
e5b0190
test: check that parse_requirements handles multiple files with downl…
aignas Oct 23, 2024
0400154
ensure that we can support multi-platform setups with the legacy pip
aignas Oct 23, 2024
c75039d
fix: make the names of the repos more user friendly
aignas Oct 23, 2024
bf4f644
revert docs about cross-platform building
aignas Oct 23, 2024
4de5a45
Revert "revert docs about cross-platform building"
aignas Oct 23, 2024
0d468df
fix the docs to fully document what is possible today
aignas Oct 23, 2024
8d418f4
adjust the code so that the experimental_target_platforms is set in d…
aignas Oct 23, 2024
cfcc8c5
Merge branch 'main' into fix/lockfile-inconsistency
aignas Oct 23, 2024
cf76913
wip
aignas Oct 23, 2024
ecf5a5b
Merge branch 'main' into fix/lockfile-inconsistency
aignas Oct 23, 2024
c4fceb9
fixup test
aignas Oct 23, 2024
9482a13
fixup test
aignas Oct 23, 2024
a520e29
Merge branch 'main' into fix/lockfile-inconsistency
aignas Oct 26, 2024
8578b02
merge conflicts
aignas Oct 26, 2024
de98a02
fix integration tests
aignas Oct 26, 2024
5025f90
Merge branch 'main' into fix/lockfile-inconsistency
aignas Nov 1, 2024
e90d257
fixup! Merge branch 'main' into fix/lockfile-inconsistency
aignas Nov 1, 2024
8835d02
use a different scheme inspired by #2366
aignas Nov 1, 2024
e2c07ae
one more try
aignas Nov 1, 2024
74383f3
fix the tests
aignas Nov 1, 2024
4120c15
simplify
aignas Nov 2, 2024
b4dad40
Merge branch 'main' into fix/lockfile-inconsistency
aignas Nov 3, 2024
7582d5b
revert: start making the change in behaviour opt in
aignas Nov 3, 2024
e5d2ff4
add a flag to use all requirements to make the extension lock file en…
aignas Nov 4, 2024
5c45b4a
fixup! add a flag to use all requirements to make the extension lock …
aignas Nov 4, 2024
58e7a6c
fixup! fixup! add a flag to use all requirements to make the extensio…
aignas Nov 4, 2024
c0fe195
fixup tests
aignas Nov 4, 2024
1ffb52c
wip
aignas Nov 4, 2024
b7577ff
Merge branch 'main' into fix/lockfile-inconsistency
aignas Nov 5, 2024
a2b8b52
wip
aignas Nov 5, 2024
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
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ A brief description of the categories of changes:

{#v0-0-0-fixed}
### Fixed
- Nothing yet
- The extension evaluation has been adjusted to always generate the same lock
file irrespective if `experimental_index_url` is set by any module or not.
Fixes [#2268](https://github.com/bazelbuild/rules_python/issues/2268). A known
issue is that it may break `bazel query` and in these usecases it is advisable
aignas marked this conversation as resolved.
Show resolved Hide resolved
to use `cquery` until we have `sdist` cross-building from source fully working.
Copy link
Collaborator

Choose a reason for hiding this comment

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

download_only=True is also an option, right? Assuming one is OK with not having sdists

Copy link
Collaborator Author

@aignas aignas Oct 22, 2024

Choose a reason for hiding this comment

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

It could be, but I haven't tested with regular requirements files, let me add a unit test to see how that would get wired through.

EDIT: With a few extra modifications, I think we can actually support that. This means that we can resolve #260 and have the stabilization for experimental_index_url done as a separate ticket. I've added extra docs on this, please check. :)


{#v0-0-0-added}
### Added
Expand Down
254 changes: 128 additions & 126 deletions examples/bzlmod/MODULE.bazel.lock

Large diffs are not rendered by default.

17 changes: 16 additions & 1 deletion python/private/pypi/config_settings.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,17 @@ def config_settings(
constraint_values.append("@platforms//cpu:" + cpu)
suffix += "_" + cpu

if suffix:
# Add python version + platform config settings
_dist_config_setting(
name = suffix.strip("_"),
flag_values = {},
python_version = python_version,
is_python = is_python,
visibility = visibility,
native = native,
)

_dist_config_settings(
suffix = suffix,
plat_flag_values = _plat_flag_values(
Expand Down Expand Up @@ -277,7 +288,7 @@ def _plat_flag_values(os, cpu, osx_versions, glibc_versions, muslc_versions):

return ret

def _dist_config_setting(*, name, is_pip_whl, is_python, python_version, native = native, **kwargs):
def _dist_config_setting(*, name, is_python, python_version, is_pip_whl = None, native = native, **kwargs):
"""A macro to create a target that matches is_pip_whl_auto and one more value.

Args:
Expand Down Expand Up @@ -310,6 +321,10 @@ def _dist_config_setting(*, name, is_pip_whl, is_python, python_version, native
# `python_version` setting.
return

if not is_pip_whl:
native.config_setting(name = _name, **kwargs)
return

config_setting_name = _name + "_setting"
native.config_setting(name = config_setting_name, **kwargs)

Expand Down
58 changes: 27 additions & 31 deletions python/private/pypi/extension.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ load("//python/private:version_label.bzl", "version_label")
load(":attrs.bzl", "use_isolated")
load(":evaluate_markers.bzl", "evaluate_markers", EVALUATE_MARKERS_SRCS = "SRCS")
load(":hub_repository.bzl", "hub_repository")
load(":parse_requirements.bzl", "host_platform", "parse_requirements", "select_requirement")
load(":parse_requirements.bzl", "parse_requirements")
load(":parse_whl_name.bzl", "parse_whl_name")
load(":pip_repository_attrs.bzl", "ATTRS")
load(":render_pkg_aliases.bzl", "whl_alias")
Expand Down Expand Up @@ -212,7 +212,6 @@ def _create_whl_repos(
logger = logger,
)

repository_platform = host_platform(module_ctx)
for whl_name, requirements in requirements_by_platform.items():
# We are not using the "sanitized name" because the user
# would need to guess what name we modified the whl name
Expand Down Expand Up @@ -260,10 +259,10 @@ def _create_whl_repos(
if v != default
})

is_exposed = False
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
dists = requirement.whls
Expand Down Expand Up @@ -319,35 +318,32 @@ def _create_whl_repos(
exposed_packages[whl_name] = None
continue

requirement = select_requirement(
requirements,
platform = None if pip_attr.download_only else repository_platform,
)
if not requirement:
# Sometimes the package is not present for host platform if there
# are whls specified only in particular requirements files, in that
# case just continue, however, if the download_only flag is set up,
# then the user can also specify the target platform of the wheel
# packages they want to download, in that case there will be always
# a requirement here, so we will not be in this code branch.
continue
elif get_index_urls:
logger.warn(lambda: "falling back to pip for installing the right file for {}".format(requirement.requirement_line))

whl_library_args["requirement"] = requirement.requirement_line
if requirement.extra_pip_args:
whl_library_args["extra_pip_args"] = requirement.extra_pip_args
for i, requirement in enumerate(requirements):
rickeylev marked this conversation as resolved.
Show resolved Hide resolved
is_exposed = is_exposed or requirement.is_exposed
if get_index_urls:
logger.warn(lambda: "falling back to pip for installing the right file for {}".format(requirement.requirement_line))

whl_library_args["requirement"] = requirement.requirement_line
if requirement.extra_pip_args:
whl_library_args["extra_pip_args"] = requirement.extra_pip_args

# We sort so that the lock-file remains the same no matter the order of how the
# args are manipulated in the code going before.
aignas marked this conversation as resolved.
Show resolved Hide resolved
repo_name = "{}_{}".format(pip_name, whl_name)
if len(requirements) > 1:
repo_name = "{}__{}".format(repo_name, i)

whl_libraries[repo_name] = dict(whl_library_args.items())
whl_map.setdefault(whl_name, []).append(
whl_alias(
repo = repo_name,
version = major_minor,
target_platforms = requirement.target_platforms if len(requirements) > 1 else None,
),
)

# We sort so that the lock-file remains the same no matter the order of how the
# args are manipulated in the code going before.
repo_name = "{}_{}".format(pip_name, whl_name)
whl_libraries[repo_name] = dict(whl_library_args.items())
whl_map.setdefault(whl_name, []).append(
whl_alias(
repo = repo_name,
version = major_minor,
),
)
if is_exposed:
exposed_packages[whl_name] = None

return struct(
is_reproducible = is_reproducible,
Expand Down
4 changes: 4 additions & 0 deletions python/private/pypi/parse_requirements.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ def parse_requirements(
def select_requirement(requirements, *, platform):
"""A simple function to get a requirement for a particular platform.

Only used in WORKSPACE.

Args:
requirements (list[struct]): The list of requirements as returned by
the `parse_requirements` function above.
Expand Down Expand Up @@ -243,6 +245,8 @@ def select_requirement(requirements, *, platform):
def host_platform(ctx):
"""Return a string representation of the repository OS.

Only used in WORKSPACE.

Args:
ctx (struct): The `module_ctx` or `repository_ctx` attribute.

Expand Down
58 changes: 31 additions & 27 deletions python/private/pypi/render_pkg_aliases.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -318,16 +318,19 @@ def multiplatform_whl_aliases(*, aliases, **kwargs):
ret = []
versioned_additions = {}
for alias in aliases:
if not alias.filename:
if not alias.filename and not alias.target_platforms:
ret.append(alias)
continue

config_settings, all_versioned_settings = get_filename_config_settings(
# TODO @aignas 2024-05-27: pass the parsed whl to reduce the
# number of duplicate operations.
filename = alias.filename,
filename = alias.filename or "",
target_platforms = alias.target_platforms,
python_version = alias.version,
# If we have multiple platforms but no wheel filename, lets use different
# config settings.
non_whl_prefix = "sdist" if alias.filename else "",
**kwargs
)

Expand Down Expand Up @@ -429,10 +432,7 @@ def get_whl_flag_versions(aliases):
if a.version:
python_versions[a.version] = None

if not a.filename:
continue

if a.filename.endswith(".whl") and not a.filename.endswith("-any.whl"):
if a.filename and a.filename.endswith(".whl") and not a.filename.endswith("-any.whl"):
parsed = parse_whl_name(a.filename)
else:
for plat in a.target_platforms or []:
Expand Down Expand Up @@ -491,10 +491,11 @@ def get_filename_config_settings(
*,
filename,
target_platforms,
glibc_versions,
muslc_versions,
osx_versions,
python_version):
python_version,
glibc_versions = None,
muslc_versions = None,
osx_versions = None,
non_whl_prefix = "sdist"):
"""Get the filename config settings.

Args:
Expand All @@ -504,6 +505,8 @@ def get_filename_config_settings(
muslc_versions: list[tuple[int, int]], list of versions.
osx_versions: list[tuple[int, int]], list of versions.
python_version: the python version to generate the config_settings for.
non_whl_prefix: the prefix of the config setting when the whl we don't have
a filename ending with ".whl".

Returns:
A tuple:
Expand All @@ -512,19 +515,20 @@ def get_filename_config_settings(
"""
prefixes = []
suffixes = []
if (0, 0) in glibc_versions:
fail("Invalid version in 'glibc_versions': cannot specify (0, 0) as a value")
if (0, 0) in muslc_versions:
fail("Invalid version in 'muslc_versions': cannot specify (0, 0) as a value")
if (0, 0) in osx_versions:
fail("Invalid version in 'osx_versions': cannot specify (0, 0) as a value")

glibc_versions = sorted(glibc_versions)
muslc_versions = sorted(muslc_versions)
osx_versions = sorted(osx_versions)
setting_supported_versions = {}

if filename.endswith(".whl"):
if (0, 0) in glibc_versions:
fail("Invalid version in 'glibc_versions': cannot specify (0, 0) as a value")
if (0, 0) in muslc_versions:
fail("Invalid version in 'muslc_versions': cannot specify (0, 0) as a value")
if (0, 0) in osx_versions:
fail("Invalid version in 'osx_versions': cannot specify (0, 0) as a value")

glibc_versions = sorted(glibc_versions)
muslc_versions = sorted(muslc_versions)
osx_versions = sorted(osx_versions)

parsed = parse_whl_name(filename)
if parsed.python_tag == "py2.py3":
py = "py"
Expand All @@ -539,10 +543,10 @@ def get_filename_config_settings(
abi = parsed.abi_tag

if parsed.platform_tag == "any":
prefixes = ["{}_{}_any".format(py, abi)]
prefixes = ["_{}_{}_any".format(py, abi)]
suffixes = [_non_versioned_platform(p) for p in target_platforms or []]
else:
prefixes = ["{}_{}".format(py, abi)]
prefixes = ["_{}_{}".format(py, abi)]
suffixes = _whl_config_setting_suffixes(
platform_tag = parsed.platform_tag,
glibc_versions = glibc_versions,
Expand All @@ -551,22 +555,22 @@ def get_filename_config_settings(
setting_supported_versions = setting_supported_versions,
)
else:
prefixes = ["sdist"]
prefixes = [""] if not non_whl_prefix else ["_" + non_whl_prefix]
suffixes = [_non_versioned_platform(p) for p in target_platforms or []]

versioned = {
":is_cp{}_{}_{}".format(python_version, p, suffix): {
version: ":is_cp{}_{}_{}".format(python_version, p, setting)
":is_cp{}{}_{}".format(python_version, p, suffix): {
version: ":is_cp{}{}_{}".format(python_version, p, setting)
for version, setting in versions.items()
}
for p in prefixes
for suffix, versions in setting_supported_versions.items()
}

if suffixes or versioned:
return [":is_cp{}_{}_{}".format(python_version, p, s) for p in prefixes for s in suffixes], versioned
return [":is_cp{}{}_{}".format(python_version, p, s) for p in prefixes for s in suffixes], versioned
else:
return [":is_cp{}_{}".format(python_version, p) for p in prefixes], setting_supported_versions
return [":is_cp{}{}".format(python_version, p) for p in prefixes], setting_supported_versions

def _whl_config_setting_suffixes(
platform_tag,
Expand Down
92 changes: 89 additions & 3 deletions tests/pypi/extension/extension_tests.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,100 @@ def _test_simple(env):
)

pypi.is_reproducible().equals(True)
pypi.exposed_packages().contains_exactly({"pypi": []})
pypi.exposed_packages().contains_exactly({"pypi": ["simple"]})
pypi.hub_group_map().contains_exactly({"pypi": {}})
pypi.hub_whl_map().contains_exactly({"pypi": {}})
pypi.whl_libraries().contains_exactly({})
pypi.hub_whl_map().contains_exactly({"pypi": {
"simple": [
struct(
config_setting = "//_config:is_python_3.15",
filename = None,
repo = "pypi_315_simple",
target_platforms = None,
version = "3.15",
),
],
}})
pypi.whl_libraries().contains_exactly({
"pypi_315_simple": {
"dep_template": "@pypi//{name}:{target}",
"python_interpreter_target": "unit_test_interpreter_target",
"repo": "pypi_315",
"requirement": "simple==0.0.1 --hash=sha256:deadbeef",
},
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am happy that my selection of the mock ctx.os values was already validating the bug in question.

pypi.whl_mods().contains_exactly({})

_tests.append(_test_simple)

def _test_simple_multiple_requirements(env):
pypi = _parse_modules(
env,
module_ctx = _mock_mctx(
_mod(
name = "rules_python",
parse = [
_parse(
hub_name = "pypi",
python_version = "3.15",
requirements_darwin = "darwin.txt",
requirements_windows = "win.txt",
),
],
),
read = lambda x: {
"darwin.txt": "simple==0.0.2 --hash=sha256:deadb00f",
"win.txt": "simple==0.0.1 --hash=sha256:deadbeef",
}[x],
),
available_interpreters = {
"python_3_15_host": "unit_test_interpreter_target",
},
)

pypi.is_reproducible().equals(True)
pypi.exposed_packages().contains_exactly({"pypi": ["simple"]})
pypi.hub_group_map().contains_exactly({"pypi": {}})
pypi.hub_whl_map().contains_exactly({"pypi": {
"simple": [
struct(
config_setting = "//_config:is_python_3.15",
filename = None,
repo = "pypi_315_simple__0",
target_platforms = [
"cp315_windows_x86_64",
],
version = "3.15",
),
struct(
config_setting = "//_config:is_python_3.15",
filename = None,
repo = "pypi_315_simple__1",
target_platforms = [
"cp315_osx_aarch64",
"cp315_osx_x86_64",
],
version = "3.15",
),
],
}})
pypi.whl_libraries().contains_exactly({
"pypi_315_simple__0": {
"dep_template": "@pypi//{name}:{target}",
"python_interpreter_target": "unit_test_interpreter_target",
"repo": "pypi_315",
"requirement": "simple==0.0.1 --hash=sha256:deadbeef",
},
"pypi_315_simple__1": {
"dep_template": "@pypi//{name}:{target}",
"python_interpreter_target": "unit_test_interpreter_target",
"repo": "pypi_315",
"requirement": "simple==0.0.2 --hash=sha256:deadb00f",
},
})
pypi.whl_mods().contains_exactly({})

_tests.append(_test_simple_multiple_requirements)

def _test_simple_get_index(env):
got_simpleapi_download_args = []
got_simpleapi_download_kwargs = {}
Expand Down
Loading