Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proto_common.direct_source_infos #22

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,30 @@ buildifier:
platforms:
macos:
build_targets:
- "..."
- "//..."
test_targets:
- "//..."
Yannic marked this conversation as resolved.
Show resolved Hide resolved

rbe_ubuntu1604:
build_targets:
- "..."
- "//..."
test_targets:
- "//..."

ubuntu1604:
build_targets:
- "..."
- "//..."
test_targets:
- "//..."

ubuntu1804:
build_targets:
- "..."
- "//..."
test_targets:
- "//..."

windows:
build_targets:
- "..."
- "//..."
test_targets:
- "//..."
4 changes: 4 additions & 0 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ rules_proto_dependencies()

rules_proto_toolchains()

load("@bazel_skylib//lib:unittest.bzl", "register_unittest_toolchains")

register_unittest_toolchains()

http_archive(
name = "bazel_toolchains",
sha256 = "b663c411acc9cf191679823aa1eb9d665358239e8bf9e6f7cbb302b41f57317c",
Expand Down
7 changes: 5 additions & 2 deletions proto/defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

"""Starlark rules for building protocol buffers."""

load("//proto/private:native.bzl", "NativeProtoInfo", "native_proto_common")
load("//proto/private:native.bzl", "NativeProtoInfo")
load("//proto/private/utils:direct_source_infos.bzl", "direct_source_infos")

_MIGRATION_TAG = "__PROTO_RULES_MIGRATION_DO_NOT_USE_WILL_BREAK__"

Expand Down Expand Up @@ -57,4 +58,6 @@ ProtoInfo = NativeProtoInfo
# Utilities for protocol buffers.
#
# https://docs.bazel.build/versions/master/skylark/lib/proto_common.html
proto_common = native_proto_common
proto_common = struct(
direct_source_infos = direct_source_infos,
)
1 change: 1 addition & 0 deletions proto/private/utils/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# Intentionally left empty (for now).
68 changes: 68 additions & 0 deletions proto/private/utils/direct_source_infos.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copyright 2019 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

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

# 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 = []):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"""Returns information about `proto_info`'s direct sources.

Files that are both in `proto_info`'s direct sources and in
`provided_sources` are skipped. This is useful, e.g., for well-known
protos that are already provided by the Protobuf runtime.

Args:
proto_info: An instance of `ProtoInfo`.
provided_sources: Optional. A sequence of import paths to ignore.

Returns:
A sequence of structs containing information about `proto_info`'s direct
sources. Each struct contains the following fields:
- `file`: The `.proto` file.
- `import_path`: The import path of the `.proto` file.
"""

files = [_info(f, proto_info) for f in proto_info.direct_sources]
return [f for f in files if f.import_path not in provided_sources]

def _info(file, proto_info):
"""Computes information about a single `.proto` file.

Args:
file: The `.proto` file to generate info for. Must be in
`direct_sources` of `proto_info`.
proto_info: An instance of `ProtoInfo`.

Returns: A struct. Consult the documented return type of
`direct_source_infos` for documentation about its fields.
"""

if "." == proto_info.proto_source_root:
# The `proto_library` didn't specify `import_prefix` or
# `strip_import_prefix`, and `file` is a regular source file
# (i.e. not generated) in the main workspace.

return struct(
file = file,
import_path = file.path,
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@lberki lberki Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return struct(
file = file,
import_path = file.path[offset:],
)
107 changes: 107 additions & 0 deletions tests/BUILD
100644 → 100755
Original file line number Diff line number Diff line change
@@ -1,8 +1,115 @@
load("//proto:defs.bzl", "proto_library")

WORKSPACE_PREFIX = "" # copybara-workspace-prefix(WORKSPACE_PREFIX)

proto_library(
name = "multiple_srcs_proto",
srcs = [
"empty.proto",
"prefix/empty.proto",
],
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_proto",
srcs = [
"empty.proto",
],
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_prefix_proto",
srcs = [
"prefix/empty.proto",
],
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_absolute_strip_prefix_proto",
srcs = [
"empty.proto",
],
strip_import_prefix = "/" + WORKSPACE_PREFIX + "tests",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_relative_strip_prefix_proto",
srcs = [
"prefix/empty.proto",
],
strip_import_prefix = "prefix",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_empty_strip_prefix_proto",
srcs = [
"empty.proto",
],
strip_import_prefix = "",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_add_and_absolute_strip_prefix_proto",
srcs = [
"empty.proto",
],
import_prefix = "foo",
strip_import_prefix = "/" + WORKSPACE_PREFIX + "tests",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_add_and_relative_strip_prefix_proto",
srcs = [
"prefix/empty.proto",
],
import_prefix = "foo",
strip_import_prefix = "prefix",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_add_and_empty_strip_prefix_proto",
srcs = [
"empty.proto",
],
import_prefix = "foo",
strip_import_prefix = "",
visibility = [
"//tests:__subpackages__",
],
)

proto_library(
name = "empty_with_add_and_empty_strip_prefix_proto_in_subdir",
srcs = [
"prefix/empty.proto",
],
import_prefix = "foo",
strip_import_prefix = "",
visibility = [
"//tests:__subpackages__",
],
)
5 changes: 5 additions & 0 deletions tests/analysis/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
load("//tests/analysis:proto_common_contains_native_fields_test.bzl", "proto_common_contains_native_fields_test")

proto_common_contains_native_fields_test(
name = "proto_common_contains_native_fields_test",
)
45 changes: 45 additions & 0 deletions tests/analysis/proto_common_contains_native_fields_test.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# Copyright 2019 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Unit-test to verify that the Starlark `proto_common` shim contains all fields
of the native version of `proto_common`, excluding fields that are considered
to be an implementation detail (thus not part of the public API).
"""

load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
load("//proto:defs.bzl", "proto_common")
load("//proto/private:native.bzl", "native_proto_common") # buildifier: disable=bzl-visibility

def _impl(ctx):
Yannic marked this conversation as resolved.
Show resolved Hide resolved
"""Verifies that the Starlark `proto_common` contains all necessary fields.

Args:
ctx: The rule context.

Returns: A (not further specified) sequence of providers.
"""

env = unittest.begin(ctx)

for field in dir(native_proto_common):
# Starlark `proto_common` only exports fields that are part of the
# stable and supported API of native `proto_common`.
if not field.endswith("_do_not_use_or_we_will_break_you_without_mercy"):
asserts.true(env, hasattr(proto_common, field))
else:
asserts.false(env, hasattr(proto_common, field))

return unittest.end(env)

proto_common_contains_native_fields_test = unittest.make(_impl)
Empty file modified tests/empty.proto
100644 → 100755
Empty file.
17 changes: 17 additions & 0 deletions tests/prefix/empty.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright 2019 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package rules_proto.tests.prefix;
Loading