Toolchain refactoring #26
Conversation
|
|
|
Are there any tests for validating these rules? there seem to not be |
srikrsna-buf
left a comment
srikrsna-buf
left a comment
There was a problem hiding this comment.
Hey @chancila. This is awesome. Thank you for the PR. I definitely like the provider approach. I've added some preliminary comments. I'll add a more detailed review soon.
| toolchain_info = platform_common.ToolchainInfo( | ||
| cli = ctx.executable.cli, | ||
| return platform_common.ToolchainInfo( | ||
| buf = BufToolchainInfo( |
There was a problem hiding this comment.
I think we can also include the old option to avoid breaking changes and mark it deprecated.
There was a problem hiding this comment.
i am not sure how this would be breaking, the previous api was not public
| if ctx.file.config != None: | ||
| files_to_include.append(ctx.file.config) | ||
| return protoc_plugin_test(ctx, proto_infos, ctx.executable._protoc, ctx.toolchains[_TOOLCHAIN].cli, config, files_to_include) | ||
| return protoc_plugin_test(ctx, proto_infos, ctx.executable._protoc, ctx.toolchains[_TOOLCHAIN].buf.protoc_lint_tool, config, files_to_include) |
There was a problem hiding this comment.
buf_breaking_test and buf_dependencies should also be updated in a similar way.
| """ | ||
|
|
||
| _TOOLCHAIN = str(Label("//tools/protoc-gen-buf-lint:toolchain_type")) | ||
| _TOOLCHAIN = "@rules_buf//buf:toolchain_type" |
There was a problem hiding this comment.
Won't using @rules_buf fail when the imported repository is not named that?
There was a problem hiding this comment.
the rule repo name should be consistent, it's part of your API essentially...
| @@ -1,17 +0,0 @@ | |||
| # Copyright 2021-2022 Buf Technologies, Inc. | |||
There was a problem hiding this comment.
The general approach thats followed is to use a tools directory for the toolchains, so we want to keep this.
There was a problem hiding this comment.
generally the public API should be in buf/
toolchains are part of your public api
do you want to keep these for backwards compatibility sake?
it's also a bit strange because there's no public api for instantiating toolchains so these toolchain types are defacto not useable publicly
There was a problem hiding this comment.
I guess it depends for example rules_java has it defined in a similar way: https://github.com/bazelbuild/rules_java/blob/master/toolchains/
|
Hey @chancila! If you can sign the CLA, I'll take up #26 (comment) and resolve the merge conflicts to get this merged. |
|
thanks, signed |
|
Any plan to ship this? It's gonna help a lot with remote execution. |
|
Closing this in favor of #97 |
4 participants
This is kind of a big change. Key takeaways:
BufToolchainInfo), this is how toolchains typically are usedtoolchain.toolchainwhich contains the ToolchainInfo with theBufToolchainInfo)