-
-
Notifications
You must be signed in to change notification settings - Fork 606
feat: freethreaded support for the builder API #3063
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
base: main
Are you sure you want to change the base?
Conversation
This splits the configuration that we have for the `rules_python` and the defaults that we set for our users from the actual unit tests where we check that the extension is working correctly. With this we will be able to dog food the API and point users into the `MODULE.bazel` as the example snippet. Work towards bazel-contrib#2949 Work towards bazel-contrib#2747
Before it seems that we were relying on particular names in the pipstar platforms. This ensures that we rely on this less. Whilst at it fix a few typos and improve the formatting of the code. Work towards bazel-contrib#2949 Work towards bazel-contrib#2747
Before this we would pull all of the wheels that the user target configuration would be compatible with and that meant that it was not customizable. This also meant that there were a lot of footguns in the configuration where the select statements were not really foolproof. With this PR we select only those sources that need to be for the declared configurations. Freethreaded support should be done by defining extre freethreaded platforms using the new builder API. Work towards bazel-contrib#2747 Work towards bazel-contrib#2759 Work towards bazel-contrib#2849
ce4dd59
to
594f026
Compare
73c74d9
to
bc8424b
Compare
Stacked on bazel-contrib#3058 This is a continuation of bazel-contrib#3058 where we define freethreaded platforms. They need to be used only for particular python versions so I included an extra marker configuration attribute where we are using pipstar marker evaluation before using the platform. I think this in general will be a useful tool to configure only particular platforms for particular python versions Work towards bazel-contrib#2548, since this shows how we can define custom platforms Work towards bazel-contrib#2747 TODO: - [ ] Fix the remaining expectations in the unit tests. Maybe make the tests less brittle and define platforms for unit testing.
bc8424b
to
5e66b5b
Compare
platforms[key] = struct( | ||
env = env_, | ||
tripple = "{}_{}_{}".format(abi, values.os_name, values.arch_name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: triple
], | ||
env = {"platform_version": "0"}, | ||
marker = "python_version ~= \"3.13\"" if freethreaded else "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this should be >= 3.13
platform_tags = [ | ||
"macosx_*_{}".format(suffix) | ||
for suffix in platform_tag_cpus | ||
], | ||
want_abis = [ | ||
"cp{0}{1}t", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: {0}{1}
is fine for experimental api, but we should use named params for the final API. Please add a todo, at the least.
requirements = requirements, | ||
requirements = { | ||
k: { | ||
# TODO @aignas 2025-07-06: should we leave this as is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave what as-is?
# TODO @aignas 2025-07-07: this should be solved in a better way | ||
platforms[candidate].tripple.partition("_")[-1]: None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, isn't this problematic if platform names are user-chosen?
@@ -406,21 +427,25 @@ def _configure(config, *, platform, os_name, arch_name, config_settings, env = { | |||
# the lowest priority one needs to be the first one | |||
platform_tags = ["any"] + platform_tags | |||
|
|||
want_abis = want_abis or [ | |||
"cp{0}{1}", | |||
"abi3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is abi3 here, but not in the MODULE config?
Stacked on #3058
This is a continuation of #3058 where we define freethreaded
platforms. They need to be used only for particular python versions
so I included an extra marker configuration attribute where we
are using pipstar marker evaluation before using the platform.
I think this in general will be a useful tool to configure only
particular platforms for particular python versions
Work towards #2548, since this shows how we can define custom platforms
Work towards #2747