From f26ee15e01cc11ec26c8654a962605c5f44771ee Mon Sep 17 00:00:00 2001 From: Spandan Das Date: Thu, 24 Aug 2023 23:21:04 +0000 Subject: [PATCH] Bugfixes for proto_library (proto.include_dirs) The fixes are - Dedupe the dir list. Since we partition the proto srcs per pacakge and then iterate the map, a single include directory was being listed multiple times - Drop target_compatible_with from these proto_library targets. The compatibility will be enforced at the top-level _proto_library. Test: Added a unit test Change-Id: Ia18c0f8c495585010fd4e372092afe80c5d8290c --- android/mutator.go | 1 + android/proto.go | 12 +++++- bp2build/cc_library_conversion_test.go | 52 ++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/android/mutator.go b/android/mutator.go index 6bcac93c1..41477b8ed 100644 --- a/android/mutator.go +++ b/android/mutator.go @@ -231,6 +231,7 @@ type Bp2buildMutatorContext interface { BazelConversionPathContext CreateBazelTargetModule(bazel.BazelTargetModuleProperties, CommonAttributes, interface{}) + CreateBazelTargetModuleWithRestrictions(bazel.BazelTargetModuleProperties, CommonAttributes, interface{}, bazel.BoolAttribute) } // PreArchBp2BuildMutators adds mutators to be register for converting Android Blueprint modules diff --git a/android/proto.go b/android/proto.go index b9365a70f..4cd73e3a8 100644 --- a/android/proto.go +++ b/android/proto.go @@ -330,7 +330,8 @@ func Bp2buildProtoProperties(ctx Bp2buildMutatorContext, m *ModuleBase, srcs baz Label: l, }) } - protoLibrariesInIncludeDir := createProtoLibraryTargetsForIncludeDirs(ctx, protoIncludeDirs) + // Partitioning by packages can create dupes of protoIncludeDirs, so dedupe it first. + protoLibrariesInIncludeDir := createProtoLibraryTargetsForIncludeDirs(ctx, SortedUniqueStrings(protoIncludeDirs)) transitiveProtoLibraries.Append(protoLibrariesInIncludeDir) } @@ -401,7 +402,13 @@ func createProtoLibraryTargetsForIncludeDirs(ctx Bp2buildMutatorContext, include if rel != "." { attrs.Import_prefix = proptools.StringPtr(rel) } - ctx.CreateBazelTargetModule( + + // If a specific directory is listed in proto.include_dirs of two separate modules (one host-specific and another device-specific), + // we do not want to create the proto_library with target_compatible_with of the first visited of these two modules + // As a workarounds, delete `target_compatible_with` + alwaysEnabled := bazel.BoolAttribute{} + alwaysEnabled.Value = proptools.BoolPtr(true) + ctx.CreateBazelTargetModuleWithRestrictions( bazel.BazelTargetModuleProperties{Rule_class: "proto_library"}, CommonAttributes{ Name: label, @@ -411,6 +418,7 @@ func createProtoLibraryTargetsForIncludeDirs(ctx Bp2buildMutatorContext, include Tags: bazel.MakeStringListAttribute([]string{"manual"}), }, &attrs, + alwaysEnabled, ) } } diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index 650121724..02ed57a51 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -5120,7 +5120,7 @@ cc_library_static { // bar dir tc.Dir = "bar" tc.ExpectedBazelTargets = []string{ - MakeBazelTarget("proto_library", "bar.include_dir_bp2build_generated_proto", AttrNameToString{ + MakeBazelTargetNoRestrictions("proto_library", "bar.include_dir_bp2build_generated_proto", AttrNameToString{ "srcs": `["bar.proto"]`, "strip_import_prefix": `""`, "tags": `["manual"]`, @@ -5131,7 +5131,7 @@ cc_library_static { // bar/baz dir tc.Dir = "bar/baz" tc.ExpectedBazelTargets = []string{ - MakeBazelTarget("proto_library", "bar.include_dir_bp2build_generated_proto", AttrNameToString{ + MakeBazelTargetNoRestrictions("proto_library", "bar.include_dir_bp2build_generated_proto", AttrNameToString{ "srcs": `["//bar/baz:baz.proto"]`, "strip_import_prefix": `""`, "import_prefix": `"baz"`, @@ -5141,6 +5141,52 @@ cc_library_static { runCcLibraryTestCase(t, tc) } +func TestProtoIncludeDirsWithSrcsInMultiplePackages(t *testing.T) { + tc := Bp2buildTestCase{ + Description: "cc_library has srcs in multiple bazel packages and uses proto.include_dirs", + ModuleTypeUnderTest: "cc_library", + ModuleTypeUnderTestFactory: cc.LibraryFactory, + Blueprint: ` +cc_library_static { + name: "foo", + srcs: [ + "foo.proto", + "bar/bar.proto", + ], + proto: { + include_dirs: ["baz"], + } +} +` + simpleModuleDoNotConvertBp2build("cc_library", "libprotobuf-cpp-lite"), + Filesystem: map[string]string{ + "bar/Android.bp": "", // package boundary + "baz/Android.bp": "", + "baz/baz.proto": "", + }, + } + + tc.ExpectedBazelTargets = []string{ + MakeBazelTarget("cc_library_static", "foo", AttrNameToString{ + "local_includes": `["."]`, + "deps": `[":libprotobuf-cpp-lite"]`, + "implementation_whole_archive_deps": `[":foo_cc_proto_lite"]`, + }), + MakeBazelTarget("proto_library", "foo_proto", AttrNameToString{ + "srcs": `["foo.proto"]`, + "tags": `["manual"]`, + }), + MakeBazelTarget("cc_lite_proto_library", "foo_cc_proto_lite", AttrNameToString{ + "deps": `[ + ":foo_proto", + "//bar:foo_proto", + ]`, + "transitive_deps": `["//baz:baz.include_dir_bp2build_generated_proto"]`, + }), + } + runCcLibraryTestCase(t, tc) + +} + func TestProtoLocalIncludeDirs(t *testing.T) { tc := Bp2buildTestCase{ Description: "cc_library depends on .proto files using proto.local_include_dirs", @@ -5187,7 +5233,7 @@ func TestProtoLocalIncludeDirs(t *testing.T) { // foo/foo_subdir tc.Dir = "foo/foo_subdir" tc.ExpectedBazelTargets = []string{ - MakeBazelTarget("proto_library", "foo.foo_subdir.include_dir_bp2build_generated_proto", AttrNameToString{ + MakeBazelTargetNoRestrictions("proto_library", "foo.foo_subdir.include_dir_bp2build_generated_proto", AttrNameToString{ "srcs": `["foo_subdir.proto"]`, "strip_import_prefix": `""`, "tags": `["manual"]`,