feat: multi-toolchain support #846

f0rmiga · 2022-10-04T03:53:59Z

PR Checklist

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Most mono repos using Bazel have the problem of needing support for multiple Python versions. Currently, there's no simple way to accomplish this.

What is the new behavior?

Add support for registering multiple Python toolchains with different versions as an opt-in feature so that users can gradually migrate portions of their mono repos to this new feature.

Using load statements to override py_binary and py_test, this feature implementation also plays well with the Gazelle extension by using Gazelle's # gazelle:map_kind from_kind to_kind to_kind_load feature. Also, pure Python libraries can take advantage of this by testing against multiple Python versions in the same BUILD file (see examples).

This feature is achieved by using platform transitions behind the scenes.

Does this PR introduce a breaking change?

Yes
No

Other information

This adds support for multiple Python versions on the same Bazel workspace. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

A py_test using 3.10 runs a py_binary using 3.9. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

See comment in code for the reasons. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga · 2022-11-18T23:05:53Z

@rickeylev

re: specifying the python version using the rule name vs attribute: you don't necessarily need to rename, or follow what I did; see below:

rules_python/examples/multi_python_versions/tests/BUILD.bazel

Lines 1 to 4 in 1053ab0

    
           load("@python//3.10:defs.bzl", py_binary_3_10 = "py_binary", py_test_3_10 = "py_test") 
        
           load("@python//3.8:defs.bzl", py_binary_3_8 = "py_binary", py_test_3_8 = "py_test") 
        
           load("@python//3.9:defs.bzl", py_binary_3_9 = "py_binary", py_test_3_9 = "py_test") 
        
           load("@rules_python//python:defs.bzl", "py_binary", "py_test")

I can think of a few cases on a monorepo for doing the renaming. The most obvious would be a monorepo where a library is shared between multiple projects, and those projects end up having different python version requirements. The maintainers of the shared library will want to assert the tests pass on all python versions, so a rename like this would be required. Otherwise, it makes more sense to just change the import statement on a BUILD file and it's like magic to move to another version. E.g. when adopting this new feature:

-load("@rules_python//python:defs.bzl", "py_binary", "py_test")
+load("@python//3.8:defs.bzl", "py_binary", "py_test")

re: transitioning platforms: I pushed 1053ab0 with this. Indeed much simpler. Thanks!

This avoids confusion with the global `rule` https://bazel.build/rules/lib/globals#rule. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

…support Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

rickeylev

Mostly looks good! Ok to merge after addressing comments.

python/config_settings/transition.bzl

python/pip.bzl

python/config_settings/BUILD.bazel

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

…support

python/config_settings/transition.bzl

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

The previous behaviour was that location expansion on native rules would accept binary targets as inputs and resolve to a single location. This was not an option on custom rules. This patch aligns the behaviour of Starlark's ctx.expand_location to the native rules. E.g. a py_binary can be expanded using location/execpath/rootpath instead of previously only locations/execpaths/rootpaths. For more context, see bazelbuild/rules_python#846. To be able to add the multi-toolchain feature to rules_python, I had to trick `ctx.expand_location` using the `tools` attribute while this is not fixed. Closes #16381. PiperOrigin-RevId: 502466606 Change-Id: I3977f2dd479a55308bdda982588697fb04abcedf

#846 introduced this dependency, and solved the broken examples by installing the dependency only for our own code. However, this was an inintentional breaking change for users, who may not have had bazel_skylib installed in their workspace. This effectively reverts #370. At that time we didn't want any dependencies, because managing them under Bazel's WORKSPACE semantics is so difficult for users. Now that bzlmod has reached General Availability in Bazel 6, such dependencies can be managed more easily. This also allows us to introduce bzl_library calls in our BUILD files, making it less brittle to ensure that users can generate docs for their rules/macros which load from rules_python.

The previous behaviour was that location expansion on native rules would accept binary targets as inputs and resolve to a single location. This was not an option on custom rules. This patch aligns the behaviour of Starlark's ctx.expand_location to the native rules. E.g. a py_binary can be expanded using location/execpath/rootpath instead of previously only locations/execpaths/rootpaths. For more context, see bazelbuild/rules_python#846. To be able to add the multi-toolchain feature to rules_python, I had to trick `ctx.expand_location` using the `tools` attribute while this is not fixed. Closes #16381. PiperOrigin-RevId: 502466606 Change-Id: I3977f2dd479a55308bdda982588697fb04abcedf

This was referenced Oct 4, 2022

fix: expand_location binary target inputs bazelbuild/bazel#16381

Closed

