Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
feat(python.toolchain): support file-based default Python version #2588
Changes from 9 commits
6813055
e0b7a3d
f2cc08a
31cb09c
86d17aa
0228b9d
2b1286d
f4631dc
4acb4d7
678a608
b5a337b
1a94ad4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How does this actually work? Will @@ not expand to rules_python in all cases? Is it possible to add a test for this? Ah, because we are not using
Label("@@//:.python-version")
we are going to use the root module's file. This may be good to document as an inline comment.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.
It didn't actually work. 🙈 It does work when I switch to using
Label("@@//:.python-version")
, which resolves to the root module's file. There is a potential issue if there is a.python-version
file in the root module, but noBUILD
orBUILD.bazel
file in the root module. I don't know any way to detect it and ignore it:For testing, there's the mocked tests in
//tests/python
, which doesn't really catch what.python-version
we resolve to because of the mocking, and I've been playing around inexamples/multi_python_versions
, but I can't have the example test both thepython.defaults
tag class and the.python-version
fallback, unless I copy the entire example. Not sure how to proceed.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.
We can get the path to module file of the root module in the same way and then try reading .python-version in the same dir.
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.
Just realized that my suggestion does not help.
The likelihood of people not having a
BUILD.bazel
file at the root module is low.However it is a little annoying that
module_ctx
is making it hard to do this. Maybe we could let the users specify//python:none
as the.python-version
in those cases? However given that we are following semantic versioning, we will only be able to default to that in 2.0.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.
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.
Please document that we default to
.python-version
if we find it.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.
Should we deprecate this in favour of the
defaults
tag_class?cc: @rickeylev what do you think?