revert(pypi): use Python for marker eval and METADATA parsing (#2834)

Summary:
- Revert to using Python for marker evaluation during parsing of
  requirements (partial revert of #2692).
- Use Python to parse whl METADATA.
- Bugfix the new simpler algorithm and add a new unit test.

Fixes #2830

(cherry picked from commit 5b9d545)

Author: aignas

CHANGELOG.md

@@ -66,8 +66,6 @@ END_UNRELEASED_TEMPLATE
     * 3.12.9
     * 3.13.2
 * (pypi) Use `xcrun xcodebuild --showsdks` to find XCode root.
-* (pypi) The `bzlmod` extension will now generate smaller lock files for  when
-  using `experimental_index_url`.
 * (toolchains) Remove all but `3.8.20` versions of the Python `3.8` interpreter who has
   reached EOL. If users still need other versions of the `3.8` interpreter, please supply
   the URLs manually {bzl:obj}`python.toolchain` or {bzl:obj}`python_register_toolchains` calls.
@@ -83,13 +81,6 @@ END_UNRELEASED_TEMPLATE
   [PR #2746](https://github.com/bazel-contrib/rules_python/pull/2746).
 * (rules) {attr}`py_binary.srcs` and {attr}`py_test.srcs` is no longer mandatory when
   `main_module` is specified (for `--bootstrap_impl=script`)
-* (pypi) From now on the `Requires-Dist` from the wheel metadata is analysed in
-  the loading phase instead of repository rule phase giving better caching
-  performance when the target platforms are changed (e.g. target python
-  versions). This is preparatory work for stabilizing the cross-platform wheel
-  support. From now on the usage of `experimental_target_platforms` should be
-  avoided and the `requirements_by_platform` values should be instead used to
-  specify the target platforms for the given dependencies.
 
 [20250317]: https://github.com/astral-sh/python-build-standalone/releases/tag/20250317
 

python/private/pypi/evaluate_markers.bzl

@@ -14,10 +14,21 @@
 
 """A simple function that evaluates markers using a python interpreter."""
 
+load(":deps.bzl", "record_files")
 load(":pep508_env.bzl", "env")
 load(":pep508_evaluate.bzl", "evaluate")
 load(":pep508_platform.bzl", "platform_from_str")
 load(":pep508_requirement.bzl", "requirement")
+load(":pypi_repo_utils.bzl", "pypi_repo_utils")
+
+# Used as a default value in a rule to ensure we fetch the dependencies.
+SRCS = [
+    # When the version, or any of the files in `packaging` package changes,
+    # this file will change as well.
+    record_files["pypi__packaging"],
+    Label("//python/private/pypi/requirements_parser:resolve_target_platforms.py"),
+    Label("//python/private/pypi/whl_installer:platform.py"),
+]
 
 def evaluate_markers(requirements, python_version = None):
     """Return the list of supported platforms per requirements line.
@@ -37,3 +48,54 @@ def evaluate_markers(requirements, python_version = None):
                 ret.setdefault(req_string, []).append(platform)
 
     return ret
+
+def evaluate_markers_py(mrctx, *, requirements, python_interpreter, python_interpreter_target, srcs, logger = None):
+    """Return the list of supported platforms per requirements line.
+
+    Args:
+        mrctx: repository_ctx or module_ctx.
+        requirements: list[str] of the requirement file lines to evaluate.
+        python_interpreter: str, path to the python_interpreter to use to
+            evaluate the env markers in the given requirements files. It will
+            be only called if the requirements files have env markers. This
+            should be something that is in your PATH or an absolute path.
+        python_interpreter_target: Label, same as python_interpreter, but in a
+            label format.
+        srcs: list[Label], the value of SRCS passed from the `rctx` or `mctx` to this function.
+        logger: repo_utils.logger or None, a simple struct to log diagnostic
+            messages. Defaults to None.
+
+    Returns:
+        dict of string lists with target platforms
+    """
+    if not requirements:
+        return {}
+
+    in_file = mrctx.path("requirements_with_markers.in.json")
+    out_file = mrctx.path("requirements_with_markers.out.json")
+    mrctx.file(in_file, json.encode(requirements))
+
+    pypi_repo_utils.execute_checked(
+        mrctx,
+        op = "ResolveRequirementEnvMarkers({})".format(in_file),
+        python = pypi_repo_utils.resolve_python_interpreter(
+            mrctx,
+            python_interpreter = python_interpreter,
+            python_interpreter_target = python_interpreter_target,
+        ),
+        arguments = [
+            "-m",
+            "python.private.pypi.requirements_parser.resolve_target_platforms",
+            in_file,
+            out_file,
+        ],
+        srcs = srcs,
+        environment = {
+            "PYTHONPATH": [
+                Label("@pypi__packaging//:BUILD.bazel"),
+                Label("//:BUILD.bazel"),
+            ],
+        },
+        logger = logger,
+    )
+    return json.decode(mrctx.read(out_file))

python/private/pypi/extension.bzl

@@ -24,7 +24,7 @@ load("//python/private:repo_utils.bzl", "repo_utils")
 load("//python/private:semver.bzl", "semver")
 load("//python/private:version_label.bzl", "version_label")
 load(":attrs.bzl", "use_isolated")
-load(":evaluate_markers.bzl", "evaluate_markers")
+load(":evaluate_markers.bzl", "evaluate_markers_py", EVALUATE_MARKERS_SRCS = "SRCS")
 load(":hub_repository.bzl", "hub_repository", "whl_config_settings_to_json")
 load(":parse_requirements.bzl", "parse_requirements")
 load(":parse_whl_name.bzl", "parse_whl_name")
@@ -71,6 +71,7 @@ def _create_whl_repos(
         whl_overrides,
         available_interpreters = INTERPRETER_LABELS,
         minor_mapping = MINOR_MAPPING,
+        evaluate_markers = evaluate_markers_py,
         get_index_urls = None):
     """create all of the whl repositories
 
@@ -85,6 +86,7 @@ def _create_whl_repos(
             used during the `repository_rule` and must be always compatible with the host.
         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.
 
     Returns a {type}`struct` with the following attributes:
         whl_map: {type}`dict[str, list[struct]]` the output is keyed by the
@@ -172,7 +174,28 @@ def _create_whl_repos(
         ),
         extra_pip_args = pip_attr.extra_pip_args,
         get_index_urls = get_index_urls,
-        evaluate_markers = evaluate_markers,
+        # NOTE @aignas 2024-08-02: , we will execute any interpreter that we find either
+        # in the PATH or if specified as a label. We will configure the env
+        # markers when evaluating the requirement lines based on the output
+        # from the `requirements_files_by_platform` which should have something
+        # similar to:
+        # {
+        #    "//:requirements.txt": ["cp311_linux_x86_64", ...]
+        # }
+        #
+        # We know the target python versions that we need to evaluate the
+        # markers for and thus we don't need to use multiple python interpreter
+        # instances to perform this manipulation. This function should be executed
+        # only once by the underlying code to minimize the overhead needed to
+        # spin up a Python interpreter.
+        evaluate_markers = lambda module_ctx, requirements: evaluate_markers(
+            module_ctx,
+            requirements = requirements,
+            python_interpreter = pip_attr.python_interpreter,
+            python_interpreter_target = python_interpreter_target,
+            srcs = pip_attr._evaluate_markers_srcs,
+            logger = logger,
+        ),
         logger = logger,
     )
 
@@ -193,6 +216,7 @@ 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,
@@ -281,6 +305,13 @@ 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
+        ]
 
         # Pure python wheels or sdists may need to have a platform here
         target_platforms = None
@@ -775,6 +806,13 @@ EXPERIMENTAL: this may be removed without notice.
             doc = """\
 A dict of labels to wheel names that is typically generated by the whl_modifications.
 The labels are JSON config files describing the modifications.
+""",
+        ),
+        "_evaluate_markers_srcs": attr.label_list(
+            default = EVALUATE_MARKERS_SRCS,
+            doc = """\
+The list of labels to use as SRCS for the marker evaluation code. This ensures that the
+code will be re-evaluated when any of files in the default changes.
 """,
         ),
     }, **ATTRS)

