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

fix(toolchain): restrict coverage tool visibility under bzlmod #1252

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented Jun 6, 2023

Before this PR the coverage_tool automatically registered by rules_python
was visible outside the toolchain repository. This fixes it to be consistent
with non-bzlmod setups and ensures that the default coverage_tool is not
visible outside the toolchain repos.

This means that the MODULE.bazel file can be cleaned-up at the expense of
relaxing 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 resolve correctly without the
use_repo statement in our MODULE.bazel.

@aignas
Copy link
Collaborator Author

aignas commented Jun 6, 2023

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

platform = platform,
))
# Some wheels are not present for some builds, so let's silently ignore those.
if url != None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if url != None:
if url == None:

fixes the failure for me.

# 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(
Copy link
Contributor

@fmeum fmeum Jun 6, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!

@aignas aignas force-pushed the chore/refactor-coverage-deps branch from be25406 to 7ec6f43 Compare June 8, 2023 04:09
@aignas aignas marked this pull request as ready for review June 8, 2023 04:10
@aignas aignas requested review from f0rmiga and rickeylev as code owners June 8, 2023 04:10
@aignas aignas changed the title refactor(coverage): register coverage dependencies next to the toolchain fix(toolchain): restrict coverage tool visibility under bzlmod Jun 8, 2023
@aignas aignas force-pushed the chore/refactor-coverage-deps branch from 7ec6f43 to 214f146 Compare June 8, 2023 04:20
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

Can you please delete coverage_deps.bzl#install_coverage_deps? It looks unused.

aignas added 2 commits June 9, 2023 08:25
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.
@aignas aignas force-pushed the chore/refactor-coverage-deps branch from 214f146 to 512abc3 Compare June 8, 2023 23:28
@aignas
Copy link
Collaborator Author

aignas commented Jun 22, 2023

@rickeylev gentle ping to get it merged if all looks good.

@chrislovecnm
Copy link
Collaborator

@rickeylev is ooo.

Can you please delete coverage_deps.bzl#install_coverage_deps? It looks unused.

Double checking you did. And I will merge tomorrow for you

@chrislovecnm chrislovecnm dismissed rickeylev’s stale review June 23, 2023 17:19

Change is made and @rickeylev is out of the office

Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

LGTM

@chrislovecnm chrislovecnm added this pull request to the merge queue Jun 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 23, 2023
@chrislovecnm chrislovecnm added this pull request to the merge queue Jun 23, 2023
Merged via the queue into bazelbuild:main with commit 5b8fa22 Jun 23, 2023
@aignas aignas deleted the chore/refactor-coverage-deps branch May 13, 2024 06:48
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.

4 participants