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

Allow bzlmod users to inhibit toolchain registration #2819

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

konkers
Copy link
Contributor

@konkers konkers commented Aug 28, 2024

Partial fix for #2818

sha256s = toolchain.sha256s,
extra_target_triples = toolchain.extra_target_triples,
urls = toolchain.urls,
versions = toolchain.versions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not modify the implementation of rust_register_toolchains to not register any toolchains when passed versions = []? This way we avoid the extra check and _empty_repository setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There did not seem to be a clean way to get an empty list passed as toolchain_names to toolchain_repository_hub here (https://github.com/bazelbuild/rules_rust/blob/main/rust/repositories.bzl#L252) without nesting most of that function under an if len(versions) > 0:. versions is not considered until inside calls to rust_repository_set and _get_toolchain_repositories. I could add the following at the beginning of rust_register_toolchains to avoid having to declare _empty_repository:

if len(versions) == 0:
    toolchain_repository_hub(
        name = "rust_toolchains",
        toolchain_names = [],
        toolchain_labels = {},
        toolchain_types = {},
        exec_compatible_with = {},
        target_compatible_with = {},
    )

Other refactor suggestions are welcome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think what you have now it cleaner than the toolchain_repository_hub approach.

sha256s = toolchain.sha256s,
extra_target_triples = toolchain.extra_target_triples,
urls = toolchain.urls,
versions = toolchain.versions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think what you have now it cleaner than the toolchain_repository_hub approach.

@krasimirgg krasimirgg added this pull request to the merge queue Sep 17, 2024
Merged via the queue into bazelbuild:main with commit f2244a5 Sep 17, 2024
3 checks passed
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.

2 participants