-
-
Notifications
You must be signed in to change notification settings - Fork 570
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(python.toolchain): support file-based default Python version #2588
base: main
Are you sure you want to change the base?
Changes from all commits
6813055
e0b7a3d
f2cc08a
31cb09c
86d17aa
0228b9d
2b1286d
f4631dc
4acb4d7
678a608
b5a337b
1a94ad4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,54 @@ def parse_modules(*, module_ctx, _fail = fail): | |
|
||
config = _get_toolchain_config(modules = module_ctx.modules, _fail = _fail) | ||
|
||
default_python_version = None | ||
for mod in module_ctx.modules: | ||
defaults_attr_structs = _create_defaults_attr_structs(mod = mod) | ||
default_python_version_env = None | ||
default_python_version_file = None | ||
|
||
# Only the root module and rules_python are allowed to specify the default | ||
# toolchain for a couple reasons: | ||
# * It prevents submodules from specifying different defaults and only | ||
# one of them winning. | ||
# * rules_python needs to set a soft default in case the root module doesn't, | ||
# e.g. if the root module doesn't use Python itself. | ||
# * The root module is allowed to override the rules_python default. | ||
if mod.is_root or (mod.name == "rules_python" and not default_python_version): | ||
for defaults_attr in defaults_attr_structs: | ||
default_python_version = _one_or_the_same( | ||
default_python_version, | ||
defaults_attr.python_version, | ||
onerror = _fail_multiple_defaults_python_version, | ||
) | ||
default_python_version_env = _one_or_the_same( | ||
default_python_version_env, | ||
defaults_attr.python_version_env, | ||
onerror = _fail_multiple_defaults_python_version_env, | ||
) | ||
default_python_version_file = _one_or_the_same( | ||
default_python_version_file, | ||
defaults_attr.python_version_file, | ||
onerror = _fail_multiple_defaults_python_version_file, | ||
) | ||
if default_python_version_file: | ||
default_python_version = _one_or_the_same( | ||
default_python_version, | ||
module_ctx.read(default_python_version_file, watch = "yes").strip(), | ||
) | ||
if default_python_version_env: | ||
default_python_version = module_ctx.getenv( | ||
default_python_version_env, | ||
default_python_version, | ||
) | ||
if not default_python_version: | ||
fallback_python_version_file = module_ctx.path(Label("@@//:.python-version")) | ||
if fallback_python_version_file.exists: | ||
default_python_version = module_ctx.read( | ||
fallback_python_version_file, | ||
watch = "yes", | ||
).strip() | ||
|
||
seen_versions = {} | ||
for mod in module_ctx.modules: | ||
module_toolchain_versions = [] | ||
|
@@ -104,7 +152,14 @@ def parse_modules(*, module_ctx, _fail = fail): | |
# * rules_python needs to set a soft default in case the root module doesn't, | ||
# e.g. if the root module doesn't use Python itself. | ||
# * The root module is allowed to override the rules_python default. | ||
is_default = toolchain_attr.is_default | ||
if default_python_version: | ||
is_default = default_python_version == toolchain_version | ||
if toolchain_attr.is_default and not is_default: | ||
fail("The 'is_default' attribute doesn't work if you set " + | ||
"the default Python version with the `defaults` tag " + | ||
"or the '.python-version' file.") | ||
else: | ||
is_default = toolchain_attr.is_default | ||
|
||
# Also only the root module should be able to decide ignore_root_user_error. | ||
# Modules being depended upon don't know the final environment, so they aren't | ||
|
@@ -115,7 +170,7 @@ def parse_modules(*, module_ctx, _fail = fail): | |
fail("Toolchains in the root module must have consistent 'ignore_root_user_error' attributes") | ||
|
||
ignore_root_user_error = toolchain_attr.ignore_root_user_error | ||
elif mod.name == "rules_python" and not default_toolchain: | ||
elif mod.name == "rules_python" and not default_toolchain and not default_python_version: | ||
# We don't do the len() check because we want the default that rules_python | ||
# sets to be clearly visible. | ||
is_default = toolchain_attr.is_default | ||
|
@@ -282,6 +337,19 @@ def _python_impl(module_ctx): | |
else: | ||
return None | ||
|
||
def _one_or_the_same(first, second, *, onerror = None): | ||
if not first: | ||
return second | ||
if not second or second == first: | ||
return first | ||
if onerror: | ||
return onerror(first, second) | ||
else: | ||
fail("Unique value needed, got both '{}' and '{}', which are different".format( | ||
first, | ||
second, | ||
)) | ||
|
||
def _fail_duplicate_module_toolchain_version(version, module): | ||
fail(("Duplicate module toolchain version: module '{module}' attempted " + | ||
"to use version '{version}' multiple times in itself").format( | ||
|
@@ -305,6 +373,30 @@ def _warn_duplicate_global_toolchain_version(version, first, second_toolchain_na | |
version = version, | ||
)) | ||
|
||
def _fail_multiple_defaults_python_version(first, second): | ||
fail(("Multiple python_version entries in defaults: " + | ||
"First default was python_version '{first}'. " + | ||
"Second was python_version '{second}'").format( | ||
first = first, | ||
second = second, | ||
)) | ||
|
||
def _fail_multiple_defaults_python_version_file(first, second): | ||
fail(("Multiple python_version_file entries in defaults: " + | ||
"First default was python_version_file '{first}'. " + | ||
"Second was python_version_file '{second}'").format( | ||
first = first, | ||
second = second, | ||
)) | ||
|
||
def _fail_multiple_defaults_python_version_env(first, second): | ||
fail(("Multiple python_version_env entries in defaults: " + | ||
"First default was python_version_env '{first}'. " + | ||
"Second was python_version_env '{second}'").format( | ||
first = first, | ||
second = second, | ||
)) | ||
|
||
def _fail_multiple_default_toolchains(first, second): | ||
fail(("Multiple default toolchains: only one toolchain " + | ||
"can have is_default=True. First default " + | ||
|
@@ -526,6 +618,21 @@ def _get_toolchain_config(*, modules, _fail = fail): | |
register_all_versions = register_all_versions, | ||
) | ||
|
||
def _create_defaults_attr_structs(*, mod): | ||
arg_structs = [] | ||
|
||
for tag in mod.tags.defaults: | ||
arg_structs.append(_create_defaults_attr_struct(tag = tag)) | ||
|
||
return arg_structs | ||
|
||
def _create_defaults_attr_struct(*, tag): | ||
return struct( | ||
python_version = getattr(tag, "python_version", None), | ||
python_version_env = getattr(tag, "python_version_env", None), | ||
python_version_file = getattr(tag, "python_version_file", None), | ||
) | ||
|
||
def _create_toolchain_attr_structs(*, mod, config, seen_versions): | ||
arg_structs = [] | ||
|
||
|
@@ -570,6 +677,49 @@ def _get_bazel_version_specific_kwargs(): | |
|
||
return kwargs | ||
|
||
_defaults = tag_class( | ||
doc = """Tag class to specify the default Python version.""", | ||
attrs = { | ||
"python_version": attr.string( | ||
mandatory = False, | ||
doc = """\ | ||
String saying what the default Python version should be. If the string | ||
matches the {attr}`python_version` attribute of a toolchain, this | ||
toolchain is the default version. If this attribute is set, the | ||
{attr}`is_default` attribute of the toolchain is ignored. | ||
|
||
:::{versionadded} VERSION_NEXT_FEATURE | ||
::: | ||
""", | ||
), | ||
"python_version_env": attr.string( | ||
mandatory = False, | ||
doc = """\ | ||
Environment variable saying what the default Python version should be. | ||
If the string matches the {attr}`python_version` attribute of a | ||
toolchain, this toolchain is the default version. If this attribute is | ||
set, the {attr}`is_default` attribute of the toolchain is ignored. | ||
|
||
:::{versionadded} VERSION_NEXT_FEATURE | ||
::: | ||
""", | ||
), | ||
"python_version_file": attr.label( | ||
mandatory = False, | ||
allow_single_file = True, | ||
doc = """\ | ||
File saying what the default Python version should be. If the contents | ||
of the file match the {attr}`python_version` attribute of a toolchain, | ||
this toolchain is the default version. If this attribute is set, the | ||
{attr}`is_default` attribute of the toolchain is ignored. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document that we default to |
||
|
||
:::{versionadded} VERSION_NEXT_FEATURE | ||
::: | ||
""", | ||
), | ||
}, | ||
) | ||
|
||
_toolchain = tag_class( | ||
doc = """Tag class used to register Python toolchains. | ||
Use this tag class to register one or more Python toolchains. This class | ||
|
@@ -653,7 +803,14 @@ error to run with root access instead. | |
), | ||
"is_default": attr.bool( | ||
mandatory = False, | ||
doc = "Whether the toolchain is the default version", | ||
doc = """\ | ||
Whether the toolchain is the default version. | ||
|
||
:::{versionchanged} VERSION_NEXT_FEATURE | ||
This setting is ignored if the default version is set using the `defaults` | ||
tag class. | ||
::: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we deprecate this in favour of the cc: @rickeylev what do you think? |
||
""", | ||
), | ||
"python_version": attr.string( | ||
mandatory = True, | ||
|
@@ -852,6 +1009,7 @@ python = module_extension( | |
""", | ||
implementation = _python_impl, | ||
tag_classes = { | ||
"defaults": _defaults, | ||
"override": _override, | ||
"single_version_override": _single_version_override, | ||
"single_version_platform_override": _single_version_platform_override, | ||
|
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.
Maybe having the logger warn the user is enough here? I imagine people may have a
.python-version
file that they expect bazel to not read and then suddenlyrules_python
will start failing, which would be surprising behaviour.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.
If we enter this branch, wouldn't there be surprising behavior no matter what? If the user has said
python.toolchain(python_version = "3.11", is_default = True)
and the.python-version
file says3.12
, suddenlyrules_python
will start using3.12
even though the user has explicitly asked for3.11
to be the default. It's not that some obscure logic used to choose 3.11 and now some obscure logic chooses 3.12 and that's worth a warning — the user explicitly asked for something that we're now actively refusing to do. That feels more like afail
than alogger.warn
, doesn't it? In this scenario, if the user wantsrules_python
to ignore the.python-version
file and keep using3.11
, they would have to migrate to thepython.defaults(python_version = "3.11")
syntax and stop usingis_default
. I suppose this may be considered a breaking change, which we might need to document better in the change log and perhaps the failure message or warning. If we don't want a breaking change, we'd have to change the logic so theis_default
overrides the.python-version
file: in that case, I think a warning would be suitable.If we enter this branch because the user is using both
python.defaults(python_version = "3.12")
andpython.toolchain(python_version = "3.11", is_default = True)
, I think that should be afail
rather than alogger.warn
, because the user is explicitly asking for things that aren't consistent. We could try to keep track of whether the.python-version
file or thepython.defaults
tag class is the source of the discrepancy and do different error messages (or warning, perhaps, in the case of the.python-version
file).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.
Thanks, makes sense.