Allow ctx.expand_location to skip mixing in hard-coded attributes bazelbuild/bazel#16382

Closed

f0rmiga force-pushed the f0rmiga/multi-python-support branch 5 times, most recently from c6eef27 to 70fa529 Compare October 12, 2022 05:58

f0rmiga added 23 commits October 12, 2022 09:23

feat: multi-toolchain support

5d004e0

This adds support for multiple Python versions on the same Bazel workspace. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

feat: cross-version testing

ecc3ba9

A py_test using 3.10 runs a py_binary using 3.9. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: error message

ae419f2

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

doc: add link to bazelbuild/bazel PR fixing expand_location

4d79a84

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: set environment variables for py_binary too

3441145

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

test: extra case for default version taking another version

44d6cb4

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: fail if args attribute is set

557848a

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: remove confusing output with same target name

5755c01

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: buildifier

cbe5a11

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

revert: use testing.TestEnvironment

30e520d

See comment in code for the reasons. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: linting issues

9eb0f3b

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: black linter

3891e54

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: move tests to a sub-dir

3f65659

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

feat: add multi_pip_parse

0ec1d4f

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: add missing aliases

c412a2e

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: use requirement function in example

32fa462

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: deleted packages

17b0ba1

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: update generated docs

835d25f

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: version checking of the rule is already done by other tests

75ad960

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: add python_interpreter_target to multi_pip_parse

ffe07ac

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: windows

8bc79c1

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: unify py_test and py_binary transition impls

174663a

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: test compatible with all platforms

519f762

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: use usr flags instead of platforms in transition

1053ab0

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga mentioned this pull request Nov 21, 2022

Correctly reference os-specific labels from central alias repo #889

Merged

12 tasks

f0rmiga added 6 commits November 21, 2022 14:40

refactor: rename rule -> rule_impl

d813740

This avoids confusion with the global `rule` https://bazel.build/rules/lib/globals#rule. Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

refactor: reduce repetition of args

161f068

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: missing test and binary-specific attributes

66b33f3

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: add srcs and deps attrs for path expansion

cecb3c8

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Merge remote-tracking branch 'origin/main' into f0rmiga/multi-python-…

90e3314

…support Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: missing bazel_skylib on integration tests

562ee9b

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

rickeylev mentioned this pull request Nov 28, 2022

Use RunEnvironmentInfo instead of testing.TestEnvironment #901

Open

rickeylev requested changes Nov 28, 2022

View reviewed changes

f0rmiga added 3 commits November 28, 2022 13:12

refactor: use ctx.target_platform_has_constraint over select

ca49f8b

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

doc: why symlink <name>.zip under Windows

906bb6a

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: apply suggestions from code review

3316928

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

f0rmiga force-pushed the f0rmiga/multi-python-support branch from c60f069 to 3316928 Compare November 28, 2022 21:25

f0rmiga added 3 commits November 28, 2022 13:32

fix: use incoming edge transitions

9c833f1

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

fix: use RunEnvironmentInfo when available

aa0fc9f

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

Merge remote-tracking branch 'origin/main' into f0rmiga/multi-python-…

070ec92

…support

f0rmiga requested a review from rickeylev November 28, 2022 21:44

rickeylev requested changes Nov 28, 2022

View reviewed changes

python/config_settings/transition.bzl Outdated Show resolved Hide resolved

fix: cfg should be target not exec

6896f15

Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>

rickeylev approved these changes Nov 28, 2022

View reviewed changes

f0rmiga merged commit 17a1573 into main Nov 28, 2022

f0rmiga deleted the f0rmiga/multi-python-support branch November 28, 2022 21:58

alexeagle mentioned this pull request Jan 21, 2023

fix(deps): declare our dependency on bazel_skylib #1001

Merged

dizzy57 mentioned this pull request Nov 14, 2023

feat(config_settings): allow matching minor version of python_version flag #1555

Merged

rickeylev mentioned this pull request Jan 31, 2024

bug: PyRuntimeInfo provided by py_binary doesn't match PyRuntimeInfo symbol from rules_python #1732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multi-toolchain support #846

feat: multi-toolchain support #846

f0rmiga commented Oct 4, 2022 •

edited

Loading

f0rmiga commented Nov 18, 2022

rickeylev left a comment

feat: multi-toolchain support #846

feat: multi-toolchain support #846

Conversation

f0rmiga commented Oct 4, 2022 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

f0rmiga commented Nov 18, 2022

rickeylev left a comment

Choose a reason for hiding this comment

f0rmiga commented Oct 4, 2022 •

edited

Loading