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(toolchain): support specifying x.y versions in transitions #1720

Merged
merged 23 commits into from
Jan 31, 2024

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jan 24, 2024

This is inspired by how rules_go is registering their toolchains.

Their toolchains have multiple target_settings values. This
allows for a simpler passing of X.Y version to the py_binary and
py_test rules and does not strictly require us to provide the APIs
that pass the full python version value as the closure. This is only
possible because #1555 introduced working aliases and now we can also
have this.

Summary:

  • refactor: move the toolchain_def to starlark as opposed to templating
  • refactor: move the version setting as well
  • feat: support matching on X.Y versions
  • feat: X.Y.Z will match if X.Y is used as python_version flag and the
    MINOR_MAPPING has "X.Y": "X.Y.Z".
  • test: add tests checking the generated config settings.
  • doc: add an example of how we could use the transition files directly

See https://github.com/bazelbuild/rules_go/blob/master/go/private/go_toolchain.bzl#L181

@aignas
Copy link
Collaborator Author

aignas commented Jan 24, 2024

Hah, the build is unhappy, but not in all situations, however, wanted to submit this as it was in my local branch and #1555 added the missing pieces that I needed to get this almost working.

examples/bzlmod/tests/BUILD.bazel Outdated Show resolved Hide resolved
examples/bzlmod/tests/BUILD.bazel Show resolved Hide resolved
python/private/toolchain_defs.bzl Outdated Show resolved Hide resolved
python/private/toolchains_repo.bzl Outdated Show resolved Hide resolved
python/config_settings/config_settings.bzl Outdated Show resolved Hide resolved
python/private/BUILD.bazel Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

LGTM. Just either remove the optional MINOR_MAPPING check, or put a comment about the case it is serving.

python/config_settings/config_settings.bzl Outdated Show resolved Hide resolved
Test that the generated selects are actually correct
@aignas
Copy link
Collaborator Author

aignas commented Jan 31, 2024

I'm gonna merge this, but if you have a comment on how to make the test for the generated code better, let me know. It seemed like visual inspection was the easiest way for now.

@aignas aignas added this pull request to the merge queue Jan 31, 2024
Merged via the queue into bazelbuild:main with commit 8f114a5 Jan 31, 2024
4 checks passed
@rickeylev
Copy link
Collaborator

For testing, analysis_test can be used with selects and a small custom rule to hold values.


subject = rule(
  implementation = lambda ctx: None,
  attrs = {"match_a": attr.bool()}
)

def _test_config_matching(name):
  subject(
    name="subject",
    match_a = select({"is_python_3.4.5": True}, "//conditions:default": False})
  )
  analysis_test(
    ...,
    config_settings = {
      "@rules_python//python/config_settings:python_version": "3.4"
    }
  )

def _test_config_matching_impl(env, target):
  env.expect.that_target(target).attr("match_a", factory=subjects.bool).equals(True)

Oh, actually, it looks like there are some tests that do basically this in tests/config_settings/construct_config_settings_test.

aignas added a commit to aignas/rules_python that referenced this pull request Jan 31, 2024
This is a followup to bazelbuild#1720 to add better tests as commented in the PR
and we ensure that we match correctly when the config setting is
configured to a minor version.
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2024
This is a followup to #1720 to add better tests as commented in the PR
and we ensure that we match correctly when the config setting is
configured to a minor version.
@aignas aignas deleted the feat/x.y-transition branch May 13, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants