feat(pypi): actually start using env_marker_setting (#2873)

Summary:
- `pep508_deps` is now much simpler, because the hard work is done in
  analysis phase
- `whl_library` BUILD.bazel tests now also have a test for the legacy
  flow.

One thing that I noticed is that now we have an implicit dependency on
the python toolchain when getting all of the `whl` target tree. This is
a filegroup target that includes dependent wheels. However, we fallback
to the flag values if we don't have the toolchain, so we should be good
in general.

Overall I like how this is turning out because we don't need to pipe the
`target_platforms` anymore when we enable `PIPSTAR` feature. This means
that we can start creating fewer whl_library instances - e.g. a
`py3-none-any` wheel can be fetched once instead of once per python
interpreter version. I'll leave this optimization for a later time.

Work towards #260

---------

Co-authored-by: Richard Levasseur <[email protected]>

Author: aignas

python/private/pypi/BUILD.bazel

@@ -236,13 +236,9 @@ bzl_library(
     name = "pep508_deps_bzl",
     srcs = ["pep508_deps.bzl"],
     deps = [
-        ":pep508_env_bzl",
         ":pep508_evaluate_bzl",
-        ":pep508_platform_bzl",
         ":pep508_requirement_bzl",
-        "//python/private:full_version_bzl",
         "//python/private:normalize_name_bzl",
-        "@pythons_hub//:versions_bzl",
     ],
 )
 

python/private/pypi/config.bzl.tmpl.bzlmod

@@ -6,4 +6,4 @@ with your usecase. This may change in between rules_python versions without any
 @generated by rules_python pip.parse bzlmod extension.
 """
 
-target_platforms = %%TARGET_PLATFORMS%%
+whl_map = %%WHL_MAP%%

python/private/pypi/extension.bzl

@@ -17,6 +17,7 @@
 load("@bazel_features//:features.bzl", "bazel_features")
 load("@pythons_hub//:interpreters.bzl", "INTERPRETER_LABELS")
 load("@pythons_hub//:versions.bzl", "MINOR_MAPPING")
+load("@rules_python_internal//:rules_python_config.bzl", rp_config = "config")
 load("//python/private:auth.bzl", "AUTH_ATTRS")
 load("//python/private:full_version.bzl", "full_version")
 load("//python/private:normalize_name.bzl", "normalize_name")
@@ -72,7 +73,8 @@ def _create_whl_repos(
         available_interpreters = INTERPRETER_LABELS,
         minor_mapping = MINOR_MAPPING,
         evaluate_markers = evaluate_markers_py,
-        get_index_urls = None):
+        get_index_urls = None,
+        enable_pipstar = False):
     """create all of the whl repositories
 
     Args:
@@ -87,6 +89,7 @@ def _create_whl_repos(
         minor_mapping: {type}`dict[str, str]` The dictionary needed to resolve the full
             python version used to parse package METADATA files.
         evaluate_markers: the function used to evaluate the markers.
+        enable_pipstar: enable the pipstar feature.
 
     Returns a {type}`struct` with the following attributes:
         whl_map: {type}`dict[str, list[struct]]` the output is keyed by the
@@ -216,7 +219,6 @@ def _create_whl_repos(
             enable_implicit_namespace_pkgs = pip_attr.enable_implicit_namespace_pkgs,
             environment = pip_attr.environment,
             envsubst = pip_attr.envsubst,
-            experimental_target_platforms = pip_attr.experimental_target_platforms,
             group_deps = group_deps,
             group_name = group_name,
             pip_data_exclude = pip_attr.pip_data_exclude,
@@ -227,6 +229,9 @@ def _create_whl_repos(
                 for p, args in whl_overrides.get(whl_name, {}).items()
             },
         )
+        if not enable_pipstar:
+            maybe_args["experimental_target_platforms"] = pip_attr.experimental_target_platforms
+
         whl_library_args.update({k: v for k, v in maybe_args.items() if v})
         maybe_args_with_default = dict(
             # The following values have defaults next to them
@@ -249,6 +254,7 @@ def _create_whl_repos(
                 auth_patterns = pip_attr.auth_patterns,
                 python_version = major_minor,
                 multiple_requirements_for_whl = len(requirements) > 1.,
+                enable_pipstar = enable_pipstar,
             ).items():
                 repo_name = "{}_{}".format(pip_name, repo_name)
                 if repo_name in whl_libraries:
@@ -277,7 +283,7 @@ def _create_whl_repos(
         },
     )
 
-def _whl_repos(*, requirement, whl_library_args, download_only, netrc, auth_patterns, multiple_requirements_for_whl = False, python_version):
+def _whl_repos(*, requirement, whl_library_args, download_only, netrc, auth_patterns, multiple_requirements_for_whl = False, python_version, enable_pipstar = False):
     ret = {}
 
     dists = requirement.whls
@@ -305,13 +311,14 @@ def _whl_repos(*, requirement, whl_library_args, download_only, netrc, auth_patt
         args["urls"] = [distribution.url]
         args["sha256"] = distribution.sha256
         args["filename"] = distribution.filename
-        args["experimental_target_platforms"] = [
-            # Get rid of the version fot the target platforms because we are
-            # passing the interpreter any way. Ideally we should search of ways
-            # how to pass the target platforms through the hub repo.
-            p.partition("_")[2]
-            for p in requirement.target_platforms
-        ]
+        if not enable_pipstar:
+            args["experimental_target_platforms"] = [
+                # Get rid of the version fot the target platforms because we are
+                # passing the interpreter any way. Ideally we should search of ways
+                # how to pass the target platforms through the hub repo.
+                p.partition("_")[2]
+                for p in requirement.target_platforms
+            ]
 
         # Pure python wheels or sdists may need to have a platform here
         target_platforms = None
@@ -357,7 +364,11 @@ def _whl_repos(*, requirement, whl_library_args, download_only, netrc, auth_patt
 
     return ret
 
-def parse_modules(module_ctx, _fail = fail, simpleapi_download = simpleapi_download, **kwargs):
+def parse_modules(
+        module_ctx,
+        _fail = fail,
+        simpleapi_download = simpleapi_download,
+        **kwargs):
     """Implementation of parsing the tag classes for the extension and return a struct for registering repositories.
 
     Args:
@@ -639,7 +650,7 @@ def _pip_impl(module_ctx):
         module_ctx: module contents
     """
 
-    mods = parse_modules(module_ctx)
+    mods = parse_modules(module_ctx, enable_pipstar = rp_config.enable_pipstar)
 
     # Build all of the wheel modifications if the tag class is called.
     _whl_mods_impl(mods.whl_mods)

python/private/pypi/generate_whl_library_build_bazel.bzl

@@ -26,10 +26,11 @@ _RENDER = {
     "entry_points": render.dict,
     "extras": render.list,
     "group_deps": render.list,
+    "include": str,
     "requires_dist": render.list,
     "srcs_exclude": render.list,
     "tags": render.list,
-    "target_platforms": lambda x: render.list(x) if x else "target_platforms",
+    "target_platforms": render.list,
 }
 
 # NOTE @aignas 2024-10-25: We have to keep this so that files in
@@ -62,28 +63,44 @@ def generate_whl_library_build_bazel(
         A complete BUILD file as a string
     """
 
-    fn = "whl_library_targets"
+    loads = []
     if kwargs.get("tags"):
+        fn = "whl_library_targets"
+
         # legacy path
         unsupported_args = [
             "requires",
             "metadata_name",
             "metadata_version",
+            "include",
         ]
     else:
-        fn = "{}_from_requires".format(fn)
+        fn = "whl_library_targets_from_requires"
         unsupported_args = [
             "dependencies",
             "dependencies_by_platform",
+            "target_platforms",
+            "default_python_version",
         ]
+        dep_template = kwargs.get("dep_template")
+        loads.append(
+            """load("{}", "{}")""".format(
+                dep_template.format(
+                    name = "",
+                    target = "config.bzl",
+                ),
+                "whl_map",
+            ),
+        )
+        kwargs["include"] = "whl_map"
 
     for arg in unsupported_args:
         if kwargs.get(arg):
             fail("BUG, unsupported arg: '{}'".format(arg))
 
-    loads = [
+    loads.extend([
         """load("@rules_python//python/private/pypi:whl_library_targets.bzl", "{}")""".format(fn),
-    ]
+    ])
 
     additional_content = []
     if annotation:

python/private/pypi/hub_repository.bzl

@@ -49,7 +49,7 @@ def _impl(rctx):
         "config.bzl",
         rctx.attr._config_template,
         substitutions = {
-            "%%TARGET_PLATFORMS%%": render.list(rctx.attr.target_platforms),
+            "%%WHL_MAP%%": render.dict(rctx.attr.whl_map, value_repr = lambda x: "None"),
         },
     )
     rctx.template("requirements.bzl", rctx.attr._requirements_bzl_template, substitutions = {

python/private/pypi/pep508_deps.bzl

@@ -15,36 +15,27 @@
 """This module is for implementing PEP508 compliant METADATA deps parsing.
 """
 
-load("@pythons_hub//:versions.bzl", "DEFAULT_PYTHON_VERSION", "MINOR_MAPPING")
-load("//python/private:full_version.bzl", "full_version")
 load("//python/private:normalize_name.bzl", "normalize_name")
-load(":pep508_env.bzl", "env")
 load(":pep508_evaluate.bzl", "evaluate")
-load(":pep508_platform.bzl", "platform", "platform_from_str")
 load(":pep508_requirement.bzl", "requirement")
 
 def deps(
         name,
         *,
         requires_dist,
-        platforms = [],
         extras = [],
         excludes = [],
-        default_python_version = None,
-        minor_mapping = MINOR_MAPPING):
+        include = []):
     """Parse the RequiresDist from wheel METADATA
 
     Args:
         name: {type}`str` the name of the wheel.
         requires_dist: {type}`list[str]` the list of RequiresDist lines from the
             METADATA file.
         excludes: {type}`list[str]` what packages should we exclude.
+        include: {type}`list[str]` what packages should we exclude. If it is not
+            specified, then we will include all deps from `requires_dist`.
         extras: {type}`list[str]` the requested extras to generate targets for.
-        platforms: {type}`list[str]` the list of target platform strings.
-        default_python_version: {type}`str` the host python version.
-        minor_mapping: {type}`type[str, str]` the minor mapping to use when
-            resolving to the full python version as DEFAULT_PYTHON_VERSION can by
-            of format `3.x`.
 
     Returns:
         A struct with attributes:
@@ -60,39 +51,20 @@ def deps(
     deps_select = {}
     name = normalize_name(name)
     want_extras = _resolve_extras(name, reqs, extras)
+    include = [normalize_name(n) for n in include]
 
     # drop self edges
     excludes = [name] + [normalize_name(x) for x in excludes]
 
-    default_python_version = default_python_version or DEFAULT_PYTHON_VERSION
-    if default_python_version:
-        # if it is not bzlmod, then DEFAULT_PYTHON_VERSION may be unset
-        default_python_version = full_version(
-            version = default_python_version,
-            minor_mapping = minor_mapping,
-        )
-    platforms = [
-        platform_from_str(p, python_version = default_python_version)
-        for p in platforms
-    ]
-
-    abis = sorted({p.abi: True for p in platforms if p.abi})
-    if default_python_version and len(abis) > 1:
-        _, _, tail = default_python_version.partition(".")
-        default_abi = "cp3" + tail
-    elif len(abis) > 1:
-        fail(
-            "all python versions need to be specified explicitly, got: {}".format(platforms),
-        )
-    else:
-        default_abi = None
-
     reqs_by_name = {}
 
     for req in reqs:
         if req.name_ in excludes:
             continue
 
+        if include and req.name_ not in include:
+            continue
+
         reqs_by_name.setdefault(req.name, []).append(req)
 
     for name, reqs in reqs_by_name.items():
@@ -102,55 +74,25 @@ def deps(
             normalize_name(name),
             reqs,
             extras = want_extras,
-            platforms = platforms,
-            default_abi = default_abi,
         )
 
     return struct(
         deps = sorted(deps),
         deps_select = {
-            _platform_str(p): sorted(deps)
-            for p, deps in deps_select.items()
+            d: markers
+            for d, markers in sorted(deps_select.items())
         },
     )
 
-def _platform_str(self):
-    if self.abi == None:
-        return "{}_{}".format(self.os, self.arch)
-
-    return "{}_{}_{}".format(
-        self.abi,
-        self.os or "anyos",
-        self.arch or "anyarch",
-    )
-
-def _add(deps, deps_select, dep, platform):
+def _add(deps, deps_select, dep, markers = None):
     dep = normalize_name(dep)
 
-    if platform == None:
+    if not markers:
         deps[dep] = True
-
-        # If the dep is in the platform-specific list, remove it from the select.
-        pop_keys = []
-        for p, _deps in deps_select.items():
-            if dep not in _deps:
-                continue
-
-            _deps.pop(dep)
-            if not _deps:
-                pop_keys.append(p)
-
-        for p in pop_keys:
-            deps_select.pop(p)
-        return
-
-    if dep in deps:
-        # If the dep is already in the main dependency list, no need to add it in the
-        # platform-specific dependency list.
-        return
-
-    # Add the platform-specific branch
-    deps_select.setdefault(platform, {})[dep] = True
+    elif len(markers) == 1:
+        deps_select[dep] = markers[0]
+    else:
+        deps_select[dep] = "({})".format(") or (".join(sorted(markers)))
 
 def _resolve_extras(self_name, reqs, extras):
     """Resolve extras which are due to depending on self[some_other_extra].
@@ -207,37 +149,24 @@ def _resolve_extras(self_name, reqs, extras):
     # Poor mans set
     return sorted({x: None for x in extras})
 
-def _add_reqs(deps, deps_select, dep, reqs, *, extras, platforms, default_abi = None):
+def _add_reqs(deps, deps_select, dep, reqs, *, extras):
     for req in reqs:
         if not req.marker:
-            _add(deps, deps_select, dep, None)
+            _add(deps, deps_select, dep)
             return
 
-    platforms_to_add = {}
-    for plat in platforms:
-        if plat in platforms_to_add:
-            # marker evaluation is more expensive than this check
-            continue
-
-        added = False
-        for extra in extras:
-            if added:
+    markers = {}
+    for req in reqs:
+        for x in extras:
+            m = evaluate(req.marker, env = {"extra": x}, strict = False)
+            if m == False:
+                continue
+            elif m == True:
+                _add(deps, deps_select, dep)
                 break
+            else:
+                markers[m] = None
+                continue
 
-            for req in reqs:
-                if evaluate(req.marker, env = env(target_platform = plat, extra = extra)):
-                    platforms_to_add[plat] = True
-                    added = True
-                    break
-
-    if len(platforms_to_add) == len(platforms):
-        # the dep is in all target platforms, let's just add it to the regular
-        # list
-        _add(deps, deps_select, dep, None)
-        return
-
-    for plat in platforms_to_add:
-        if default_abi:
-            _add(deps, deps_select, dep, plat)
-        if plat.abi == default_abi or not default_abi:
-            _add(deps, deps_select, dep, platform(os = plat.os, arch = plat.arch))
+    if markers:
+        _add(deps, deps_select, dep, sorted(markers))