Fixups to use of Labels to support bzlmod #872
Conversation
|
For context see PR to bzlmod registry: |
| doc = "The label of a `.bzl` source which defines toolchain commands", | ||
| allow_files = [".bzl"], | ||
| "commands_src": attr.string( | ||
| doc = "The string of a `.bzl` source which defines toolchain commands", |
There was a problem hiding this comment.
Why remove the .bzl restriction?
There was a problem hiding this comment.
because it becomes a string here - bzlmod remaps the names of the repos and so we can't evaluate the Label at this point as it gets used in the context of another repo so unfortunately we need to pass it through as a string here. It's a bit brittle but as its all internal it works but isn't great.
I guess the real solution is to move the .bzl into the generated repo itself so we don't need to resolve a label to access it?
There was a problem hiding this comment.
Oooh, I didn't notice that the type changed. I'm more nervous about making larger changes than needing to restrict the values here. This seems fine.
| @@ -96,7 +89,7 @@ def register_framework_toolchains(register_toolchains = True): | |||
|
|
|||
| for item in TOOLCHAIN_MAPPINGS: | |||
| # Generate a toolchain name without the `.bzl` suffix | |||
| toolchain_name = "rules_foreign_cc_framework_toolchain_" + item.file.name[:-len(".bzl")] | |||
| toolchain_name = "rules_foreign_cc_framework_toolchain_" + item.file.split(":")[1][:-len(".bzl")] | |||
There was a problem hiding this comment.
Always nervous about raw indexing. Can you update the comment to be a bit more descriptive about that?
There was a problem hiding this comment.
It's not great - the generated name you get within bzlmod is even more horific due to the injection of the .ext. from the extension name into the repo name. Maybe we should just put the name of the repo explicitly into TOOLCHAIN_MAPPING instead of trying to do string manipulation?
There was a problem hiding this comment.
I think it's fine just want to make sure the expectation is super clear as indexing is easy to miss.
I think it's probably time to discuss writing a process wrapper instead of making more changes to the toolchain. If you could assign item.file.split(":")[1][:-len(".bzl")] to another value with some comment I'd be happy.
There was a problem hiding this comment.
I've added the name of the repo into TOOLCHAIN_MAPPING so it is now explicit as to what this name is rather than deriving something from a string.
I also slightly simplified the generated repo but at the expense of some boiler plate in each of the *_commands.bzl files to export a struct.
I think I'd follow this up with a commit that would remove the whole generated repo logic here in favour of just explicitly enumerating the toolchain instances in a BUILD file in the main repo. Its overly complex at the moment for no real reason.
| doc = "The label of a `.bzl` source which defines toolchain commands", | ||
| allow_files = [".bzl"], | ||
| "commands_src": attr.string( | ||
| doc = "The string of a `.bzl` source which defines toolchain commands", |
There was a problem hiding this comment.
Oooh, I didn't notice that the type changed. I'm more nervous about making larger changes than needing to restrict the values here. This seems fine.
| @@ -96,7 +89,7 @@ def register_framework_toolchains(register_toolchains = True): | |||
|
|
|||
| for item in TOOLCHAIN_MAPPINGS: | |||
| # Generate a toolchain name without the `.bzl` suffix | |||
| toolchain_name = "rules_foreign_cc_framework_toolchain_" + item.file.name[:-len(".bzl")] | |||
| toolchain_name = "rules_foreign_cc_framework_toolchain_" + item.file.split(":")[1][:-len(".bzl")] | |||
There was a problem hiding this comment.
I think it's fine just want to make sure the expectation is super clear as indexing is easy to miss.
I think it's probably time to discuss writing a process wrapper instead of making more changes to the toolchain. If you could assign item.file.split(":")[1][:-len(".bzl")] to another value with some comment I'd be happy.
jsharpe
force-pushed
the
bzlmod_labels
branch
from
05507e0 to
3ea01b2
Compare
jsharpe
force-pushed
the
bzlmod_labels
branch
from
3ea01b2 to
cf4e671
Compare
|
@UebelAndre PTAL when you get a moment, hopefully the changes I've made end up in simpler code in the end despite being a bit more churn. |
No description provided.