-
Notifications
You must be signed in to change notification settings - Fork 22
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
Bump required version to 3.9 and add it to bazel #137
base: master
Are you sure you want to change the base?
Conversation
This bumps the required version to 3.9 which is what our codebase currently requires. It also adds a bazel rule to install a hermetic python 3.9 package. This will make `bazel` always use the hermetic package which should avoid backwards compatibility issues in the future. Fixes google#136
# Use Python 3.9 for bazel Python rules. | ||
python_register_toolchains( | ||
name = "python3", | ||
python_version = "3.9", |
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.
Without this change which version of Python is used? Would it default to current (i.e. 3.11 today)?
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's arbitrary, it'll use whatever the user has on their system. I happen to have 3.11.8
on my dev machine.
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.
Is there a way for users to get that functionality again after this change? Can it be overridden with a flag to bazel, for example? I'd assume that 3.11 would generally have better performance so a user might rather keep using that. If there's a flag or some way to override then a comment would be sufficient.
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.
Hmmm good question, while testing things out I used the --action_env=PYENV_VERSION=3.8
param to bazel so presumably something like that might work. For my current use case we're using a cusom integration with gn
and cmake
so they'll just use whatever we've specified in our build system. For bazel I'm not sure which toolchain would "win.", I'll do some research here, but I suspect the toolchain we specify in our project would be used.
Really my intention was to just use this for tests and not the exported emboss rules, I'll see if I can scope this to just that use case.
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 want to keep the current behavior (use the system python), but still ensure that things aren't broken on the oldest supported Python release, I could add setup-python
to the GitHub actions, add a .python-version
file to the repo. That would be the "source of truth" so to speak of the minimum supported version for the repo and we would just bump that whenever we update the supported version of python.
@reventlov fyi if you have an opinion |
No strong opinion. Scoping to tests only would use fewer system resources while compiling, but leave the open possibility of things breaking if an end user has Python 3.8 (or earlier) as their system Python 3. |
As I just discovered in #167, this is insufficient to fix the issue as currently 3.9 is also failing. I confirmed on all other supported Python versions and it works fine, but it is failing on both 3.8 and 3.9. |
This should "fix" the 3.8 issues and highlight failures on 3.9 so that we prevent future regressions right? Ofc it's already regressed, but once that's fixed it should do the job right? |
Yeah, I didn't word that well, I meant without fixing 3.9 first. |
Im fine landing #163 and breaking this PR. I'll be out a few weeks so if
you want to land a reworked version that's fine with me and we can just
abandon this one.
…On Fri, Aug 9, 2024, 12:59 PM Jason Graffius ***@***.***> wrote:
Would it be possible to rebase this on #163
<#163> and do this or something
similar to this in the MODULE.bazel file instead of WORKSPACE? That way
we can land #163 <#163> and this
without conflict.
—
Reply to this email directly, view it on GitHub
<#137 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADIHJ6CEP2L27Q3X4F5LSLZQUNRDAVCNFSM6AAAAABGOCF5WOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZYGY2TEMJZGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This bumps the required version to 3.9 which is what our codebase currently requires. It also adds a bazel rule to install a hermetic python 3.9 package. This will make
bazel
always use the hermetic package which should avoid backwards compatibility issues in the future.Fixes #136