Support --include_imports equivalent in proto_library

[djudd-stripe]

Description of the feature request:

We would find it useful to have an attribute on the native proto_library rule which causes it to behave like protoc when passed --include_imports, which would include all the transitive descriptor set contents in the output descriptor set. Supporting self-contained descriptor set outputs in this way would make it easier to write tooling that uses descriptor sets as the import/export unit for passing schemas between repos, which in our setting are not always built with Bazel, and it would bring the rule closer to feature parity with using protoc directly.

[cgrushko]

Our internal solution for this, so far, is a tiny Skylark rule which concatenates the individual descriptors (protobuf's wire format allows for that) and exposes a single file.

Then we wrap a proto_library rule in that Skylark rule and get a single file.

I can run the code through the open-sourcing process and publish it, but really it's no more than cat ${list of files from proto.transitive_descriptor_sets} > $@

[djudd-stripe]

I'm doing the same thing, but I couldn't figure out how to get the result to be treated like or usable in a native proto_library.

[drigz]

When I invoke bazel build on a proto_library directly, it creates a descriptor set with protoc but doesn't use --include_imports. If this were added (maybe by default?) the output file would presumably be a complete descriptor set.

@iirina, WDYT? Would it just be a matter of adding --include_imports to createDescriptorSetToolchain?

We need such a descriptor set for deploying to Service Management and if I understand right, this would also be useful for @djudd-stripe. Additionally, from protocolbuffers/protobuf#2725, it sounds like @cgrushko was interested in using this with --descriptor_set_in to simplify Bazel's protoc invocations.

[cgrushko]

My objection to --include_imports is that the total output size is quadratic, which is a no-no for large proto graphs. We have plenty of those at Google :)

My solution from https://github.com/bazelbuild/bazel/issues/4201#issuecomment-348673194 is really the way to go if you need to collect all transitive descriptors at a single point.

protoc invocations can still be given all of the transitive descriptor sets separately (e.g., as separate command-line arguments) without proto_library concatenating them at each step.

@djudd-stripe 's use case is special because he's trying to bridge two build systems.

There's the possibility of controlling this using a configuration-wide option in Blaze, but we should be careful not to turn Bazel into a swamp of options.

I'll defer to @iirina and @lberki.

[drigz]

@cgrushko Thanks for the explanation! That's a good reason not to make it the default.

We currently have a Skylark rule that converts transitive_srcs into command-line flags for protoc. Having read further, we probably need a descriptor set with --include_imports --include_source_info, in which case it's easiest to keep the rule we have.

EDIT: I found out that --include_source_info is not a requirement for Cloud Endpoints, and have switched to the cat ${transitive_descriptor_sets} approach @cgrushko suggested.

[RNabel]

@cgrushko, you mentioned that Google's workaround may be open-sourced: is there any progress there / has this already happened?

[cgrushko]

not that I know of - and I'm no longer at Google, so can't help :| It should be relatively straightforward - use ctx.actions.run_shell() [1] with command = 'cat ' + ' '.join(filenames) + ' > ' + output_filename (output_filename should be also used in declare_file [2] to declare that this rule produces this file) or something like that. [1] - https://docs.bazel.build/versions/master/skylark/lib/actions.html#run_shell [2] - https://docs.bazel.build/versions/master/skylark/lib/actions.html#declare_file

…

On Tue, Jun 19, 2018 at 3:39 PM Robin Nabel ***@***.***> wrote: @cgrushko <https://github.com/cgrushko>, you mentioned that Google's workaround may be open-sourced: is there any progress there / has this already happened? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/bazelbuild/bazel/issues/4201#issuecomment-398423104>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB5_YRh1Jldl5Hl3UMpRqjFgHv9_gWTSks5t-Q0dgaJpZM4QxNke> .

[drigz]

@RNabel Here's what we're using:

# Copyright 2018 Google LLC.
# SPDX-License-Identifier: Apache-2.0

def paths(files):
  return [f.path for f in files]

def _impl(ctx):
  descriptors = ctx.attr.proto_library[ProtoInfo].transitive_descriptor_sets
  ctx.actions.run_shell(
    inputs=descriptors,
    outputs=[ctx.outputs.out],
    command='cat %s > %s' % (
        ' '.join(paths(descriptors)), ctx.outputs.out.path))

proto_descriptor = rule(
  implementation=_impl,
  attrs = {
    "proto_library": attr.label(),
    "out": attr.output(mandatory=True),
  }
)

You can then use it like:

load("//tools/build_rules:proto_descriptor.bzl", "proto_descriptor")

proto_descriptor(
    name = "proto_descriptor",
    out = "proto_descriptor.pb",
    proto_library = ":my_proto",
)

[RNabel]

Thanks both for the super-fast response - really useful!

[ngalaiko]

Thanks, that's awesome!

[krlvi]

Thanks! PSA - for anybody using this rule

descriptors = ctx.attr.proto_library.proto.transitive_descriptor_sets

will no longer work with bazel 1.0.0
Spent like an hour trying to figure this one out.

Replacing with

descriptors = ctx.attr.proto_library[ProtoInfo].transitive_descriptor_sets

is compliant it seems

[drigz]

@krlvi, thanks for pointing this out, I've updated the comment.

[manlinl]

In bazel 1.2.1, you will need
descriptors = ctx.attr.proto_library[ProtoInfo].transitive_descriptor_sets.to_list()

[aiuto]

It seems like this is resolved for now, but there could be some docs around it.
Moving this to rules_proto as the documentation should go there eventually.

[Yannic]

Duplicating into #45

TL;DR rules_proto should provide a rule for this (similar to @drigz's rule above).