-
Notifications
You must be signed in to change notification settings - Fork 556
RFC: rust_toolchains submodule #3857
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| toolchain_type( | ||
| alias( | ||
| name = "toolchain_type", | ||
| actual = "@rust_toolchains//rust_analyzer:toolchain_type", | ||
| deprecation = "instead use `@rust_toolchains//rust_analyzer:toolchain_type`", | ||
| tags = ["manual"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| toolchain_type( | ||
| alias( | ||
| name = "toolchain_type", | ||
| actual = "@rust_toolchains//rustfmt:toolchain_type", | ||
| deprecation = "instead use `@rust_toolchains//rustfmt:toolchain_type`", | ||
| tags = ["manual"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| package(default_visibility = ["//visibility:public"]) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module( | ||
| name = "rust_toolchains", | ||
| version = "0.0.1", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| # The toolchain for the exec platform. | ||
| toolchain_type( | ||
| name = "toolchain_type", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| # The toolchain for the exec platform. | ||
| toolchain_type( | ||
| name = "toolchain_type", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| # The toolchain for the exec platform. | ||
| toolchain_type( | ||
| name = "toolchain_type", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| # The toolchain for the exec platform. | ||
| toolchain_type( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we comment explicitly that these are all meant to resolve to the exec platform? I dunno why it would be useful to have target-platform for these tools but I can imagine the feature request
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, we can :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, I can imagine a scenario where your target platform is meant for execution. Imagine you are building dev VM images or similar and want to package a compiler.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and you could also have clippy run as a test, and use target platform for tests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get this landed first to unblock progress and then we can do target-platform toolchains as a followup |
||
| name = "toolchain_type", | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| package(default_visibility = ["//visibility:public"]) | ||
|
|
||
| # The toolchain for the exec platform. | ||
| toolchain_type( | ||
| name = "toolchain_type", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: there's an existing convention on BCR to prefix instead, so toolchains_rust ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a super strong opinion on this bikeshed, i just want it built. That said, if you take a look at https://registry.bazel.build/search?q=toolchains you'll see that:
xxx_toolchaininsteadproto_toolchainsalso followed it, probably because you/Fabian picked the name :)So it doesn't feel like we necessarily need to draw a trend from a few isolated points, we should just pick whatever name is clearest IMO