Add proto_common.direct_source_infos #22

Yannic · 2019-10-10T15:19:13Z

This change adds a helper to extract information about direct sources
of ProtoInfo providers.

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

Note that this was originally named proto_common.protos_to_generate,
but was renamed because it applies more broadly.

lberki · 2019-10-11T08:54:06Z

/me deferring this to @hlopko -- @hlopko , ping me if you need a hand!

hlopko · 2019-10-16T05:46:02Z

Side note, lets submit this after #26 if possible?

Yannic · 2019-10-16T08:21:26Z

sgtm

hlopko

Looking good, only minor comments. Thank you!

hlopko · 2019-10-16T07:12:05Z

proto/private/utils/direct_source_infos.bzl

+        # Before Bazel 1.0, `proto_source_root` wasn't guaranteed to be a prefix
+        # of `file.path`. This could happend, e.g., if `file` was generated.
+
+        # TODO(yannic): Remove this hack when we drop support for Bazel >= 0.29.


Do we support bazel < 1.0? If yes, we should have a CI coverage for it (I vote for only supporting the latest Bazel explicitly).

I'm still migrating to 1.0, but I'm fine with not polluting rules_proto with this. Supporting only the latest Bazel explicitly sgtm.

hlopko · 2019-10-16T07:16:50Z

proto/private/utils/direct_source_infos.bzl

+    },
+)
+
+def direct_source_infos(proto_info, blacklisted_sources = []):


SuperNit: The term blacklist seems to have a negative charge for some people. Wdyt about blocked_sources? ignored_sources? skipped_sources? Something else?

I wasn't aware that this could offend some people, I'll put it on my list of terms to avoid.
These files are usually provided by the runtime, so [runtime_]provided_sources sounds good.

tests/analysis/proto_common_contains_native_fields_test.bzl

hlopko · 2019-10-16T10:52:00Z

tests/utils/compute_import_path_test.bzl

+    },
+)
+
+def compute_import_path_test(name, deps):


Let's see if I got this right. This test iterates over each dependency, gets ProtoInfo from the dependency, generates a new .proto file, writes is in _compute_import_path rule, and then adds a proto_library that depends on that new proto. The test assertion is actually the fact that proto_library builds. Correct?

So it seems like an integration test. I see the value of this test, but also consider adding unit-test-like tests checking that proto_common.direct_source_infos returns expected values for each proto_library in //tests/.

Pls document the flow of this integration test somewhere, it's quite too complex to assume a reader will get it just by reading it.

I replaced this test with a unit test for now. Adding an integration test just for this won't provide much value. We'll add one later when we also have proto_common.generat.

hlopko · 2019-10-16T10:53:43Z

tests/utils/compute_import_path_test.bzl

+
+def compute_import_path_test(name, deps):
+    for dep in deps:
+        target_under_test = "//tests:{}".format(dep)


I'd prefer to assume deps attribute to be list of labels, and instead of constructing the label in the macro we could cut the name part of the label for gen_name. Imho that's more idiomatic this way.

hlopko · 2019-10-16T10:54:18Z

tests/utils/compute_import_path_test.bzl

+        proto_library(
+            name = "{}.{}".format(name, dep),
+            srcs = [
+                ":{}".format(gen_name),


You can just use gen_name, no need to prepend ":", both are valid label syntax.

hlopko · 2019-10-16T10:55:51Z

tests/utils/compute_import_path_test.bzl

+        )
+
+        proto_library(
+            name = "{}.{}".format(name, dep),


Maybe extract variable for this too? For consistency.. But I don't insist, it's a matter of taste I guess.

Yannic

Sorry for the delay. ptal

Yannic · 2019-10-16T13:53:00Z

proto/private/utils/direct_source_infos.bzl

+    },
+)
+
+def direct_source_infos(proto_info, blacklisted_sources = []):


I wasn't aware that this could offend some people, I'll put it on my list of terms to avoid.
These files are usually provided by the runtime, so [runtime_]provided_sources sounds good.

Yannic · 2019-10-22T12:55:54Z

proto/private/utils/direct_source_infos.bzl

