From ef10f423d6caf75d062ff05c6c01ef8931d48a9b Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Tue, 7 Aug 2018 11:14:08 -0400 Subject: [PATCH 1/2] Break dependency on protobuf.bzl from google/protobuf for C++ This replaces the use of cc_proto_library / proto_gen from the @com_google_protobuf//:protobuf.bzl. The reason for doing so is that newer versions of google/protobuf have changed cc_proto_library + proto_gen so that they no longer work with pgv_cc_proto_library. pgv_proto_library now takes a proto_library containing the protos instead of a list of proto files. The approach for generating the protoc command line arguments was taken from bazelbuild/rules_go The py_proto_library imported from @com_google_protobuf//:protobuf.bzl is used by Envoy, so we left it in. Signed-off-by: Andrew Keesler Signed-off-by: Amin Jamali --- bazel/pgv_proto_library.bzl | 112 ++++++------------------ bazel/protobuf.bzl | 80 +++++++++++++++++ tests/harness/BUILD | 8 +- tests/harness/cases/BUILD | 20 +---- tests/harness/cases/other_package/BUILD | 4 +- validate/BUILD | 7 +- 6 files changed, 114 insertions(+), 117 deletions(-) create mode 100644 bazel/protobuf.bzl diff --git a/bazel/pgv_proto_library.bzl b/bazel/pgv_proto_library.bzl index bd6174ae1..d36fd1ec3 100644 --- a/bazel/pgv_proto_library.bzl +++ b/bazel/pgv_proto_library.bzl @@ -1,6 +1,6 @@ load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") load("@io_bazel_rules_go//proto:compiler.bzl", "go_proto_compiler") -load("@com_google_protobuf//:protobuf.bzl", "proto_gen", "cc_proto_library") +load(":protobuf.bzl", "cc_proto_gen_validate") def pgv_go_proto_library(name, proto = None, deps = [], **kwargs): go_proto_compiler( @@ -18,94 +18,34 @@ def pgv_go_proto_library(name, proto = None, deps = [], **kwargs): visibility = ["//visibility:public"], **kwargs) -def _CcValidateHdrs(srcs): - ret = [s[:-len(".proto")] + ".pb.validate.h" for s in srcs] - return ret - -def _CcValidateSrcs(srcs): - ret = [s[:-len(".proto")] + ".pb.validate.cc" for s in srcs] - return ret - def pgv_cc_proto_library( name, - srcs=[], + proto, deps=[], - external_deps=[], - cc_libs=[], - include=None, - protoc="@com_google_protobuf//:protoc", - protoc_gen_validate = "@com_lyft_protoc_gen_validate//:protoc-gen-validate", - internal_bootstrap_hack=False, - use_grpc_plugin=False, - default_runtime="@com_google_protobuf//:protobuf", **kargs): - """Bazel rule to create a C++ protobuf validation library from proto source files - - Args: - name: the name of the pgv_cc_proto_library. - srcs: the .proto files of the pgv_cc_proto_library. - deps: a list of PGV dependency labels; must be pgv_cc_proto_library. - external_deps: a list of dependency labels; must be cc_proto_library. - include: a string indicating the include path of the .proto files. - protoc: the label of the protocol compiler to generate the sources. - protoc_gen_validate: override the default version of protoc_gen_validate. - Most users won't need this. - default_runtime: the implicitly default runtime which will be depended on by - the generated cc_library target. - **kargs: other keyword arguments that are passed to cc_library. - - """ - - # Generate the C++ protos - cc_proto_library( - name=name + "_proto", - srcs=srcs, - deps=[d + "_proto" for d in deps] + [ - "@com_lyft_protoc_gen_validate//validate:validate_cc", - ] + external_deps, - cc_libs=cc_libs, - incude=include, - protoc=protoc, - internal_bootstrap_hack=internal_bootstrap_hack, - use_grpc_plugin=use_grpc_plugin, - default_runtime=default_runtime, - **kargs) - - includes = [] - if include != None: - includes = [include] - - gen_hdrs = _CcValidateHdrs(srcs) - gen_srcs = _CcValidateSrcs(srcs) - - proto_gen( - name=name + "_validate", - srcs=srcs, - # This is a hack to work around the fact that all the deps must have an - # import_flags field, which is only set on the proto_gen rules, so depend - # on the cc rule - deps=[d + "_validate" for d in deps] + [ - "@com_lyft_protoc_gen_validate//validate:validate_cc_genproto" - ] + [d + "_genproto" for d in external_deps], - includes=includes, - protoc=protoc, - plugin=protoc_gen_validate, - plugin_options=["lang=cc"], - outs=gen_hdrs + gen_srcs, - visibility=["//visibility:public"], - ) + """Bazel rule to create a C++ protobuf validation library from proto source files + Args: + name: the name of the pgv_cc_proto_library. + proto: a single proto_library rule which contains the necessary .proto files. Note that this + must include @com_lyft_protoc_gen_validate//validate:validate_proto + deps: C++ dependencies of the protos being compiled. Likely cc_proto_library or pgv_cc_proto_library + **kargs: other keyword arguments that are passed to cc_library. + """ + + cc_proto_gen_validate( + name=name+"_validate", + proto=proto, + ) - if default_runtime and not default_runtime in cc_libs: - cc_libs = cc_libs + [default_runtime] + native.cc_library( + name=name, + hdrs=[":"+name+"_validate"], + srcs=[":"+name+"_validate"], + deps= deps + [ + "@com_lyft_protoc_gen_validate//validate:cc_validate", + "@com_lyft_protoc_gen_validate//validate:validate_cc", + "@com_google_protobuf//:protobuf", + ], + alwayslink=1, + **kargs) - native.cc_library( - name=name, - hdrs=gen_hdrs, - srcs=gen_srcs, - deps=cc_libs + deps + [ - ":" + name + "_proto", - "@com_lyft_protoc_gen_validate//validate:cc_validate", - ], - includes=includes, - alwayslink=1, - **kargs) diff --git a/bazel/protobuf.bzl b/bazel/protobuf.bzl new file mode 100644 index 000000000..733798115 --- /dev/null +++ b/bazel/protobuf.bzl @@ -0,0 +1,80 @@ +def _proto_path(proto): + """ + The proto path is not really a file path + It's the path to the proto that was seen when the descriptor file was generated. + """ + path = proto.path + root = proto.root.path + ws = proto.owner.workspace_root + if path.startswith(root): + path = path[len(root):] + if path.startswith("/"): + path = path[1:] + if path.startswith(ws): + path = path[len(ws):] + if path.startswith("/"): + path = path[1:] + return path + +def _protoc_gen_validate_impl(ctx): + """Generate protos using protoc-gen-validate plugin""" + protos = [f for f in ctx.attr.proto.proto.direct_sources] + + cc_hdrs = [p.basename[:-len(".proto")] + ".pb.validate.h" for p in protos] + cc_hdrs += [p.basename[:-len(".proto")] + ".pb.h" for p in protos] + + cc_srcs = [p.basename[:-len(".proto")] + ".pb.validate.cc" for p in protos] + cc_srcs += [p.basename[:-len(".proto")] + ".pb.cc" for p in protos] + + out_files = [ctx.actions.declare_file(out) for out in cc_hdrs+cc_srcs] + + dir_out = ctx.genfiles_dir.path + if ctx.label.workspace_root: + dir_out += ("/"+ctx.label.workspace_root) + + args = [ + "--cpp_out="+dir_out, + "--plugin=protoc-gen-validate="+ctx.executable._plugin.path, + "--validate_out=lang=cc:"+ dir_out, + ] + + tds = ctx.attr.proto.proto.transitive_descriptor_sets + if tds: + args += ["--descriptor_set_in=%s" % ":".join([ds.path for ds in tds])] + + ctx.action( + inputs=protos + tds.to_list() + [ctx.executable._plugin], + outputs=out_files, + arguments=args + [_proto_path(proto) for proto in protos], + executable=ctx.executable._protoc, + mnemonic="ProtoGenValidateCompile", + use_default_shell_env=True, + ) + + return struct( + files=depset(out_files) + ) + +cc_proto_gen_validate = rule( + attrs = { + "proto": attr.label( + mandatory = True, + providers = ["proto"], + ), + + "_protoc": attr.label( + cfg = "host", + default = Label("@com_google_protobuf//:protoc"), + executable = True, + single_file = True, + ), + "_plugin": attr.label( + cfg = "host", + default = Label("@com_lyft_protoc_gen_validate//:protoc-gen-validate"), + allow_files = True, + executable = True, + ), + }, + output_to_genfiles = True, + implementation = _protoc_gen_validate_impl, +) diff --git a/tests/harness/BUILD b/tests/harness/BUILD index b4494eb66..8ae7890c0 100644 --- a/tests/harness/BUILD +++ b/tests/harness/BUILD @@ -1,7 +1,6 @@ # gazelle:exclude harness.pb.go load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") -load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library") proto_library( name = "harness_proto_target", @@ -27,11 +26,8 @@ go_proto_library( cc_proto_library( name = "harness_proto", - srcs = ["harness.proto"], - default_runtime = "@com_google_protobuf//:protobuf", - protoc = "@com_google_protobuf//:protoc", - visibility = ["//visibility:public"], deps = [ - "@com_google_protobuf//:cc_wkt_protos", + ":harness_proto_target", ], + visibility = ["//visibility:public"], ) diff --git a/tests/harness/cases/BUILD b/tests/harness/cases/BUILD index 23f91b9d0..fef2730c8 100644 --- a/tests/harness/cases/BUILD +++ b/tests/harness/cases/BUILD @@ -8,7 +8,7 @@ load( ) proto_library( - name = "go_proto", + name = "cases_proto", srcs = [ "bool.proto", "bytes.proto", @@ -36,7 +36,7 @@ proto_library( pgv_go_proto_library( name = "go", - proto = ":go_proto", + proto = ":cases_proto", importpath = "github.com/lyft/protoc-gen-validate/tests/harness/cases/go", deps = [ "//tests/harness/cases/other_package:go", @@ -64,21 +64,7 @@ go_library( pgv_cc_proto_library( name = "cc", - srcs = [ - "bool.proto", - "bytes.proto", - "enums.proto", - "maps.proto", - "messages.proto", - "numbers.proto", - "oneofs.proto", - "repeated.proto", - "strings.proto", - "wkt_any.proto", - "wkt_duration.proto", - "wkt_timestamp.proto", - "wkt_wrappers.proto", - ], + proto = ":cases_proto", deps = [ "//tests/harness/cases/other_package:cc", ], diff --git a/tests/harness/cases/other_package/BUILD b/tests/harness/cases/other_package/BUILD index a441115f0..b0fcad5e1 100644 --- a/tests/harness/cases/other_package/BUILD +++ b/tests/harness/cases/other_package/BUILD @@ -40,8 +40,6 @@ go_library( pgv_cc_proto_library( name = "cc", - srcs = [ - "embed.proto", - ], + proto = ":embed_proto", visibility = ["//tests:__subpackages__"], ) diff --git a/validate/BUILD b/validate/BUILD index 5ab41d4f0..9493d1dc2 100644 --- a/validate/BUILD +++ b/validate/BUILD @@ -1,7 +1,7 @@ # gazelle:exclude validate.pb.go load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library") -load("@com_google_protobuf//:protobuf.bzl", "cc_proto_library", "py_proto_library") +load("@com_google_protobuf//:protobuf.bzl", "py_proto_library") proto_library( name = "validate_proto", @@ -16,10 +16,7 @@ proto_library( cc_proto_library( name = "validate_cc", - srcs = ["validate.proto"], - protoc = "@com_google_protobuf//:protoc", - default_runtime = "@com_google_protobuf//:protobuf", - deps = ["@com_google_protobuf//:cc_wkt_protos"], + deps = [":validate_proto"], visibility = ["//visibility:public"], ) From 8de962732b0bde93d51a47c2eb4084e41cbae6ad Mon Sep 17 00:00:00 2001 From: Amin Jamali Date: Tue, 7 Aug 2018 14:22:06 -0400 Subject: [PATCH 2/2] Fix nits - change mnemonic to ProtoGenValidateCcGenerate - accept array of proto_library rules as argument Signed-off-by: Sam Smith --- bazel/pgv_proto_library.bzl | 10 +++++----- bazel/protobuf.bzl | 18 +++++++++++------- tests/harness/cases/BUILD | 4 ++-- tests/harness/cases/other_package/BUILD | 2 +- tests/kitchensink/BUILD | 2 +- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/bazel/pgv_proto_library.bzl b/bazel/pgv_proto_library.bzl index d36fd1ec3..39ad0a9bb 100644 --- a/bazel/pgv_proto_library.bzl +++ b/bazel/pgv_proto_library.bzl @@ -20,28 +20,28 @@ def pgv_go_proto_library(name, proto = None, deps = [], **kwargs): def pgv_cc_proto_library( name, - proto, deps=[], + cc_deps=[], **kargs): """Bazel rule to create a C++ protobuf validation library from proto source files Args: name: the name of the pgv_cc_proto_library. - proto: a single proto_library rule which contains the necessary .proto files. Note that this + deps: proto_library rules that contains the necessary .proto files. Note that this must include @com_lyft_protoc_gen_validate//validate:validate_proto - deps: C++ dependencies of the protos being compiled. Likely cc_proto_library or pgv_cc_proto_library + cc_deps: C++ dependencies of the protos being compiled. Likely cc_proto_library or pgv_cc_proto_library **kargs: other keyword arguments that are passed to cc_library. """ cc_proto_gen_validate( name=name+"_validate", - proto=proto, + deps=deps, ) native.cc_library( name=name, hdrs=[":"+name+"_validate"], srcs=[":"+name+"_validate"], - deps= deps + [ + deps= cc_deps + [ "@com_lyft_protoc_gen_validate//validate:cc_validate", "@com_lyft_protoc_gen_validate//validate:validate_cc", "@com_google_protobuf//:protobuf", diff --git a/bazel/protobuf.bzl b/bazel/protobuf.bzl index 733798115..61770548d 100644 --- a/bazel/protobuf.bzl +++ b/bazel/protobuf.bzl @@ -18,7 +18,9 @@ def _proto_path(proto): def _protoc_gen_validate_impl(ctx): """Generate protos using protoc-gen-validate plugin""" - protos = [f for f in ctx.attr.proto.proto.direct_sources] + protos = [] + for dep in ctx.attr.deps: + protos += [f for f in dep.proto.direct_sources] cc_hdrs = [p.basename[:-len(".proto")] + ".pb.validate.h" for p in protos] cc_hdrs += [p.basename[:-len(".proto")] + ".pb.h" for p in protos] @@ -38,16 +40,18 @@ def _protoc_gen_validate_impl(ctx): "--validate_out=lang=cc:"+ dir_out, ] - tds = ctx.attr.proto.proto.transitive_descriptor_sets - if tds: - args += ["--descriptor_set_in=%s" % ":".join([ds.path for ds in tds])] + tds = depset([], transitive = [dep.proto.transitive_descriptor_sets for dep in ctx.attr.deps]) + descriptor_args = [ds.path for ds in tds] + + if len(descriptor_args) != 0: + args += ["--descriptor_set_in=%s" % ":".join(descriptor_args)] ctx.action( - inputs=protos + tds.to_list() + [ctx.executable._plugin], + inputs=protos + tds.to_list() + [ctx.executable._plugin], outputs=out_files, arguments=args + [_proto_path(proto) for proto in protos], executable=ctx.executable._protoc, - mnemonic="ProtoGenValidateCompile", + mnemonic="ProtoGenValidateCcGenerate", use_default_shell_env=True, ) @@ -57,7 +61,7 @@ def _protoc_gen_validate_impl(ctx): cc_proto_gen_validate = rule( attrs = { - "proto": attr.label( + "deps": attr.label_list( mandatory = True, providers = ["proto"], ), diff --git a/tests/harness/cases/BUILD b/tests/harness/cases/BUILD index fef2730c8..99174921e 100644 --- a/tests/harness/cases/BUILD +++ b/tests/harness/cases/BUILD @@ -64,8 +64,8 @@ go_library( pgv_cc_proto_library( name = "cc", - proto = ":cases_proto", - deps = [ + deps = [":cases_proto"], + cc_deps = [ "//tests/harness/cases/other_package:cc", ], visibility = ["//tests:__subpackages__"], diff --git a/tests/harness/cases/other_package/BUILD b/tests/harness/cases/other_package/BUILD index b0fcad5e1..49ea39587 100644 --- a/tests/harness/cases/other_package/BUILD +++ b/tests/harness/cases/other_package/BUILD @@ -40,6 +40,6 @@ go_library( pgv_cc_proto_library( name = "cc", - proto = ":embed_proto", + deps = [":embed_proto"], visibility = ["//tests:__subpackages__"], ) diff --git a/tests/kitchensink/BUILD b/tests/kitchensink/BUILD index 3ed708fd0..5a0d8188d 100644 --- a/tests/kitchensink/BUILD +++ b/tests/kitchensink/BUILD @@ -21,7 +21,7 @@ pgv_go_proto_library( pgv_cc_proto_library( name = "fixed32_cc", - srcs = ["fixed32.proto"], + deps = [":fixed32_proto"], ) proto_library(