refactor: support rendering pkg aliases without whl_library_alias

[aignas]

Before this PR the only way to render aliases for PyPI package repos
using the version-aware toolchain was to use the whl_library_alias repo.
However, we have code that is creating aliases for packages within the hub repo
and it is natural to merge the two approaches to keep the number of layers of
indirection to minimum.

• feat: support alias rendering for python aware toolchain targets.
• refactor: use render_pkg_aliases everywhere.
• refactor: move the function to a private .bzl file.
• test: add unit tests for rendering of the aliases.

Split from #1294 and work towards #1262 with ideas taken from #1320.

[rickeylev]

feat: support alias rendering for python aware toolchain targets.

What does this mean? I don't see the behavior change that is new functionality?

Otherwise, mostly LGTM

[rickeylev]

Also, does this overlap with #1344 ? The files don't, but I see no_match_error as part of the logic, but don't see no_match_error in the removed code, nor recall that in other existing code.

[aignas]

Also, does this overlap with #1344 ? The files don't, but I see no_match_error as part of the logic, but don't see no_match_error in the removed code, nor recall that in other existing code.

@rickeylev, with regards to overlap with #1344, I wanted to ensure that the new alias rendering function can fail if the appropriate version is not found. Let me know if I should move addition of the error message to a separate PR.

[chrislovecnm]

@aignas can you provide more details to @rickeylev about the error message so that we can get that error handling in this PR?

[aignas]

After having a look at #1344 I see that the PRs overlap with a great extent. To keep this PR scoped I would like to not add more things here but leave it for a followup PR. My plan would be to:

• Move of the pip_hub_repository from python/pip_install to python/extensions/private.bzl and remove the usage of whl_library_alias and use render_pkg_aliases for everything. This would not include anything to do with the entry_point macro usage from the main hub repo yet because I think it is a bit complex and I'd like to first do the refactor of getting rid of some of the complexity in the pip.bzl extension.
• Fix the usage of the pip.parse extension with various toolchain versions, which is the fix: prefer X.Y over X.Y.Z version aliases in python.toolchain #1340 PR (or a different one entirely). I am afraid that refactor: add a version label function for consistent labels #1328 may have regressed the functionality in cases where we the users have the specific X.Y.Z Python toolchain versions specified in their MODULE.bazel. Previously it would work but we would have weird and undocumented pip_XYZ_ prefixes and weird @python_versions//X.Y.Z:defs.bzl imports. If you would like to revert refactor: add a version label function for consistent labels #1328 in the mean time, feel free to do so.

[rickeylev]

FYI 1344 has been merged. If this PR obviates the no_match_error logic I put in that PR, that's mostly fine -- just retain the detailed error message. When select() fails to match, it's a very confusing state to figure out unless you're well acquainted with bazel query, bazel info, configurations, etc

I wanted to ensure that the new alias rendering function can fail if the appropriate version is not found.

Makes sense, SGTM.

My plan would be to

Mostly SGTM, I think? I'd have to see the code to get a better sense of what this means and looks like. I'm convinced that whl_library_alias is extraneous. My gut says that something else is, too, but I'm not sure to what, especially with entry_point looming around.

If you would like to revert #1328 in the mean time, feel free to do so.

I don't think that's necessary. We've only ever gave examples for and documented usage of Major.Minor formats. If anything, we can just fail or warn if a non-Major.Minor value is given until we figure out how to properly support that.

[aignas]

@rickeylev, could you have a look at this once more? I have rebased on main and have made minor changes to reuse the same no_match_error between the whl_library_alias and this new utilitiy. I think this is ready to be merged as is without any extra work that I mentioned in my previous comment.

[rickeylev]

Ah, I guess I better get on bazelbuild/rules_testing#65 so you can do e.g. that_str(actual).matches("_NO_MATCH_ERROR*<everything else>"). The long error message is nice, but embedding it in the test is brittle and distracting.

(I'm not asking to change this, it's fine as is; it's easy to update if we change the error message)