refactor: support rendering pkg aliases without whl_library_alias#1346
refactor: support rendering pkg aliases without whl_library_alias#1346rickeylev merged 5 commits intobazel-contrib:mainfrom
Conversation
52bdd01 to
9ea1fc0
Compare
rickeylev
left a comment
There was a problem hiding this comment.
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
tests/pip_hub_repository/render_pkg_aliases/render_pkg_aliases_test.bzl
Outdated
Show resolved
Hide resolved
|
Also, does this overlap with #1344 ? The files don't, but I see |
@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. |
|
@aignas can you provide more details to @rickeylev about the error message so that we can get that error handling in this PR? |
|
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:
|
|
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
Makes sense, SGTM.
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.
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. |
Move and extend the alias rendering utility function.
f66a171 to
de2b7e1
Compare
|
@rickeylev, could you have a look at this once more? I have rebased on |
| want_content = """\ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| _NO_MATCH_ERROR = \"\"\"\\ |
There was a problem hiding this comment.
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)
Before this PR the only way to render aliases for PyPI package repos
using the version-aware toolchain was to use the
whl_library_aliasrepo.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.
.bzlfile.Split from #1294 and work towards #1262 with ideas taken from #1320.