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 repo aliases and add a test. #3181

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

sputt
Copy link
Contributor

@sputt sputt commented Jan 10, 2025

No description provided.

@sputt
Copy link
Contributor Author

sputt commented Jan 10, 2025

@UebelAndre a fix and test for the repo aliasing

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Thanks! Just one question

@@ -269,7 +269,7 @@ def rust_register_toolchains(
sha256s = sha256s,
urls = urls,
versions = versions,
aliases = aliases,
aliases = dict(aliases),
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is this fixing?

Copy link
Contributor Author

@sputt sputt Jan 16, 2025

Choose a reason for hiding this comment

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

When I originally implemented this feature, I added a check that when you aliased a repository that repository existed. I did it by removing entries from this dict as they were consumed.

It worked for the check, but it emptied the dict for when we actually make the repos below. Unfortunately I thought bringing the repo into scope with use_repo in the test was sufficient, but actually we needed to use the repo in the build for it to be fetched.

@sputt
Copy link
Contributor Author

sputt commented Jan 16, 2025

Functional test

Alias a junk repo name:

(deck@steamdeck aliased_toolchains)$ bazel build ...
INFO: Streaming build results to: https://req-compile.buildbuddy.io/invocation/74dd1f1d-4be0-4ef7-8d90-b5df6695ccad
ERROR: /home/deck/.cache/bazel/_bazel_deck/596758dc673d4f9d583c4ae1c5ffbc6e/external/rules_rust~/rust/repositories.bzl:317:13: Traceback (most recent call last):
        File "/home/deck/.cache/bazel/_bazel_deck/596758dc673d4f9d583c4ae1c5ffbc6e/external/rules_rust~/rust/extensions.bzl", line 106, column 37, in _rust_impl
                rust_register_toolchains(
        File "/home/deck/.cache/bazel/_bazel_deck/596758dc673d4f9d583c4ae1c5ffbc6e/external/rules_rust~/rust/repositories.bzl", line 317, column 13, in rust_register_toolchains
                fail("No repositories were created matching the requested names to alias:\n{}".format("\n".join(sorted(aliases))))
Error in fail: No repositories were created matching the requested names to alias:
Junk repo

Add tests in tests/aliased_toolchains to actually use an aliased repository.

Example failed test before the dict(aliases) call:

(1)(deck@steamdeck aliased_toolchains)$ bazel build ...
INFO: Streaming build results to: https://req-compile.buildbuddy.io/invocation/fbeb544b-8a41-4df6-b89c-ed16ff8e8716
ERROR: /home/deck/rules_rust/test/aliased_toolchains/BUILD.bazel:3:13: While resolving toolchains for target //:hello (5c86453): invalid registered toolchain '@rust_toolchains//:rust_linux': no such target '@@rules_rust~~rust~rust_toolchains//:rust_linux': target 'rust_linux' not declared in package '' defined by /home/deck/.cache/bazel/_bazel_deck/596758dc673d4f9d583c4ae1c5ffbc6e/external/rules_rust~~rust~rust_toolchains/BUILD.bazel

@sputt
Copy link
Contributor Author

sputt commented Jan 16, 2025

@UebelAndre questions resolved, should be good to go.

@UebelAndre UebelAndre enabled auto-merge January 20, 2025 17:26
@UebelAndre UebelAndre added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bazelbuild:main with commit aae84e9 Jan 20, 2025
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