+        # Before Bazel 1.0, `proto_source_root` wasn't guaranteed to be a prefix
+        # of `file.path`. This could happend, e.g., if `file` was generated.
+
+        # TODO(yannic): Remove this hack when we drop support for Bazel >= 0.29.


I'm still migrating to 1.0, but I'm fine with not polluting rules_proto with this. Supporting only the latest Bazel explicitly sgtm.

tests/analysis/proto_common_contains_native_fields_test.bzl

Yannic · 2019-10-22T13:58:55Z

tests/utils/compute_import_path_test.bzl

+    },
+)
+
+def compute_import_path_test(name, deps):


I replaced this test with a unit test for now. Adding an integration test just for this won't provide much value. We'll add one later when we also have proto_common.generat.

Yannic · 2020-02-02T12:59:55Z

Updated design to exclude protos based on import path instead of file equality. See bazelbuild/bazel#10590 for context.

Yannic · 2020-03-04T20:12:20Z

@lberki Could we get this prioritized and merged soon? I need this functionality to fix all those proto rules that don't support proto_library#import_prefix yet (e.g. grpc/grpc-web#575, grpc/grpc-web#744, or bazelbuild/rules_closure#390) and I really don't want start copy+pasing proto_common.direct_source_infos everywhere.

.bazelci/presubmit.yml

lberki · 2020-03-09T09:52:12Z

proto/private/utils/direct_source_infos.bzl

+
+"""Contains the implementation for `proto_common.direct_source_infos`."""
+
+ProtoFileInfo = provider(


Why is this a provider? You probably will not want to return this from a rule since this describes only a single file, and in that case, a struct() does the job just as well, but is a bit less confusing.

Providers make generating useful documentation easier. The alternative would be to document the structure of the struct() in the documentation of direct_source_infos's return type, which, to me, sounds worse than making it a provider.

But I un-exported it from //proto:defs.bzl. Users won't need to instantiate it.

Au contraire! Providers convey the meaning that they are used for communication between configured targets, which isn't the case here. Mind turning it into a struct?

lberki · 2020-03-09T09:54:57Z

proto/private/utils/direct_source_infos.bzl

+# TODO(yannic): Remove this suppression when
+# https://github.com/bazelbuild/buildtools/issues/774 has been fixed.
+# buildifier: disable=function-docstring-args
+def direct_source_infos(proto_info, *, provided_sources = []):


Why isn't this a member of ProtoInfo? Sounds like a reasonable functionality there. (I might be missing something obvious here, since I haven't been following this pull request very closely)

I thought about making import paths of direct sources part of ProtoInfo before but never came to a conclusion.

ProtoInfo already is "complicated" (:cough: original sources) and I'd like to avoid adding more half-baked stuff to it. Can you file a feature request?

lberki · 2020-03-09T09:56:06Z

@Yannic , doing my best... I left a code review here, although probably all it does is make my ignorance obvious: it feels like everything I asked has an obvious answer which I should know, had I followed this progress of this change.

Yannic

Thanks, added some comments

.bazelci/presubmit.yml

Yannic · 2020-03-09T12:42:33Z

proto/private/utils/direct_source_infos.bzl

