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.

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

feat(bzlmod): Adding base_url attr to python.toolchain #1216

Closed
wants to merge 1 commit into from

Conversation

chrislovecnm
Copy link
Collaborator

@chrislovecnm chrislovecnm commented May 9, 2023

The bazel_url attribute allows the user to set a custom location (URL) that hosts the Python binaries.

The base_url attribute allows users to override the URL passed into python_register_toolchains, via kwargs. This URL is then passed to python_repository, which enables the download of a Python installation package from a custom URL.

Closes #1172

The base_url attribute allows a user to override the URL
that is passed into python_register_toolchains, via kwargs.
This URL is then passed to python_repository which enables
the download of a Python installation package from a custom URL.
A user is now able to set a custom location that hosts the
Python binaries.
register_coverage_tool = toolchain_attr.configure_coverage_tool,
ignore_root_user_error = toolchain_attr.ignore_root_user_error,
)
if toolchain_attr.base_url:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rickeylev not sure if there is a cleaner way to do this. If we don't set the base_url when we call python.toolchain, we end up with an empty string passed in. Then python_register_toolchains passes in an empty value to python_repository. We may have a weird use case where an empty URL is allowed; if not, I can test for an empty URL in python_register_toolchains, which is fewer lines of code.

The URL that is used to override rules_python DEFAULT_RELEASE_BASE_URL. This rule set downloads
the different Python binaries from this website. Override this URL if you want to have rules_python
download the hermetic versions of Python from a different website.
""",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use the bazel downloader for this instead?

See https://blog.aspect.dev/configuring-bazels-downloader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great question! I asked the folks on #1172 to get their opinion. I think folks are using base_url, so if we are not going to support it people will have to migrate to this as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@chrislovecnm
Copy link
Collaborator Author

#1172 (comment)

Folks are requesting feature parity

@rickeylev
Copy link
Contributor

Just to recap my post on the issue: An arg on the toolchain() tag class doesn't seem like the way to solve this. A separate tag class, probably only respected by the root module, to customize it would make more sense. Let's continue any design discussion on the issue and leave the PR for code review comments.

@chrislovecnm
Copy link
Collaborator Author

Closing as we don't want to add an argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to specify base_url in python.toolchain() when using bzlmod
3 participants