wip: flag to evaluate pypa dep specs #2832
Conversation
| def _impl(ctx): | ||
| env = {} | ||
|
|
||
| runtime = ctx.toolchains[TARGET_TOOLCHAIN_TYPE].py3_runtime |
There was a problem hiding this comment.
I was wondering if we should get it from the toolchain info or the //python/config_settings:python_version flag. Probably better from toolchain as it will be more representative, but then we need to fetch the toolchain.
There was a problem hiding this comment.
Fetching the toolchain should be NBD. If we're evaluating which pypi dependency to choose, then it's almost certainly because there's some downstream py_binary or py_library asking, which lookup the toolchain already. (aside: having a separate toolchain type of just the interpreter "metadata" might be handy, though).
The part using the toolchain info will fall short with is the runtime_env toolchain -- that has a bunch of fields set to None, so we need fallback code paths.
|
|
||
| env["os_name"] = struct() | ||
| env["platform_machine"] = struct() | ||
| env["platform_python_implementation"] = struct() |
There was a problem hiding this comment.
This could be CPython if cpython is implementation name. We could probably have that as a flag/or attach it to the runtime.
There was a problem hiding this comment.
I dug through peps a bit. From what i can tell, sys.implementation.name was created to replace platform.python_implementation. It turns out the platform uses regex parsing of the sys.version string to determine its values 😲 .
The docs say there are 4 possible values, 2 of which are essentially defunct (IronPython, Jython). The other two are CPython and PyPy.
I just added an if condition to transform cpython to CPython. If we know the sys.implementation.name value that pypy uses, we could special case that, too.
| # likely to be true. | ||
| env["implementation_name"] = runtime.implementation_name or "cpython" | ||
|
|
||
| env["os_name"] = struct() |
There was a problem hiding this comment.
We can have a select statement in that sets a string based on @platform//os: or the user can override it. Have the default based on some logic could help.
There was a problem hiding this comment.
Done.
(aside: this value is kinda weird, see #2826 (comment) )
| env["platform_system"] = struct() | ||
|
|
||
| # todo: add flag to carry this | ||
| env["platform_version"] = struct() |
There was a problem hiding this comment.
A note to myself. Right now pypi/config_settings is generating something similar for osx_version and maybe this flag could be used instead of creating a new separate flag for the osx_version.
Yes, go ahead and push changes. |
| def _impl(ctx): | ||
| env = {} | ||
|
|
||
| runtime = ctx.toolchains[TARGET_TOOLCHAIN_TYPE].py3_runtime |
There was a problem hiding this comment.
Fetching the toolchain should be NBD. If we're evaluating which pypi dependency to choose, then it's almost certainly because there's some downstream py_binary or py_library asking, which lookup the toolchain already. (aside: having a separate toolchain type of just the interpreter "metadata" might be handy, though).
The part using the toolchain info will fall short with is the runtime_env toolchain -- that has a bunch of fields set to None, so we need fallback code paths.
| # likely to be true. | ||
| env["implementation_name"] = runtime.implementation_name or "cpython" | ||
|
|
||
| env["os_name"] = struct() |
There was a problem hiding this comment.
Done.
(aside: this value is kinda weird, see #2826 (comment) )
|
|
||
| env["os_name"] = struct() | ||
| env["platform_machine"] = struct() | ||
| env["platform_python_implementation"] = struct() |
There was a problem hiding this comment.
I dug through peps a bit. From what i can tell, sys.implementation.name was created to replace platform.python_implementation. It turns out the platform uses regex parsing of the sys.version string to determine its values 😲 .
The docs say there are 4 possible values, 2 of which are essentially defunct (IronPython, Jython). The other two are CPython and PyPy.
I just added an if condition to transform cpython to CPython. If we know the sys.implementation.name value that pypy uses, we could special case that, too.
|
As I've poked around the various pep508.bzl files, I see there's a lot of little utilities, tests, edge cases, etc. I'm just plowing ahead and copy/pasting things. Feel free to point out specific places where something should be wired into something else |
| "expression": attt.string(), | ||
| "os_name": attr.string(), | ||
| "sys_platform": attr.string(), | ||
| "platform_machine": attr.string(), |
There was a problem hiding this comment.
nit: Ideally I would just like to use the value of the currently active @platforms//cpu value. We can use the aliases feature to connect different values together.
That would allow us to not need modify rules_python just to add a new supported arch, which would be a nice thing for the Tier2 platforms out there.
There was a problem hiding this comment.
Yeah, that'd be nice. Unfortunately, I don't see a direct way to get that flag from Starlark. The closet I could find was CcToolchain.cpu. I'll ask around just in case I missed something. At the least, we can simply map all the platform:cpu values to what we know of today
|
Ok, basic flag with tests is working. Based on the convo in the other issue, it's pretty clear I didn't understand the jargon and renaming things is in order. |
wip/prototype to help bootstrap the impl of an analysis-time flag that evaluates
the pep508 dep specs
Creating a PR to make collab easier (maintainers can directly edit)
TODO:
Work towards #2826