Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Yannic · 2019-07-28T16:25:49Z

This change exposes some of the Protobuf-related command-line flags to Starlark.

There is a risk of adding these fields, so we gave them a scary name and documented how users are supposed to access them. Hopefully, this will be enough to fight Hyrum's Law.

Blocking #10006

Working towards #10005

Design doc:
https://docs.google.com/document/d/1u95vlQ1lWeQNR4bUw5T4cMeHTGJla2_e1dHHx7v4Dvg/edit#

… Starlark

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/proto/ProtoModuleApi.java

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/ProtoConfigurationApi.java

[1] https://github.com/bazelbuild/rules_proto/pull/25/files#diff-ef5b868c0a77c08e46b4cab4105c4470R18

Yannic · 2019-10-13T15:24:00Z

@lberki This is ready for review now.

//cc @hlopko

lberki

Mind also adding some rudimentary tests?

lberki · 2019-10-14T07:14:31Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

+  @SkylarkConfigurationField(
+      // Must match the value of `_protoc_key`
+      // in `@rules_proto//proto/private/rules:proto_toolchain.bzl`.
+      name = "protoc_do_not_use_or_we_will_break_you_without_mercy",


What's the plan for this command line option?

The usual thing we do is to have rules like CcToolchainAliasRule, although those can just as well be implemented in Starlark. That way, you don't need to have a late-bound default. Even better, you can prescribe a particular name for that rule so that people use it as little as possible (although we don't do that for the rest of the alias rules)

All of these options here are meant for use in a new proto_toolchain rule that is eventually going to replace the command-line options.

i.e. Now, proto_toolchain exposes the options and looks like this: https://github.com/bazelbuild/rules_proto/blob/919671ef0f73f5a82037eaabedc39c938c13cf60/proto/BUILD#L8, and eventually, it will look like this, and the command-line options are deprecated and removed:

proto_toolchain( name = "default_toolchain_impl", compiler = "@com_google_protobuf//:protoc", protocopt = ["--foo", "--bar"], strict_deps = "ERROR", visibility = ["//visibility:public"], )

I guess we could also implement proto_toolchain as native rule first, but since we're going to replace it with a Starlark rule eventually, we can also implement it Starlark now.

Oh my question I already sent is answered here :) So you plan to replace command line options completely?

Yes, long-term (>1y), I'd like to get rid of the (native) command-line options.

lberki · 2019-10-14T07:17:22Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

+        return "OFF";
+
+      case WARN:
+        return "WARN";


Do we actually use "WARN"? AFAIU it's only used for the Java sandwich, but that's passed in from Starlark. And if not, how about using a Boolean?

Today, WARN==ERROR (and DEFAULT==OFF [1], but the default value for --strict_proto_deps is error 🤷‍♀). I'd like proto_library to support WARN eventually, and also print some nice buildozer commands when there are errors or warnings, similar to what Java does.

[1] https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java;l=616?q=areDepsStrict

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/ProtoConfigurationApi.java

lberki · 2019-10-14T07:25:29Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/proto/ProtoModuleApi.java

+      doc = "Do not use this field, its only purpose is to help with migration of "
+                + "Protobuf rules to Starlark.",
+      structField = true)
+  default void configurationFieldProtocExists() {}


What's this used for? To check for the presence of other fields? Then why not check directly for their presence instead?

This is used to check whether --proto_compiler is available. Unfortunately, it's the only way to feature-detect late-bound defaults. https://github.com/bazelbuild/rules_proto/blob/919671ef0f73f5a82037eaabedc39c938c13cf60/proto/private/rules/proto_toolchain.bzl#L35

Alternative is to check for Bazel version (like we do e.g. here: https://github.com/bazelbuild/rules_rust/blob/master/rust/private/rustc.bzl#L138). That requires creating an external repository to write the bazel version.

cc @laurentlb your thoughts on this? This is an example of users having to jump through hoops because we don't have a way to test if constant is defined?

lberki · 2019-10-14T07:25:58Z

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/ProtoConfigurationApi.java

+  @SkylarkCallable(
+      // Must match the value of `_protocopt_key`
+      // in `@rules_proto//proto/private/rules:proto_toolchain.bzl`.
+      name = "protocopt_do_not_use_or_we_will_break_you_without_mercy",


@hlopko , how about living without the scary "this is not a supported API" suffix?

I thought you like these suffices :) For my perspective I don't think this can cause too much trouble if we expose it under short name. And removing the option using an incompatible flag won't be too bad either. So feel free to use the short name Yannic, if you don't feel strongly.

I don't feel strongly either way. @lberki originally suggested them, so it's your call whether we use long or short names :). I'd like to keep the access them through proto_toolchain-suggestion, though.

Yannic · 2019-10-14T14:28:48Z

Thanks! I'll add some tests to verify the fields are there and return the correct values.

As I said, the goal is to eventually remove the command-line options, thus the fields, so I'd like to keep the usage as low as possible.

lberki · 2019-10-15T07:00:49Z

Do we want to remove the command-line options? I thought --protocopt is something we want to keep indefinitely because it's useful functionality. --proto_compiler should arguably be done in a platformized way, but it'll be a good while until we platformize all of Google and until then so I don't think it makes sense to prepare for its removal.

