fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

aignas · 2023-07-24T04:10:45Z

Before this PR the bzlmod and legacy setups would only expose the
multi-version python toolchain aliases as the string that is passed to
the python_register_multi_toolchains function. This meant that if the
user decided to pass the full version (e.g. 3.11.1) then they had to
import the version aware py_* defs via @foo//3.11.1:defs.bzl. This
PR changes it such that the user can still do the old way of importing
but we print a deprecation warning and ask the user to use
3.11:defs.bzl instead which better reflects how the multi-version
toolchain should be used. This also means that the PRs bumping the minor
versions like in #1352 won't need updating Python code elsewhere.

Whilst at it, we introduce a validation that disallows multiple Python
toolchains of the same X.Y version. This is handled in the bzlmod by
printing warnings that the toolchains are ignored, whilst the non-bzlmod
users will get validation errors, which is a potentially breaking
change.

Fixes #1339

chrislovecnm · 2023-07-24T13:52:01Z

Breaking for bzlmod users who previously used
@python_versions//X.Y.Z:defs.bzl in their BUILD.bazel files and/or
depended on the @pip_XYZ//:requirements.bzl.

How is this a breaking change? Can we make it non breaking?

aignas · 2023-07-24T23:09:38Z

@chrislovecnm, because the bzlmod support is still in early stages I am not sure if we should spend more time to actually generate extra aliases in the python_versions repo. I am also not sure what the behaviour should be if there are two toolchains used with the same Python minor version.

rickeylev · 2023-07-27T17:30:46Z

fixes the python_versions to always contain
@python_versions//X.Y:defs.bzl imports.

#1328 also fixed the case where the Python-version specific pip hub repos would be named
<hub_name>_XYZ as opposed to the <hub_name>_XY, which is expected
across the codebase.

Breaking for bzlmod users who previously used
@python_versions//X.Y.Z:defs.bzl in their BUILD.bazel files and/or
depended on the @pip_XYZ//:requirements.bzl.

To clarify, these XYZ names were only occurring if python_version = "X.Y.Z" was specified, right? Such values were unsupported, so behavior change there is fine. We don't need to call fixing that a breaking change.

My main question with allowing/supporting a patch-level specificity is how non-patch-level-specificity behaves when two different patch levels are requested. e.g. given

python.toolchain(python_version="3.8.1")
python.toolchain(python_version="3.8.2")

Keep in mind those may occur across different modules.

What version does load("@python_version//3.11/defs.bzl", ...) use? Similar questions occur for e.g. the pip stuff.

aignas · 2023-07-28T02:39:49Z

@rickeylev, I'll add a test for this in the examples/bzlmod.

aignas · 2023-07-28T02:51:36Z

It seems that if the micro versions are different, then it will fail because of duplicate coverage repositories with:

