-
Notifications
You must be signed in to change notification settings - Fork 3
Replace python toolchain with @rules_python #140
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
bf1da1d to
9fb4c71
Compare
nettle
left a 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.
Thanks for working on Bazel 8, @furtib!
This change is crucial.
At the same time the incompatibility between 8 and 7 and 6 is quite big.
We must be very careful to do not destroy what is working somehow in 6 and 7.
Please do not submit without consensus between all of us.
e49ac73 to
962d110
Compare
605e170 to
8459e07
Compare
| # this script will be run with the python interpreter | ||
| "{Mode}": "Run", | ||
| "{Verbosity}": "DEBUG", | ||
| "{PythonPath}": python_path, |
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.
Are you sure this is needed? Can you explain a little? Is there any precedence for this in other projects?
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.
This was the point of this patch.
Shebangs are always absolute paths!
Bazel, providing its own interpreter/compiler etc, will always give relative paths.
I will look for an example from the wild, but getting rid of the shebang was the main point of this PR.
nettle
left a 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.
Well, seams like we will have to use rules_python at some point anyway, so we should be ready. And this patch is very good prototype!
However, I'm sure we should split it and move forward very carefully:
- Refactor py_runtime to python toolchain at least for Bazel 6 and 7
- Make implementation work with all Bazel versions
(theoretically this can be combined, at least partially, with the first step) - Add Bazel 8 support
Before that we must clarify requirements towards rules_python vs bazel_tools in different Bazel versions. As I can understand Bazel 8 still support bazel_tools, but I'm not sure.
| load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") | ||
|
|
||
| register_default_python_toolchain() | ||
| http_archive( | ||
| name = "rules_python", | ||
| sha256 = "ca2671529884e3ecb5b79d6a5608c7373a82078c3553b1fa53206e6b9dddab34", | ||
| strip_prefix = "rules_python-0.38.0", | ||
| url = "https://github.com/bazelbuild/rules_python/releases/download/0.38.0/rules_python-0.38.0.tar.gz", | ||
| ) | ||
|
|
||
| load("@rules_python//python:repositories.bzl", "py_repositories") | ||
|
|
||
| py_repositories() |
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.
What I really dont like is that we add external dependency like this.
In Bazel mod it looks OK, but for Bazel 6 it sucks.
| executable = py_toolchain.interpreter, | ||
| arguments = [ctx.outputs.codechecker_script.path], |
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, maybe...
| default = "@default_python_tools//:py3_runtime", | ||
| ), | ||
| }, | ||
| toolchains = ["@rules_python//python:toolchain_type"], |
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.
I was also thinking about toolchain, but for some reasons used runtime.
It is definitely good idea to try toolchain again!
| output = ctx.outputs.codechecker_test_script, | ||
| is_executable = True, | ||
| substitutions = { | ||
| #"#{Python_path}": py_toolchain.stub_shebang, |
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.
This might be interesting!
I did not know about stub_shebang!
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.
I'm not entirely happy with it.
stub_shebang seems to be a static string #!/usr/bin/env python3. (env is not guaranteed to be there)
As I see it, the interpreter is a more lucrative option. Bazel places the interpreter into the sandbox, and we don't have to worry about it at all.
| ctx.actions.write( | ||
| output = ctx.outputs.test_script_wrapper, | ||
| is_executable = True, | ||
| content = """ | ||
| {} {} | ||
| """.format(py_toolchain.interpreter.short_path, ctx.outputs.codechecker_test_script.short_path) | ||
| ) |
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.
Oh, this does not look elegant to me!
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.
I agree.
I did this to use the bazel provided python instead of the system-provided (through stub_shebang).
f8f3f59 to
5dae841
Compare
|
I tried to minimize the changes in this patch for easier review. |
5dae841 to
beb76ea
Compare
Why:
We should use an external Python toolchain instead of maintaining our own.
This is necessary mainly for Bazel 8 support.
What:
Addresses:
#108