Update for new bazel proto API

[keith]

This fixes issues if you build a proto_gen rule with the
--incompatible_disable_legacy_proto_provider flag that will be enabled
with an upcoming bazel release. bazelbuild/bazel#7152

[keith]

@acozzette think you could review this one? 🙏

[acozzette]

The Bazel test run is showing this error:

+ bazel test :protobuf_test --copt=-Werror --host_copt=-Werror
Starting local Bazel server and connecting to it...
Loading:
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading:
Loading: 1 packages loaded
Analyzing: target //:protobuf_test (1 packages loaded, 0 targets configured)
Analyzing: target //:protobuf_test (3 packages loaded, 19 targets configured)
Analyzing: target //:protobuf_test (10 packages loaded, 46 targets configured)
Analyzing: target //:protobuf_test (20 packages loaded, 1039 targets configured)
ERROR: /tmpfs/src/github/protobuf/BUILD:481:1: in deps attribute of proto_gen rule //:cc_test_protos_genproto: '//:cc_wkt_protos_genproto' does not have mandatory providers: 'ProtoInfo'. Since this rule was created by the macro 'cc_proto_library', the error might have been caused by the macro implementation in /tmpfs/src/github/protobuf/protobuf.bzl:293:16
ERROR: Analysis of target '//:protobuf_test' failed; build aborted: Analysis of target '//:cc_test_protos_genproto' failed; build aborted
INFO: Elapsed time: 9.508s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (20 packages loaded, 1041 targets configured)
ERROR: Couldn't start the build. Unable to run tests
FAILED: Build did NOT complete successfully (20 packages loaded, 1041 targets configured)

Do you have any ideas on how to fix this?

[keith]

It looks like CI is running with a version of bazel that doesn't have this new provider. I don't see what version it's running, do you know?

[acozzette]

The test script is here: https://github.com/protocolbuffers/protobuf/blob/master/kokoro/linux/bazel/build.sh I'm not sure what the Bazel version is but I bet you're right that it's not very recent. Could you try updating that script to add the line use_bazel.sh 0.26.0 (or whichever version you need)? I'm not 100% sure if that will work but we can give it a try.

[keith]

Added that line, I don't see that script but that should "just work" on CI?

[acozzette]

Right, I was thinking that script should be present in the CI environment. I restarted the tests so we'll see if it works.

[keith]

Ah the problem has nothing to do with version, it's actually that I haven't changed

protobuf/protobuf.bzl

Lines 169 to 175 in 9690f3f

	return struct(
	proto = struct(
	srcs = srcs,
	import_flags = import_flags,
	deps = deps,
	),
	)

I'm not sure yet what the replacement is for this, but I'm looking into it

[keith]

So I think the import_flags and deps fields here, aren't really "official" fields on the legacy proto provider. I'm not 100% sure since I can't find any documentation on the old provider, but the new one doesn't seem to have fields to replace these https://docs.bazel.build/versions/master/skylark/lib/ProtoInfo.html and I think it's supposed to be a 1:1 transition.

I guess this means that this rule should really return 2 providers, and add these variable fields in the second custom provider. But it would be nice to sanity check that approach if you understand what's going on here?

[acozzette]

@keith I am not too familiar with this legacy provider migration, so unfortunately I don't think I have much insight to offer. I would say let's just try your approach and see if it works.

[keith]

@lberki do you have any insight on how I should migrate this use case for incompatible_disable_legacy_proto_provider?

[lberki]

Erm, sorry for the delay. This message got routed to my private e-mail account somehow.

Well, ProtoInfo cannot be instantiated outside of a proto_library currently. This is because it has a bunch of odd fields Bazel needs but we are not sure if they are principled enough to expose to Starlark.

I (embarrassingly) don't know much about how proto_gen works so I don't know if there is a viable replacement. Do you have a link to a use of this rule so that I can take a look how it's supposed to be used? Is the provider here:

protobuf/protobuf.bzl

Line 170 in 9690f3f

proto = struct(

supposed to interoperate with Bazel's internal proto_library somehow? I'd be surprised, because, well, it contains completely different fields...

Does dep.proto in this change refer to the proto provider of Bazel (the one that is being turned into ProtoInfo) or the one created by this rule? If the latter, you can create your own provider, i.e.:

MyProtoInfo = provider(fields=["import_flags", "deps"])
def _impl(ctx):
  ...
  return [MyProtoInfo(import_flags=..., deps=...)]

If you really need to create a ProtoInfo, one (awful) alternative is to delegate to proto_library: in that would, you'd turn proto_gen into a macro that creates a "secret" proto_library rule behind the scenes and invokes the "real" proto_gen rule.

/cc @hlopko

[hlopko]

+1 on proto_gen reason d'etre question. Why is it used and not proto_library?

[Bocete]

+1 on proto_gen reason d'etre question. Why is it used and not proto_library?

I can't comment on Keith's requirements, but I want to define a proto_library-like rule that figures out its dependency set using providers from my own rules and providers. It's not possible to turn that rule into a macro because we rely on providers to pass required information along.

[hlopko]

@Bocete yup that's where I'm heading, gen_proto has many disadvantages that proto_library doesn't have. If there are limitations to proto_library that prevented its use I'd like to know (and fix them in the proto_library).

@acozzette Do you remember what was the problem with proto_library that forced you to go the gen_proto way?

[acozzette]

@hlopko I am not very familiar with this file, so I am not sure of the history of it or why we went with gen_proto. Unfortunately I think the people who did the most work on this code are no longer working on protobuf. It seems quite possible that there's no longer a good reason for using gen_proto instead of proto_library, so we could perhaps try going in that direction and see if it works.

[Bocete]

@hlopko my usecase is not as simple as using a proto_library, but using macros will not the solution for me.

I want to create my own, non-native rule that produces proto_library-compatible provider and, if necessary, outputs. I am unable to do so because the ProtoInfo constructor doesn't work. I expect I'd hit other problems if I could return the right provider, but who knows.

Why am I doing this: I'm developing a non-trivial system definition rule suite. The system would be composed of components. I wish for components in the system to be able to optionally point at various proto_library targets via bzl providers. Then, post-assembly but still during the analysis phase, the rule I'm trying to write would scan the provider graph for all proto_library targets that components are pointing at, and assemble them to one rule that behaves like a native proto_library with no srcs and those targets as deps.

This may be done using macros (e.g. each component :a would have an associated :a_protos that would transitively include one another up the dependency tree), but I'm explicitly trying to avoid this. I want to keep all build targets optional, suffixes flexible, and to have any number of proto_library assemblies that work this way.

Anyhoo, that's an example of why one would want to avoid defining a proto_library from a macro. I suspect that making ProtoInfo() callable would solve my problem. Alternatively but a bit overkill would be a Proto Sandwich (like the c++ one), letting me effectively invoke a native.proto_library from within a rule.

[keith]

@Bocete can you submit a issue on bazel to track this?

[keith]

After looking at this rule I think someone more familiar with how this is used is going to have to make this change, since I think it will be rather large and well tested.

[Bocete]

@Bocete can you submit a issue on bazel to track this?

I suspect that Bazel #6901 should address this ProtoInfo deficiency, so I commented there instead. I'll open a new issue if I'm wrong about this.