Added support for custom strip_prefix args in toolchains
#664
Conversation
| """ | ||
|
|
||
| url = tool_versions[python_version]["url"] | ||
|
|
||
| if type(url) == type({}): | ||
| url = url[platform] | ||
|
|
||
| strip_prefix = tool_versions[python_version].get("strip_prefix", None) | ||
| if type(strip_prefix) == type({}): |
There was a problem hiding this comment.
nit: https://docs.aspect.build/bazelbuild/bazel-skylib/1.1.1/docs/types_doc_gen.html#types.is_dict is slightly better
There was a problem hiding this comment.
That would require introducing a dependency on bazel-skylib or vendoring this file which I think would be better done in an explicit cleanup PR. Does that sound reasonable?
There was a problem hiding this comment.
sure - but I'm pretty sure rules_python already requires users to have bazel-skylib installed? agree we shouldn't add a new dependency
There was a problem hiding this comment.
I'm not aware of that being a requirement. If that's the case, it'd be nice if there was a python_rules_dependencies macro that loaded all dependencies (adhering to bazel-contrib).
| @@ -34,6 +34,7 @@ TOOL_VERSIONS = { | |||
| "x86_64-apple-darwin": "8d06bec08db8cdd0f64f4f05ee892cf2fcbc58cfb1dd69da2caab78fac420238", | |||
| "x86_64-unknown-linux-gnu": "aec8c4c53373b90be7e2131093caa26063be6d9d826f599c935c0e1042af3355", | |||
| }, | |||
| "strip_prefix": "python", | |||
There was a problem hiding this comment.
curious, are these required here? or are you just being explicit by setting this to the default value?
There was a problem hiding this comment.
It's kinda both. There's no default so without this the toolchain would not be structured correctly. So to continue to have these particular ones function correctly, I added the strip_prefixes. I kinda like having these here since I noticed the archive structure changed some time ago. Looking at https://github.com/indygreg/python-build-standalone/releases/tag/20200822, the toolchains are in a python/install directory. My hope in adding these here is that it'd draw attention to the fact that this might be needed to get toolchains working.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Not all toolchains are packaged the same. Even older releases of https://github.com/indygreg/python-build-standalone have different archive structures which are incompatible with how toolchains are currently setup.
Issue Number: N/A
What is the new behavior?
This change allows for a
strip_prefixkey to be added topython_register_toolchains.tool_versionsso that users can use reasonably different archive layouts.Does this PR introduce a breaking change?
Other information