python/private/pypi/generate_whl_library_build_bazel.bzl

@@ -21,11 +21,14 @@ _RENDER = {
     "copy_files": render.dict,
     "data": render.list,
     "data_exclude": render.list,
+    "dependencies": render.list,
+    "dependencies_by_platform": lambda x: render.dict(x, value_repr = render.list),
     "entry_points": render.dict,
     "extras": render.list,
     "group_deps": render.list,
     "requires_dist": render.list,
     "srcs_exclude": render.list,
+    "tags": render.list,
     "target_platforms": lambda x: render.list(x) if x else "target_platforms",
 }
 
@@ -37,7 +40,7 @@ _TEMPLATE = """\
 
 package(default_visibility = ["//visibility:public"])
 
-whl_library_targets_from_requires(
+{fn}(
 {kwargs}
 )
 """
@@ -59,17 +62,28 @@ def generate_whl_library_build_bazel(
         A complete BUILD file as a string
     """
 
+    fn = "whl_library_targets"
+    if kwargs.get("tags"):
+        # legacy path
+        unsupported_args = [
+            "requires",
+            "metadata_name",
+            "metadata_version",
+        ]
+    else:
+        fn = "{}_from_requires".format(fn)
+        unsupported_args = [
+            "dependencies",
+            "dependencies_by_platform",
+        ]
+
+    for arg in unsupported_args:
+        if kwargs.get(arg):
+            fail("BUG, unsupported arg: '{}'".format(arg))
+
     loads = [
-        """load("@rules_python//python/private/pypi:whl_library_targets.bzl", "whl_library_targets_from_requires")""",
+        """load("@rules_python//python/private/pypi:whl_library_targets.bzl", "{}")""".format(fn),
     ]
-    if not kwargs.setdefault("target_platforms", None):
-        dep_template = kwargs["dep_template"]
-        loads.append(
-            "load(\"{}\", \"{}\")".format(
-                dep_template.format(name = "", target = "config.bzl"),
-                "target_platforms",
-            ),
-        )
 
     additional_content = []
     if annotation:
@@ -87,6 +101,7 @@ def generate_whl_library_build_bazel(
         [
             _TEMPLATE.format(
                 loads = "\n".join(loads),
+                fn = fn,
                 kwargs = render.indent("\n".join([
                     "{} = {},".format(k, _RENDER.get(k, repr)(v))
                     for k, v in sorted(kwargs.items())

python/private/pypi/parse_requirements.bzl

@@ -80,7 +80,7 @@ def parse_requirements(
 
         The second element is extra_pip_args should be passed to `whl_library`.
     """
-    evaluate_markers = evaluate_markers or (lambda _: {})
+    evaluate_markers = evaluate_markers or (lambda _ctx, _requirements: {})
     options = {}
     requirements = {}
     for file, plats in requirements_by_platform.items():
@@ -156,7 +156,7 @@ def parse_requirements(
     # to do, we could use Python to parse the requirement lines and infer the
     # URL of the files to download things from. This should be important for
     # VCS package references.
-    env_marker_target_platforms = evaluate_markers(reqs_with_env_markers)
+    env_marker_target_platforms = evaluate_markers(ctx, reqs_with_env_markers)
     if logger:
         logger.debug(lambda: "Evaluated env markers from:\n{}\n\nTo:\n{}".format(
             reqs_with_env_markers,

python/private/pypi/pip_repository.bzl

@@ -16,12 +16,11 @@
 
 load("@bazel_skylib//lib:sets.bzl", "sets")
 load("//python/private:normalize_name.bzl", "normalize_name")
-load("//python/private:repo_utils.bzl", "REPO_DEBUG_ENV_VAR", "repo_utils")
+load("//python/private:repo_utils.bzl", "REPO_DEBUG_ENV_VAR")
 load("//python/private:text_util.bzl", "render")
-load(":evaluate_markers.bzl", "evaluate_markers")
+load(":evaluate_markers.bzl", "evaluate_markers_py", EVALUATE_MARKERS_SRCS = "SRCS")
 load(":parse_requirements.bzl", "host_platform", "parse_requirements", "select_requirement")
 load(":pip_repository_attrs.bzl", "ATTRS")
-load(":pypi_repo_utils.bzl", "pypi_repo_utils")
 load(":render_pkg_aliases.bzl", "render_pkg_aliases")
 load(":requirements_files_by_platform.bzl", "requirements_files_by_platform")
 
@@ -71,27 +70,7 @@ package(default_visibility = ["//visibility:public"])
 exports_files(["requirements.bzl"])
 """
 
-def _evaluate_markers(rctx, requirements, logger = None):
-    python_interpreter = _get_python_interpreter_attr(rctx)
-    stdout = pypi_repo_utils.execute_checked_stdout(
-        rctx,
-        op = "GetPythonVersionForMarkerEval",
-        python = python_interpreter,
-        arguments = [
-            # Run the interpreter in isolated mode, this options implies -E, -P and -s.
-            # Ensures environment variables are ignored that are set in userspace, such as PYTHONPATH,
-            # which may interfere with this invocation.
-            "-I",
-            "-c",
-            "import sys; print(f'{sys.version_info[0]}.{sys.version_info[1]}.{sys.version_info[2]}', end='')",
-        ],
-        srcs = [],
-        logger = logger,
-    )
-    return evaluate_markers(requirements, python_version = stdout)
-
 def _pip_repository_impl(rctx):
-    logger = repo_utils.logger(rctx)
     requirements_by_platform = parse_requirements(
         rctx,
         requirements_by_platform = requirements_files_by_platform(
@@ -103,7 +82,13 @@ def _pip_repository_impl(rctx):
             extra_pip_args = rctx.attr.extra_pip_args,
         ),
         extra_pip_args = rctx.attr.extra_pip_args,
-        evaluate_markers = lambda requirements: _evaluate_markers(rctx, requirements, logger),
+        evaluate_markers = lambda rctx, requirements: evaluate_markers_py(
+            rctx,
+            requirements = requirements,
+            python_interpreter = rctx.attr.python_interpreter,
+            python_interpreter_target = rctx.attr.python_interpreter_target,
+            srcs = rctx.attr._evaluate_markers_srcs,
+        ),
     )
     selected_requirements = {}
     options = None
@@ -249,6 +234,13 @@ file](https://github.com/bazel-contrib/rules_python/blob/main/examples/pip_repos
         _template = attr.label(
             default = ":requirements.bzl.tmpl.workspace",
         ),
+        _evaluate_markers_srcs = attr.label_list(
+            default = EVALUATE_MARKERS_SRCS,
+            doc = """\
+The list of labels to use as SRCS for the marker evaluation code. This ensures that the
+code will be re-evaluated when any of files in the default changes.
+""",
+        ),
         **ATTRS
     ),
     doc = """Accepts a locked/compiled requirements file and installs the dependencies listed within.