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

Make rules_swift work with virtual import directories. #298

Merged
merged 6 commits into from
Aug 30, 2019
Merged
Show file tree
Hide file tree
Changes from 5 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
18 changes: 11 additions & 7 deletions swift/internal/proto_gen_utils.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
"""Utility functions shared by rules/aspects that generate sources from .proto files."""

load("@bazel_skylib//lib:paths.bzl", "paths")
load(":utils.bzl", "workspace_relative_path")
load(":utils.bzl", "proto_import_path")

def declare_generated_files(name, actions, extension_fragment, proto_srcs):
def declare_generated_files(name, actions, extension_fragment, proto_source_root, proto_srcs):
"""Declares generated `.swift` files that correspond to a list of `.proto` files.

Args:
Expand All @@ -26,18 +26,19 @@ def declare_generated_files(name, actions, extension_fragment, proto_srcs):
extension_fragment: An extension fragment that precedes `.swift` on the end of the
generated files. In other words, the file `foo.proto` will generate a file named
`foo.{extension_fragment}.swift`.
proto_source_root: the source root of the `.proto` files in `proto_srcs`.
proto_srcs: A list of `.proto` files.

Returns:
A list of files that map one-to-one to `proto_srcs` but with `.{extension_fragment}.swift`
extensions instead of `.proto`.
"""
return [
actions.declare_file(_generated_file_path(name, extension_fragment, f))
actions.declare_file(_generated_file_path(name, extension_fragment, proto_source_root, f))
for f in proto_srcs
]

def extract_generated_dir_path(name, extension_fragment, generated_files):
def extract_generated_dir_path(name, extension_fragment, proto_source_root, generated_files):
"""Extracts the full path to the directory where `.swift` files are generated.

This dance is required because we cannot get the full (repository-relative) path to the
Expand All @@ -52,6 +53,8 @@ def extract_generated_dir_path(name, extension_fragment, generated_files):
extension_fragment: An extension fragment that precedes `.swift` on the end of the
generated files. In other words, the file `foo.proto` will generate a file named
`foo.{extension_fragment}.swift`.
proto_source_root: the source root for the `.proto` files `generated_files` are generated
from.
generated_files: A list of generated `.swift` files, one of which will be used to extract
the directory path.

Expand All @@ -62,7 +65,7 @@ def extract_generated_dir_path(name, extension_fragment, generated_files):
return None

first_path = generated_files[0].path
dir_name = _generated_file_path(name, extension_fragment)
dir_name = _generated_file_path(name, extension_fragment, proto_source_root)
offset = first_path.find(dir_name)
return first_path[:offset + len(dir_name)]

Expand All @@ -88,7 +91,7 @@ def register_module_mapping_write_action(name, actions, module_mappings):

return mapping_file

def _generated_file_path(name, extension_fragment, proto_file = None):
def _generated_file_path(name, extension_fragment, proto_source_root, proto_file = None):
"""Returns the short path of a generated `.swift` file corresponding to a `.proto` file.

The returned workspace-relative path should be used to declare output files so that they are
Copy link
Member

Choose a reason for hiding this comment

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

Is this part still true, or is the output path now slightly different since it might not have some the the original workspace relative info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this is still true -- the format is PACKAGE/NAME.protoc_gen_EXTENSION_swift/IMPORT_PATH.<something>, to which all of this is true. Except that the path now contains the import path and not the workspace-relative path of the source file.

Expand All @@ -102,6 +105,7 @@ def _generated_file_path(name, extension_fragment, proto_file = None):
extension_fragment: An extension fragment that precedes `.swift` on the end of the
generated files. In other words, the file `foo.proto` will generate a file named
`foo.{extension_fragment}.swift`.
proto_source_root: The source root for the `.proto` file.
proto_file: The `.proto` file whose generated `.swift` path should be computed.

Returns:
Expand All @@ -115,7 +119,7 @@ def _generated_file_path(name, extension_fragment, proto_file = None):
)
if proto_file:
generated_file_path = paths.replace_extension(
workspace_relative_path(proto_file),
proto_import_path(proto_file, proto_source_root),
".{}.swift".format(extension_fragment),
)
return paths.join(dir_path, generated_file_path)
Expand Down
28 changes: 24 additions & 4 deletions swift/internal/swift_grpc_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,19 @@ load(
"register_module_mapping_write_action",
)
load(":providers.bzl", "SwiftInfo", "SwiftProtoInfo", "SwiftToolchainInfo")
load(":utils.bzl", "compact", "create_cc_info", "get_providers", "workspace_relative_path")
load(
":utils.bzl",
"compact",
"create_cc_info",
"get_providers",
"proto_import_path",
)