+
+"""Contains the implementation for `proto_common.direct_source_infos`."""
+
+ProtoFileInfo = provider(


Providers make generating useful documentation easier. The alternative would be to document the structure of the struct() in the documentation of direct_source_infos's return type, which, to me, sounds worse than making it a provider.

Yannic · 2020-03-09T12:43:57Z

proto/private/utils/direct_source_infos.bzl

+
+"""Contains the implementation for `proto_common.direct_source_infos`."""
+
+ProtoFileInfo = provider(


But I un-exported it from //proto:defs.bzl. Users won't need to instantiate it.

Yannic · 2020-03-09T12:51:21Z

proto/private/utils/direct_source_infos.bzl

+# TODO(yannic): Remove this suppression when
+# https://github.com/bazelbuild/buildtools/issues/774 has been fixed.
+# buildifier: disable=function-docstring-args
+def direct_source_infos(proto_info, *, provided_sources = []):


I thought about making import paths of direct sources part of ProtoInfo before but never came to a conclusion.

ProtoInfo already is "complicated" (:cough: original sources) and I'd like to avoid adding more half-baked stuff to it. Can you file a feature request?

This change adds a helper to extract information about direct sources of `ProtoInfo` providers. Design doc: https://docs.google.com/document/d/1u95vlQ1lWeQNR4bUw5T4cMeHTGJla2_e1dHHx7v4Dvg/edit# Note that this was originally named `proto_common.protos_to_generate`, but was renamed because it applies more broadly.

lberki · 2020-03-09T13:04:51Z

proto/private/utils/direct_source_infos.bzl

+        )
+
+    source_root = proto_info.proto_source_root
+    offset = len(source_root) + 1  # + '/'.


Whoa, I think I caught an actual bug. This logic seems to want to do same thing as the the exec path -> import path logic from the Java code:

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java#L633

but it doesn't do that, AFAIU: it diverges for .proto files in external repositories and generated .proto files (yes, they do exist)

Do I understand this correctly? If so, it might be more reasonable to expose this information from Java in the interest of not re-implementing the same functionality twice. WDYT?

I'm not sure I understand the case in which it doesn't work.

We do have coverage for external repositories, so I'm confident that works correctly. We don't have tests for generated .proto files here, but aren't they always simlink'ed into <pkg>/_virtual_imports/<name>/<import_path> since bazelbuild/bazel#9215, thus work as well?

The only case I see when this logic could fail is when the file is not under proto_source_root (which, AFAIK, cannot happen since Bazel 1.0). What am I missing?

You're totally right! However, there is also the problem of the direct proto source root being a lie (e.g. if a proto_library has both source and derived .proto files, not all of them are going to be under .proto_source_root.

I suppose bazelbuild/bazel#10939 will help with that?

gattytto · 2020-03-21T03:16:17Z

@lberki this is a missing feature to have nested (proto) dependencies in grpc-web implemented 🙏 I hope you don't mind me pinging you about the pending review

Yannic · 2020-11-14T15:01:46Z

This won't happen as is. Instead, we will expose ProtoSource#getImportPath() to Starlark.

Yannic requested review from dbabkin, hlopko and lberki as code owners October 10, 2019 15:19

lberki removed the request for review from dbabkin October 11, 2019 08:53

hlopko reviewed Oct 16, 2019

View reviewed changes

Yannic commented Oct 22, 2019

View reviewed changes

Yannic force-pushed the protos_to_generate branch 6 times, most recently from e89dc51 to 2026bce Compare December 21, 2019 01:12

Yannic mentioned this pull request Feb 2, 2020

Buildifier: warning function-docstring-args cannot handle keyword-only function arguments bazelbuild/buildtools#774

Closed

Yannic mentioned this pull request Mar 4, 2020

[bazel] Update rules_closure + fix linter warnings grpc/grpc-web#745

Merged

Yannic assigned hlopko and lberki and unassigned hlopko Mar 4, 2020

lberki reviewed Mar 9, 2020

View reviewed changes

Yannic commented Mar 9, 2020

View reviewed changes

Yannic force-pushed the protos_to_generate branch 2 times, most recently from f683d3c to 590fdba Compare March 9, 2020 12:55

lberki reviewed Mar 9, 2020

View reviewed changes

Yannic force-pushed the protos_to_generate branch from 590fdba to 2fdaa50 Compare March 9, 2020 13:04

gattytto mentioned this pull request Apr 9, 2020

gRPC stack eclipse-che/che#15696

Closed

Yannic closed this Nov 14, 2020

Yannic deleted the protos_to_generate branch November 14, 2020 15:01


		"""Contains the implementation for `proto_common.direct_source_infos`."""

		ProtoFileInfo = provider(

Add proto_common.direct_source_infos #22

Add proto_common.direct_source_infos #22

Conversation

Yannic commented Oct 10, 2019

lberki commented Oct 11, 2019

hlopko commented Oct 16, 2019

Yannic commented Oct 16, 2019

hlopko 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

Yannic 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

Yannic commented Feb 2, 2020

Yannic commented Mar 4, 2020

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

lberki commented Mar 9, 2020

Yannic 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

lberki Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

gattytto commented Mar 21, 2020 • edited Loading

Yannic commented Nov 14, 2020

lberki Mar 23, 2020 •

edited

Loading

gattytto commented Mar 21, 2020 •

edited

Loading