Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added support for custom strip_prefix args in toolchains #664

Merged
merged 3 commits into from
Mar 20, 2022
Merged

Added support for custom strip_prefix args in toolchains #664

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0625896
Added support for custom `strip_prefix` args in toolchains
UebelAndre Mar 19, 2022
f36be83
Merge branch 'main' into prefix
Mar 20, 2022
9cc4483
mandatory
UebelAndre Mar 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions python/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def _python_repository_impl(rctx):
rctx.download_and_extract(
url = url,
sha256 = rctx.attr.sha256,
stripPrefix = "python",
stripPrefix = rctx.attr.strip_prefix,
)

# Write distutils.cfg to the Python installation.
Expand Down Expand Up @@ -157,6 +157,7 @@ py_runtime_pair(
"python_version": python_version,
"release_filename": release_filename,
"sha256": rctx.attr.sha256,
"strip_prefix": rctx.attr.strip_prefix,
"url": url,
}

Expand Down Expand Up @@ -192,6 +193,9 @@ python_repository = repository_rule(
doc = "The SHA256 integrity hash for the Python interpreter tarball.",
mandatory = True,
),
"strip_prefix": attr.string(
doc = "A directory prefix to strip from the extracted files.",
UebelAndre marked this conversation as resolved.
Show resolved Hide resolved
),
"url": attr.string(
doc = "The URL of the interpreter to download",
mandatory = True,
Expand Down Expand Up @@ -244,7 +248,7 @@ def python_register_toolchains(
if not sha256:
continue

(release_filename, url) = get_release_url(platform, python_version, base_url, tool_versions)
(release_filename, url, strip_prefix) = get_release_url(platform, python_version, base_url, tool_versions)

python_repository(
name = "{name}_{platform}".format(
Expand All @@ -258,6 +262,7 @@ def python_register_toolchains(
url = url,
distutils = distutils,
distutils_content = distutils_content,
strip_prefix = strip_prefix,
**kwargs
)
native.register_toolchains("@{name}_toolchains//:{platform}_toolchain".format(
Expand Down
12 changes: 10 additions & 2 deletions python/versions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ TOOL_VERSIONS = {
"x86_64-apple-darwin": "8d06bec08db8cdd0f64f4f05ee892cf2fcbc58cfb1dd69da2caab78fac420238",
"x86_64-unknown-linux-gnu": "aec8c4c53373b90be7e2131093caa26063be6d9d826f599c935c0e1042af3355",
},
"strip_prefix": "python",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, are these required here? or are you just being explicit by setting this to the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
"3.8.12": {
"url": "20220227/cpython-{python_version}+20220227-{platform}-{build}.tar.gz",
Expand All @@ -43,6 +44,7 @@ TOOL_VERSIONS = {
"x86_64-pc-windows-msvc": "924f9fd51ff6ccc533ed8e96c5461768da5781eb3dfc11d846f9e300fab44eda",
"x86_64-unknown-linux-gnu": "5be9c6d61e238b90dfd94755051c0d3a2d8023ebffdb4b0fa4e8fedd09a6cab6",
},
"strip_prefix": "python",
},
"3.9.10": {
"url": "20220227/cpython-{python_version}+20220227-{platform}-{build}.tar.gz",
Expand All @@ -52,6 +54,7 @@ TOOL_VERSIONS = {
"x86_64-pc-windows-msvc": "5bc65ce023614bf496a6748e41dca934b70fc5fac6dfacc46aa8dbcad772afc2",
"x86_64-unknown-linux-gnu": "455089cc576bd9a58db45e919d1fc867ecdbb0208067dffc845cc9bbf0701b70",
},
"strip_prefix": "python",
},
"3.10.2": {
"url": "20220227/cpython-{python_version}+20220227-{platform}-{build}.tar.gz",
Expand All @@ -61,6 +64,7 @@ TOOL_VERSIONS = {
"x86_64-pc-windows-msvc": "a293c5838dd9c8438a84372fb95dda9752df63928a8a2ae516438f187f89567d",
"x86_64-unknown-linux-gnu": "9b64eca2a94f7aff9409ad70bdaa7fbbf8148692662e764401883957943620dd",
},
"strip_prefix": "python",
},
}

Expand Down Expand Up @@ -118,21 +122,25 @@ def get_release_url(platform, python_version, base_url = DEFAULT_RELEASE_BASE_UR
tool_versions: A dict listing the interpreter versions, their SHAs and URL

Returns:
filename and url pair
A tuple of (filename, url, and archive strip prefix)
"""

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({}):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: https://docs.aspect.build/bazelbuild/bazel-skylib/1.1.1/docs/types_doc_gen.html#types.is_dict is slightly better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure - but I'm pretty sure rules_python already requires users to have bazel-skylib installed? agree we shouldn't add a new dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

strip_prefix = strip_prefix[platform]

release_filename = url.format(
platform = platform,
python_version = python_version,
build = "static-install_only" if (WINDOWS_NAME in platform) else "install_only",
)
url = "/".join([base_url, release_filename])
return (release_filename, url)
return (release_filename, url, strip_prefix)

def print_toolchains_checksums(name):
native.genrule(
Expand Down