Yannic · 2019-10-15T10:30:44Z

When I talked about removing command-line options, I was talking about the native ones in ProtoConfiguration. For those that provide value to users, we'll keep them as Starlark flags (and if we ever get shorthand mapping of flag labels, they'll even have the same name).

The reason I'm submitting this PR is to get the work on rewriting {cc,java}_proto_library started, which obviously need to honor the existing configuration mechanisms, and to allow other proto rules to use these configurations (rather than requiring them to be set on a per-library base, or not allowing configuration at all). I'm unsure about whether inventing something more complicated as this is worth the effort and doing it this feature-detectable way (rather than adding a new native rule) has the advantage that, e.g., go_proto_library, closure_proto_library, gRPC-rules, ..., can adopt it today.

hlopko · 2019-10-15T12:49:20Z

Is this PR about preparing for proto_toolchain? or proto_lang_toolchain? (The former is a desire to be able to use system installed protoc, the later is a desire to specify which plugin to use with which lang_proto_library)

Yannic · 2019-10-15T13:23:00Z

This is about creating proto_toolchain (but actually using the system installed protoc/runtime is a long way to go).

Yannic

Added some tests and comments. ptal.

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

Yannic · 2019-10-15T15:26:37Z

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoConfiguration.java

+        return "OFF";
+
+      case WARN:
+        return "WARN";


Today, WARN==ERROR (and DEFAULT==OFF [1], but the default value for --strict_proto_deps is error 🤷‍♀). I'd like proto_library to support WARN eventually, and also print some nice buildozer commands when there are errors or warnings, similar to what Java does.

[1] https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCommon.java;l=616?q=areDepsStrict

Yannic · 2019-11-28T19:06:05Z

@lberki Friendly ping?

aiuto · 2020-04-23T02:42:58Z

ping

Yannic · 2020-04-23T13:48:23Z

Closing in favor of #10937

Expose proto fragment fields necessary for writing proto_toolchain to…

ef4912b

… Starlark

googlebot added the cla: yes label Jul 28, 2019

jin added the team-Rules-Server Issues for serverside rules included with Bazel label Jul 28, 2019

jin assigned lberki Jul 28, 2019

lberki reviewed Jul 29, 2019

View reviewed changes

Yannic added 3 commits October 13, 2019 16:30

Merge branch 'master' into experimental_proto_fragment_fields

86ff3de

Update fields to match [1] and add docs

be2dc06

[1] https://github.com/bazelbuild/rules_proto/pull/25/files#diff-ef5b868c0a77c08e46b4cab4105c4470R18

Fix typo

0f2ce4e

Yannic marked this pull request as ready for review October 13, 2019 15:23

Yannic requested a review from hlopko as a code owner October 13, 2019 15:23

Yannic mentioned this pull request Oct 13, 2019

Create proto_toolchain and expose values of command-line flags on it bazelbuild/rules_proto#25

Closed

lberki reviewed Oct 14, 2019

View reviewed changes

Yannic added 2 commits October 15, 2019 19:30

Add tests

f4bef50

Remove extra imports/deps

63ba7e5

Yannic commented Oct 15, 2019

View reviewed changes

Yannic added 2 commits October 15, 2019 19:48

Add missing <

551ca2f

Merge branch 'master' into experimental_proto_fragment_fields

bf25aa2

hlopko removed their request for review November 29, 2019 16:11

Yannic mentioned this pull request Mar 10, 2020

Create proto_toolchain rule #10937

Closed

Yannic closed this Apr 23, 2020

Yannic mentioned this pull request Jan 15, 2021

ProtoConfiguration: Expose some fields to Starlark #12484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Yannic commented Jul 28, 2019 •

edited

Loading

Yannic commented Oct 13, 2019

lberki left a comment

lberki Oct 14, 2019

Yannic Oct 14, 2019

hlopko Oct 15, 2019

Yannic Oct 15, 2019

lberki Oct 14, 2019

Yannic Oct 15, 2019

lberki Oct 14, 2019

Yannic Oct 14, 2019

hlopko Oct 15, 2019

hlopko Oct 15, 2019

lberki Oct 14, 2019

hlopko Oct 15, 2019

Yannic Oct 15, 2019

Yannic commented Oct 14, 2019

lberki commented Oct 15, 2019

Yannic commented Oct 15, 2019

hlopko commented Oct 15, 2019

Yannic commented Oct 15, 2019

Yannic left a comment

Yannic Oct 15, 2019

Yannic commented Nov 28, 2019

aiuto commented Apr 23, 2020

Yannic commented Apr 23, 2020

Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Expose proto fragment fields necessary for writing proto_toolchain to Starlark #9000

Conversation

Yannic commented Jul 28, 2019 • edited Loading

Yannic commented Oct 13, 2019

lberki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Oct 14, 2019

lberki commented Oct 15, 2019

Yannic commented Oct 15, 2019

hlopko commented Oct 15, 2019

Yannic commented Oct 15, 2019

Yannic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yannic commented Nov 28, 2019

aiuto commented Apr 23, 2020

Yannic commented Apr 23, 2020

Yannic commented Jul 28, 2019 •

edited

Loading