def _register_grpcswift_generate_action(
label,
actions,
direct_srcs,
proto_source_root,
transitive_descriptor_sets,
module_mapping_file,
mkdir_and_run,
Expand All @@ -46,6 +53,7 @@ def _register_grpcswift_generate_action(
actions: The context's actions object.
direct_srcs: The direct `.proto` sources belonging to the target being analyzed, which
will be passed to `protoc`.
proto_source_root: the source root for `direct_srcs`.
transitive_descriptor_sets: The transitive `DescriptorSet`s from the `proto_library` being
analyzed.
module_mapping_file: The `File` containing the mapping between `.proto` files and Swift
Expand All @@ -61,8 +69,19 @@ def _register_grpcswift_generate_action(
Returns:
A list of generated `.grpc.swift` files corresponding to the `.proto` sources.
"""
generated_files = declare_generated_files(label.name, actions, "grpc", direct_srcs)
generated_dir_path = extract_generated_dir_path(label.name, "grpc", generated_files)
generated_files = declare_generated_files(
label.name,
actions,
"grpc",
proto_source_root,
direct_srcs,
)
generated_dir_path = extract_generated_dir_path(
label.name,
"grpc",
proto_source_root,
generated_files,
)

mkdir_args = actions.args()
mkdir_args.add(generated_dir_path)
Expand Down Expand Up @@ -107,7 +126,7 @@ def _register_grpcswift_generate_action(
)
protoc_args.add("--descriptor_set_in")
protoc_args.add_joined(transitive_descriptor_sets, join_with = ":")
protoc_args.add_all([workspace_relative_path(f) for f in direct_srcs])
protoc_args.add_all([proto_import_path(f, proto_source_root) for f in direct_srcs])

additional_command_inputs = []
if module_mapping_file:
Expand Down Expand Up @@ -168,6 +187,7 @@ def _swift_grpc_library_impl(ctx):
ctx.label,
ctx.actions,
direct_srcs,
ctx.attr.srcs[0][ProtoInfo].proto_source_root,
transitive_descriptor_sets,
transitive_module_mapping_file,
ctx.executable._mkdir_and_run,
Expand Down
51 changes: 41 additions & 10 deletions swift/internal/swift_protoc_gen_aspect.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,13 @@ load(
"register_module_mapping_write_action",
)
load(":providers.bzl", "SwiftInfo", "SwiftProtoInfo", "SwiftToolchainInfo")
load(":utils.bzl", "compact", "create_cc_info", "get_providers", "workspace_relative_path")
load(
":utils.bzl",
"compact",
"create_cc_info",
"get_providers",
"proto_import_path",
)

# The paths of proto files bundled with the runtime. This is mainly the well
# known type protos, but also includes descriptor.proto to make generation of
Expand Down Expand Up @@ -67,11 +73,12 @@ This provider is an implementation detail not meant to be used by clients.
},
)

def _filter_out_well_known_types(srcs):
def _filter_out_well_known_types(srcs, proto_source_root):
"""Returns the given list of files, excluding any well-known type protos.

Args:
srcs: A list of `.proto` files.
proto_source_root: the source root where the `.proto` files are.

Returns:
The given list of files with any well-known type protos (those living under
Expand All @@ -80,13 +87,14 @@ def _filter_out_well_known_types(srcs):
return [
f
for f in srcs
if workspace_relative_path(f) not in _RUNTIME_BUNDLED_PROTO_FILES
if proto_import_path(f, proto_source_root) not in _RUNTIME_BUNDLED_PROTO_FILES
]

def _register_pbswift_generate_action(
label,
actions,
direct_srcs,
proto_source_root,
transitive_descriptor_sets,
module_mapping_file,
mkdir_and_run,
Expand All @@ -99,6 +107,7 @@ def _register_pbswift_generate_action(
actions: The context's actions object.
direct_srcs: The direct `.proto` sources belonging to the target being analyzed, which
will be passed to `protoc-gen-swift`.
proto_source_root: the source root where the `.proto` files are.
transitive_descriptor_sets: The transitive `DescriptorSet`s from the `proto_library` being
analyzed.
module_mapping_file: The `File` containing the mapping between `.proto` files and Swift
Expand All @@ -112,8 +121,19 @@ def _register_pbswift_generate_action(
Returns:
A list of generated `.pb.swift` files corresponding to the `.proto` sources.
"""
generated_files = declare_generated_files(label.name, actions, "pb", direct_srcs)
generated_dir_path = extract_generated_dir_path(label.name, "pb", generated_files)
generated_files = declare_generated_files(
label.name,
actions,
"pb",
proto_source_root,
direct_srcs,
)
generated_dir_path = extract_generated_dir_path(
label.name,
"pb",
proto_source_root,
generated_files,
)

mkdir_args = actions.args()
mkdir_args.add(generated_dir_path)
Expand Down Expand Up @@ -142,7 +162,9 @@ def _register_pbswift_generate_action(
)
protoc_args.add("--descriptor_set_in")
protoc_args.add_joined(transitive_descriptor_sets, join_with = ":")
protoc_args.add_all([workspace_relative_path(f) for f in direct_srcs])
protoc_args.add_all(
[proto_import_path(f, proto_source_root) for f in direct_srcs],
)

additional_command_inputs = []
if module_mapping_file:
Expand Down Expand Up @@ -195,12 +217,13 @@ def _build_swift_proto_info_provider(
),
)

def _build_module_mapping_from_srcs(target, proto_srcs):
def _build_module_mapping_from_srcs(target, proto_srcs, proto_source_root):
"""Returns the sequence of module mapping `struct`s for the given sources.

Args:
target: The `proto_library` target whose module mapping is being rendered.
proto_srcs: The `.proto` files that belong to the target.
proto_source_root: The source root for `proto_srcs`.

Returns:
A string containing the module mapping for the target in protobuf text
Expand All @@ -216,7 +239,7 @@ def _build_module_mapping_from_srcs(target, proto_srcs):
# update to protoc-gen-swift?
return struct(
module_name = swift_common.derive_module_name(target.label),
proto_file_paths = [workspace_relative_path(f) for f in proto_srcs],
proto_file_paths = [proto_import_path(f, proto_source_root) for f in proto_srcs],
)

def _gather_transitive_module_mappings(targets):
Expand Down Expand Up @@ -250,7 +273,10 @@ def _gather_transitive_module_mappings(targets):
def _swift_protoc_gen_aspect_impl(target, aspect_ctx):
swift_toolchain = aspect_ctx.attr._toolchain[SwiftToolchainInfo]

direct_srcs = _filter_out_well_known_types(target[ProtoInfo].direct_sources)
direct_srcs = _filter_out_well_known_types(
target[ProtoInfo].direct_sources,
target[ProtoInfo].proto_source_root,
)

# Direct sources are passed as arguments to protoc to generate *only* the
# files in this target, but we need to pass the transitive sources as inputs
Expand All @@ -264,7 +290,11 @@ def _swift_protoc_gen_aspect_impl(target, aspect_ctx):
minimal_module_mappings = []
if direct_srcs:
minimal_module_mappings.append(
_build_module_mapping_from_srcs(target, direct_srcs),
_build_module_mapping_from_srcs(
target,
direct_srcs,
target[ProtoInfo].proto_source_root,
),
)
if proto_deps:
minimal_module_mappings.extend(_gather_transitive_module_mappings(proto_deps))
Expand All @@ -283,6 +313,7 @@ def _swift_protoc_gen_aspect_impl(target, aspect_ctx):
target.label,
aspect_ctx.actions,
direct_srcs,
target[ProtoInfo].proto_source_root,
transitive_descriptor_sets,
transitive_module_mapping_file,
aspect_ctx.executable._mkdir_and_run,
Expand Down
17 changes: 17 additions & 0 deletions swift/internal/utils.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,20 @@ def workspace_relative_path(file):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Is anything left using workspace_relative_path or should it be made private to this file?

Copy link
Contributor Author

@lberki lberki Aug 30, 2019

Choose a reason for hiding this comment

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

You cannot know that -- anyone can load() this file and use this function from it. If you wish, I can remove it, but be aware that it's a potential incompatible change.

Copy link
Member

Choose a reason for hiding this comment

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

You cannot know that -- anyone can load()s this file and use this function from it. If you wish, I can remove it, but be aware that it's a potential incompatible change.

Actually, it is in an internal directory, so we're fine breaking folks that directly tried to use it. It just needs to be not used by anything else within rules_swift. The directory naming is all we can do until bazel has visibility for bzl files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So be it then!

workspace_path = paths.join(file.root.path, file.owner.workspace_root)
return paths.relativize(file.path, workspace_path)

def proto_import_path(f, proto_source_root):
""" Returns the import path of a `.proto` file given its path.

Args:
f: The `File` object representing the `.proto` file.
proto_source_root: The source root for the `.proto` file.

Returns:
The path the `.proto` file should be imported at.
"""
if f.path.startswith(proto_source_root):
return f.path[len(proto_source_root) + 1:]
else:
# Happens before Bazel 1.0, where proto_source_root was not
# guaranteed to be a parent of the .proto file
return workspace_relative_path(f)