ProtoConfiguration: Expose some fields to Starlark

[Yannic]

This change exposes the values of --protoc and --proto_options to
Starlark, thus making it possible to implement proto_toolchain in
Starlark. See bazelbuild/rules_proto#82 for the
implementation of proto_toolchain.

Working towards #10005

[Yannic]

@comius or @lberki PTAL, thanks!

[comius]

Since we have a long term plan to write everything in Starlark, it might make more sense to implement this already in Starlark in the rules_proto repository. See for example rules Scala which alread uses Starlark https://github.com/bazelbuild/rules_scala/blob/master/scala/scala_toolchain.bzl

[Yannic]

I did choose to implement this as a native rule for two reasons:

• It avoids exposing the porto-related command-line flags to Starlark that would tie the proto_toolchain rule to specific versions of Bazel that support these fields and that we do not want anyone to depend on, and
• We most likely need at least a new native Provider to migrate the native protobuf rules to use the toolchain.

I absolutely do plan to rewrite this new rule in Starlark as soon as the command-line flags are removed.

[comius]

I did choose to implement this as a native rule for two reasons:

• It avoids exposing the porto-related command-line flags to Starlark that would tie the proto_toolchain rule to specific versions of Bazel that support these fields and that we do not want anyone to depend on, and

Can you be a bit more specific. Are you talking about ProtoConfiguration flags?

I'll go forward with the review, but I would still like to know about this.

• We most likely need at least a new native Provider to migrate the native protobuf rules to use the toolchain.

I also thought so and I would accept a new native Provider, however if you take a look at scala toolchain, there is a case where they don't need a new provider and can extend it with arbitrary attributes.

@lberki, @katre is Scala using ToolchainInfo provider (https://github.com/bazelbuild/rules_scala/blob/master/scala/scala_toolchain.bzl#L51) in a good way by ad hoc extending it or is this going to create another technical debt?

I absolutely do plan to rewrite this new rule in Starlark as soon as the command-line flags are removed.

[comius]

Nicely done, thanks. But I still think, the whole PR can already be done in Starlark.

[Yannic]

I did choose to implement this as a native rule for two reasons:

• It avoids exposing the porto-related command-line flags to Starlark that would tie the proto_toolchain rule to specific versions of Bazel that support these fields and that we do not want anyone to depend on, and

Can you be a bit more specific. Are you talking about ProtoConfiguration flags?

Yes, I'm talking about ProtoConfiguration options. I did experiment with exposing the fields on the fragment and writing the rule in Starlark in #9000, but it wasn't simpler than implementing it as native rule (and I never came to try out how exactly to use the Starlark toolchain from native rules). I think having this as a native rule for now is the better option.

I'll go forward with the review, but I would still like to know about this.

• We most likely need at least a new native Provider to migrate the native protobuf rules to use the toolchain.

I also thought so and I would accept a new native Provider, however if you take a look at scala toolchain, there is a case where they don't need a new provider and can extend it with arbitrary attributes.

@lberki, @katre is Scala using ToolchainInfo provider (https://github.com/bazelbuild/rules_scala/blob/master/scala/scala_toolchain.bzl#L51) in a good way by ad hoc extending it or is this going to create another technical debt?

AFAIK, ToolchainInfo is the recommended way to create toolchains in Starlark and would work if we only read the values from Starlark. However, there are native rules that would also need to access the fields of the toolchain. It's still possible, but it doesn't feel right. Once everything is migrated to Starlark, we can switch to ToolchainInfo.

I absolutely do plan to rewrite this new rule in Starlark as soon as the command-line flags are removed.

[Yannic]

Rewrote this CL to expose the fragment fields that are necessary for writing proto_toolchain in Starlark.

I also have some tests to verify that this is working as intended, but they depend on bazelbuild/rules_proto#82, so I'd like to wait until thet PR lands and we roll @rules_proto if that's ok

@comius PTAL

[comius]

Everything else seems fine.

[comius]

Please see my comments in bazelbuild/rules_proto#82

[keertk]

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.