Skip to content

feat(toolchain): support specifying x.y versions in transitions#1720

Merged
aignas merged 23 commits intobazel-contrib:mainfrom
aignas:feat/x.y-transition
Jan 31, 2024
Merged

feat(toolchain): support specifying x.y versions in transitions#1720
aignas merged 23 commits intobazel-contrib:mainfrom
aignas:feat/x.y-transition

Conversation

@aignas
Copy link
Collaborator

@aignas aignas commented Jan 24, 2024 •
edited
Loading

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.

@aignas aignas mentioned this pull request Jan 24, 2024
Merged
rickeylev
rickeylev reviewed Jan 25, 2024
View reviewed changes
@aignas aignas force-pushed the feat/x.y-transition branch from f9b86c8 to f963deb Compare January 26, 2024 01:04
@aignas aignas requested a review from rickeylev January 26, 2024 01:07
rickeylev
rickeylev reviewed Jan 29, 2024
View reviewed changes
rickeylev
rickeylev reviewed Jan 29, 2024
View reviewed changes
rickeylev
rickeylev approved these changes Jan 31, 2024
View reviewed changes
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.

aignas added 2 commits January 31, 2024 12:04
@aignas
comment: move the bzl_library
b739ed2
@aignas
Fix the select statements
3ca03ec 
Test that the generated selects are actually correct
@aignas aignas force-pushed the feat/x.y-transition branch from bc89626 to 3ca03ec Compare January 31, 2024 04:53
@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 bazel-contrib:main with commit 8f114a5 Jan 31, 2024
@rickeylev
Copy link
Collaborator

rickeylev commented Jan 31, 2024

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
@aignas
test: config_settings matching latest minor versions
72d1bca 
This is a followup to bazel-contrib#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 mentioned this pull request Jan 31, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 31, 2024
@aignas
test: config_settings matching latest minor versions (#1738)
e7f8f0f 
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

Reviewers

@rickeylev rickeylev rickeylev approved these changes

@f0rmiga f0rmiga Awaiting requested review from f0rmiga

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aignas @rickeylev