$ bazel test ...
Starting local Bazel server and connecting to it...
ERROR: Traceback (most recent call last):
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/extensions/python.bzl", line 127, column 57, in _python_impl
                toolchain_info = _python_register_toolchains(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/extensions/python.bzl", line 50, column 31, in _python_register_toolchains
                python_register_toolchains(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/repositories.bzl", line 539, column 41, in python_register_toolchains
                coverage_tool = coverage_dep(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/private/coverage_deps.bzl", line 127, column 10, in coverage_dep
                maybe(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/bazel_tools/tools/build_defs/repo/utils.bzl", line 233, column 18, in maybe
                repo_rule(name = name, **kwargs)
Error in repository_rule: A repo named python_3_9_aarch64-apple-darwin_coverage is already generated by this module extension at /private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/bazel_tools/tools/build_defs/repo/utils.bzl:233:18
ERROR: error evaluating module extension python in @rules_python~override//python/extensions:python.bzl
INFO: Elapsed time: 9.593s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
    currently loading: tests ... (6 packages)
    Fetching https://bcr.bazel.build/bazel_registry.json
ERROR: Couldn't start the build. Unable to run tests

If we do not register the coverage for the second 3.9 toolchain the failure is due to the toolchain alias definition, I think:

$ bazel test ...
ERROR: Traceback (most recent call last):
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/extensions/python.bzl", line 127, column 57, in _python_impl
                toolchain_info = _python_register_toolchains(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/extensions/python.bzl", line 50, column 31, in _python_register_toolchains
                python_register_toolchains(
        File "/private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/repositories.bzl", line 552, column 26, in python_register_toolchains
                python_repository(
Error in repository_rule: A repo named python_3_9_aarch64-apple-darwin is already generated by this module extension at /private/var/tmp/_bazel_ignas.anikevicius/ee423e5d9d08c035dfd0b914eadb5025/external/rules_python~override/python/repositories.bzl:552:26

I am not sure if it is in scope for this PR though.

aignas · 2023-07-29T07:17:34Z

@rickeylev wrote:

python.toolchain(python_version="3.8.1")
python.toolchain(python_version="3.8.2")

So with the latest iteration of the patch, 3.8.1 would win and 3.8.2 would be ignored. This gives an added benefit that the coverage dependency registration does not need to be changed. I think that bzlmod example is testing this well enough now.

The log from the CI is:

bazel build --show_progress_rate_limit=5 --curses=yes --color=yes --terminal_columns=143 --show_timestamps --verbose_failures --jobs=30 --announce_rc --experimental_repository_cache_hardlinks --disk_cache= --sandbox_tmpfs_path=/tmp --google_default_credentials --remote_cache=remotebuildexecution.googleapis.com --remote_instance_name=projects/bazel-untrusted/instances/default_instance --bes_backend=buildeventservice.googleapis.com --bes_timeout=360s --project_id=bazel-untrusted --remote_timeout=60 --remote_max_connections=200 --remote_default_platform_properties=properties:{name:"cache-silo-key" value:"8d289f2a1cea83df66f90efee75da8dc615b199c80f262fa30a652f2d174a951"} --remote_download_toplevel --test_env=HOME --test_env=BAZELISK_USER_AGENT -- ...
--
 <snip>
  | (07:14:57) INFO: Current date is 2023-07-29
  | (07:14:57) DEBUG: <redacted>: WARNING: Ignoring toolchain 'python_3_9' (3.9.10) from module 'other_module': Toolchain 'python_3_9' from module 'example_bzlmod' already registered Python version 3.9.16 and has precedence
  | (07:14:57) DEBUG: <redacted>: WARNING: Ignoring toolchain 'python_3_11' (3.11.1) from module 'other_module': Toolchain 'python_3_11' from module 'rules_python' already registered Python version 3.11.1 and has precedence
  | (07:15:08) INFO: Analyzed 115 targets (183 packages loaded, 12923 targets configured).
<snip>

aignas · 2023-08-05T03:17:47Z

@chrislovecnm, @rickeylev made this a non-breaking change and updated the legacy setup to also use the same approach, where only a single X.Y python toolchain is supported per multi-version toolchain hub.

rickeylev

Awesome, thank you for figuring out something that has better backwards compatibility.

I filed #1371 for tracking support of patch-level specificity.

The general impl LGTM. Just a few small changes and nits.

python/private/full_version.bzl

python/private/toolchains_repo.bzl

python/private/full_version.bzl

rickeylev · 2023-08-10T18:42:09Z

python/extensions/python.bzl

@@ -74,28 +76,30 @@ def _python_impl(module_ctx):
        module_toolchain_versions = []

        for toolchain_attr in mod.tags.toolchain:
-            toolchain_version = toolchain_attr.python_version
-            toolchain_name = "python_" + toolchain_version.replace(".", "_")
+            toolchain_version = full_version(toolchain_attr.python_version)


Please also add a warning if the passed in python_version includes the patch-level to make it more clear we don't support it.

if toolchain_version_short != toolchain_attr.python_version: _warn_patch_level_toolchain_version(...)

And something like

WARNING: Python version changed from X to Y in toolchain T in module M: patch level specificity is not supported

Or something to that effect. Get all the bits of info in there so users can better identify which module and part of it's configuration is problematic.

I think that supporting patch level specificity in the python.toolchain could be actually desirable for only one reason, but it would be a good reason enough to keep it - different Python toolchain versions come from different indygreg toolchain releases and some of them may have different packaging behaviour, like the last release contains the following changelog:

CPython 3.9.16 -> 3.9.17
CPython 3.10.11 -> 3.10.12
CPython 3.11.3 -> 3.11.4
setuptools 67.7.2 -> 68.0.0
pip 23.1.2 -> 23.2.2
musl 1.2.3 -> 1.2.4
OpenSSL 1.1.1s -> 1.1.1u
SQLite 3.41.2 -> 3.42.0
libxzma 5.2.9 -> 5.2.12
Release artifacts for Linux s390x are now published.
_crypt extension module is now distributed as a standalone shared library (as opposed to being statically linked in libpython). This removes a hard dependency on libcrypt.so.1 for compatibility with modern Linux distributions. See indygreg/python-build-standalone#173 for context. Affects Linux non-musl distributions.

So actually supporting pinning to the X.Y.Z could be an important feature to consider, because some users may want to stick with an older version than what is available in the MINOR_MAPPING.

What we should not support though is two toolchains of X.Y.Z1 and X.Y.Z2 at the same time being used where X.Y are the same.

I am inclined to leave the addition of the warning in a separate PR, if you are not against it.

different indygreg releases have different behavior

Ah, good point. I added that to the FR.

We should not support two toolchains with different patch versions

Oh? Why? That position would avoid several complications. Can you post your rationale to the FR issue?

re: warning in a separate PR: SGTM.

rickeylev · 2023-08-10T18:47:02Z

examples/bzlmod/MODULE.bazel

@@ -28,7 +28,8 @@ python.toolchain(
 # work in progress.
 python.toolchain(
    configure_coverage_tool = True,
-    python_version = "3.10",
+    # One can also provide the full Python version.


Because this isn't supported, please remove it from the examples. Having an example and saying "you can also..." implies its supported, which isn't true.

rickeylev · 2023-08-10T18:47:25Z

examples/bzlmod/other_module/MODULE.bazel

@@ -27,7 +27,7 @@ PYTHON_NAME_311 = "python_3_11"
 python = use_extension("@rules_python//python/extensions:python.bzl", "python")
 python.toolchain(
    configure_coverage_tool = True,
-    python_version = "3.9",
+    python_version = "3.9.10",


Same here: please remove the patch level

rickeylev · 2023-08-10T18:47:37Z

examples/multi_python_versions/WORKSPACE

@@ -22,7 +22,7 @@ python_register_multi_toolchains(
        "3.8",
        "3.9",
        "3.10",
-        "3.11",
+        "3.11.1",


Same here: please remove the patch level

… X.Y.Z Before this PR in numerous places we would check the MINOR_MAPPING dict. This PR adds a function that also fails if the X.Y format is not in the MINOR_MAPPING dict making the code more robust. Split from #1340 to unblock #1364.

… X.Y.Z (#1423) Before this PR in numerous places we would check the MINOR_MAPPING dict. This PR adds a function that also fails if the X.Y format is not in the MINOR_MAPPING dict making the code more robust. Split from #1340 to unblock #1364. --------- Co-authored-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

aignas · 2023-10-17T00:53:11Z

Note that this is working without these code changes, see comment in #1371.

aignas requested a review from rickeylev as a code owner July 24, 2023 04:10

aignas changed the title ~~fix(bzlmod)!: support X.Y or X.Y.Z versions in python.toolchain~~ fix(bzlmod): support X.Y or X.Y.Z versions in python.toolchain Jul 28, 2023

aignas marked this pull request as draft July 28, 2023 05:17

aignas mentioned this pull request Jul 29, 2023

refactor: support rendering pkg aliases without whl_library_alias #1346

Merged

aignas force-pushed the fix/1339/drop-minor-version branch from 0e93efb to d8d4b8d Compare July 29, 2023 07:14

aignas marked this pull request as ready for review July 29, 2023 07:15

aignas requested a review from f0rmiga as a code owner July 29, 2023 07:15

aignas mentioned this pull request Aug 1, 2023

feat: Add s390x release #1352

Merged

aignas force-pushed the fix/1339/drop-minor-version branch from d8d4b8d to a68af10 Compare August 5, 2023 03:04

aignas changed the title ~~fix(bzlmod): support X.Y or X.Y.Z versions in python.toolchain~~ fix: prefer X.Y over X.Y.Z versions in python.toolchain Aug 5, 2023

aignas force-pushed the fix/1339/drop-minor-version branch from a68af10 to a916199 Compare August 5, 2023 03:12

aignas mentioned this pull request Aug 7, 2023

fix: Fixing select when we don't have specific platforms #1364

Closed

rickeylev requested changes Aug 10, 2023

View reviewed changes

aignas force-pushed the fix/1339/drop-minor-version branch from a916199 to 35e763c Compare August 11, 2023 02:11

aignas changed the title ~~fix: prefer X.Y over X.Y.Z versions in python.toolchain~~ fix: prefer X.Y over X.Y.Z version aliases in python.toolchain Aug 11, 2023

aignas mentioned this pull request Aug 12, 2023

bzlmod pip extension generates a different repo if the python.toolchain version is specified in X.Y.Z format #1339

Closed

aignas mentioned this pull request Sep 20, 2023

refactor(toolchain): use a helper method to convert an X.Y version to X.Y.Z #1423

Merged

aignas added 3 commits September 29, 2023 22:08

test: work with minor versions

93995a5

feat(bzlmod): use X.Y to search for toolchain duplicates

9f06b4e

feat(legacy): ensure that we are using an X.Y version in pip_multiparse

3ecc4df

aignas marked this pull request as draft September 29, 2023 13:39

aignas force-pushed the fix/1339/drop-minor-version branch from e890a5a to 3ecc4df Compare September 29, 2023 13:42

aignas mentioned this pull request Oct 17, 2023

Support patch-level Python versions #1371

Closed

aignas closed this Oct 17, 2023

aignas deleted the fix/1339/drop-minor-version branch October 17, 2023 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

aignas commented Jul 24, 2023 •

edited

Loading

chrislovecnm commented Jul 24, 2023

aignas commented Jul 24, 2023

rickeylev commented Jul 27, 2023

aignas commented Jul 28, 2023

aignas commented Jul 28, 2023 •

edited

Loading

aignas commented Jul 29, 2023 •

edited

Loading

aignas commented Aug 5, 2023

rickeylev left a comment

rickeylev Aug 10, 2023

aignas Aug 11, 2023

rickeylev Aug 11, 2023

rickeylev Aug 10, 2023

rickeylev Aug 10, 2023

rickeylev Aug 10, 2023

aignas commented Oct 17, 2023

fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340

Conversation

aignas commented Jul 24, 2023 • edited Loading

chrislovecnm commented Jul 24, 2023

aignas commented Jul 24, 2023

rickeylev commented Jul 27, 2023

aignas commented Jul 28, 2023

aignas commented Jul 28, 2023 • edited Loading

aignas commented Jul 29, 2023 • edited Loading

aignas commented Aug 5, 2023

rickeylev left a comment

Choose a reason for hiding this comment

rickeylev Aug 10, 2023

Choose a reason for hiding this comment

aignas Aug 11, 2023

Choose a reason for hiding this comment

rickeylev Aug 11, 2023

Choose a reason for hiding this comment

rickeylev Aug 10, 2023

Choose a reason for hiding this comment

rickeylev Aug 10, 2023

Choose a reason for hiding this comment

rickeylev Aug 10, 2023

Choose a reason for hiding this comment

aignas commented Oct 17, 2023

aignas commented Jul 24, 2023 •

edited

Loading

aignas commented Jul 28, 2023 •

edited

Loading

aignas commented Jul 29, 2023 •

edited

Loading