fix(toolchain): restrict coverage tool visibility under bzlmod#1252
fix(toolchain): restrict coverage tool visibility under bzlmod#1252chrislovecnm merged 2 commits intobazel-contrib:mainfrom
Conversation
|
@fmeum, do you have an idea of why the "friendly" name of the coverage tool does not work in this case when registering the toolchain? It does work for specifying the visibility, but it does not work for passing the label to the toolchain repository rule. |
python/private/coverage_deps.bzl
Outdated
| platform = platform, | ||
| )) | ||
| # Some wheels are not present for some builds, so let's silently ignore those. | ||
| if url != None: |
There was a problem hiding this comment.
| if url != None: | |
| if url == None: |
fixes the failure for me.
python/repositories.bzl
Outdated
| # extension that creates the repo, however, since we are | ||
| # calling this inside of a macro, I am not sure how to avoid it | ||
| # as the friendly name is not working here. | ||
| coverage_tool = "{prefix}~python~{toolchain_repo}_coverage//:coverage".format( |
There was a problem hiding this comment.
The friendly name only works from inside a repo created by the same extension, but as far as I can tell, this is called in the extension? Could python_register_toolchains have access to module_ctx?
I am not sure where coverage_tool ends up being used, but if ends up in generated Starlark, you could try to replace @@canonical~name with "@@" + Label("@friendly_name").workspace_name.
There was a problem hiding this comment.
Yeah, the repository rule was accepting a label and I wanted to construct a valid label here. Maybe if we refactor the implementation function to accept the module_ctx, things would be different. That explains why the visibility setting works the way it does.
Thanks a lot for the help!
be25406 to
7ec6f43
Compare
7ec6f43 to
214f146
Compare
rickeylev
left a comment
There was a problem hiding this comment.
Can you please delete coverage_deps.bzl#install_coverage_deps? It looks unused.
This unifies how the coverage toolchain is registered under bzlmod and non-bzlmod by leveraging the fact that repositories created by the same bzlmod extension can see each other. This means that the `MODULE.bazel` file can be cleaner and the visibility of the `coverage` tool can be restricted to be only visible by the appropriate toolchain. This also means that we have to relax the 'coverage_tool' attribute for the python_repository to be a simple string as the label would be evaluated within the context of `rules_python` which may not necessarily have access to it via the friendly name used from the toolchain repo.
214f146 to
512abc3
Compare
|
@rickeylev gentle ping to get it merged if all looks good. |
|
@rickeylev is ooo.
Double checking you did. And I will merge tomorrow for you |
Change is made and @rickeylev is out of the office
Before this PR the
coverage_toolautomatically registered byrules_pythonwas visible outside the toolchain repository. This fixes it to be consistent
with
non-bzlmodsetups and ensures that the defaultcoverage_toolis notvisible outside the toolchain repos.
This means that the
MODULE.bazelfile can be cleaned-up at the expense ofrelaxing the
coverage_toolattribute for thepython_repositoryto be asimple string as the label would be evaluated within the context of
rules_pythonwhich may not necessarily resolve correctly without theuse_repostatement in ourMODULE.bazel.