From bc9c8b494ca176505c5d0cb5cfad7a61c07145f6 Mon Sep 17 00:00:00 2001 From: Vinh Tran Date: Fri, 9 Dec 2022 12:03:52 -0500 Subject: [PATCH] Add check for handcrafted BUILD file in bp2build ag/20469925 added a handcrafted BUILD file (allowlisted in ag/20597986) to internal master. ag/c/platform/build/soong/+/20583593/12/cc/bp2build.go#818 generates references to the targets in the BUILD file. However in tm-dev, the BUILD file doesn't exist while the .afdo files do. One solution is to cherry-pick the BUILD file to tm-dev. However, tm-dev doesn't have vendor/google/build/soong/bp2build_allowlist.go to check in the BUILD file in bp2build. This CL adds a check that the BUILD file exists to avoid the failure as in https://android-build.googleplex.com/builds/pending/P45724500/aosp_cf_x86_64_phone-userdebug/latest/view/logs/build_error.log in tm-qpr-dev-plus-aosp branch. Bug: 253540178 Test: go test Change-Id: I47fb853015ca230afe3cefe1d37728bf714624be Merged-In: I47fb853015ca230afe3cefe1d37728bf714624be --- bp2build/cc_library_conversion_test.go | 25 ++++++++++++++++--- cc/bp2build.go | 34 ++++++++++++++------------ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/bp2build/cc_library_conversion_test.go b/bp2build/cc_library_conversion_test.go index 1ffdc0e8b..416e1298d 100644 --- a/bp2build/cc_library_conversion_test.go +++ b/bp2build/cc_library_conversion_test.go @@ -3619,7 +3619,10 @@ cc_library { }{ { description: "cc_library with afdo enabled and existing profile", - filesystem: map[string]string{"vendor/google_data/pgo_profile/sampling/foo.afdo": ""}, + filesystem: map[string]string{ + "vendor/google_data/pgo_profile/sampling/BUILD": "", + "vendor/google_data/pgo_profile/sampling/foo.afdo": "", + }, expectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{}), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ @@ -3629,7 +3632,10 @@ cc_library { }, { description: "cc_library with afdo enabled and existing profile in AOSP", - filesystem: map[string]string{"toolchain/pgo-profiles/sampling/foo.afdo": ""}, + filesystem: map[string]string{ + "toolchain/pgo-profiles/sampling/BUILD": "", + "toolchain/pgo-profiles/sampling/foo.afdo": "", + }, expectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{}), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{ @@ -3639,7 +3645,10 @@ cc_library { }, { description: "cc_library with afdo enabled but profile filename doesn't match with module name", - filesystem: map[string]string{"toolchain/pgo-profiles/sampling/bar.afdo": ""}, + filesystem: map[string]string{ + "toolchain/pgo-profiles/sampling/BUILD": "", + "toolchain/pgo-profiles/sampling/bar.afdo": "", + }, expectedBazelTargets: []string{ MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{}), MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{}), @@ -3652,6 +3661,16 @@ cc_library { MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{}), }, }, + { + description: "cc_library with afdo enabled and existing profile but BUILD file doesn't exist", + filesystem: map[string]string{ + "vendor/google_data/pgo_profile/sampling/foo.afdo": "", + }, + expectedBazelTargets: []string{ + MakeBazelTarget("cc_library_static", "foo_bp2build_cc_library_static", AttrNameToString{}), + MakeBazelTarget("cc_library_shared", "foo", AttrNameToString{}), + }, + }, } for _, testCase := range testCases { t.Run(testCase.description, func(t *testing.T) { diff --git a/cc/bp2build.go b/cc/bp2build.go index 598e35001..3cd0b1a3f 100644 --- a/cc/bp2build.go +++ b/cc/bp2build.go @@ -811,21 +811,25 @@ func bp2buildFdoProfile( m *Module, ) *bazel.Label { for _, project := range globalAfdoProfileProjects { - // We handcraft a BUILD file with fdo_profile targets that use the existing profiles in the project - // This implementation is assuming that every afdo profile in globalAfdoProfileProjects already has - // an associated fdo_profile target declared in the same package. - // TODO(b/260714900): Handle arch-specific afdo profiles (e.g. `-arm<64>.afdo`) - path := android.ExistentPathForSource(ctx, project, m.Name()+".afdo") - if path.Valid() { - // FIXME: Some profiles only exist internally and are not released to AOSP. - // When generated BUILD files are checked in, we'll run into merge conflict. - // The cc_library_shared target in AOSP won't have reference to an fdo_profile target because - // the profile doesn't exist. Internally, the same cc_library_shared target will - // have reference to the fdo_profile. - // For more context, see b/258682955#comment2 - fdoProfileLabel := "//" + strings.TrimSuffix(project, "/") + ":" + m.Name() - return &bazel.Label{ - Label: fdoProfileLabel, + // Ensure handcrafted BUILD file exists in the project + BUILDPath := android.ExistentPathForSource(ctx, project, "BUILD") + if BUILDPath.Valid() { + // We handcraft a BUILD file with fdo_profile targets that use the existing profiles in the project + // This implementation is assuming that every afdo profile in globalAfdoProfileProjects already has + // an associated fdo_profile target declared in the same package. + // TODO(b/260714900): Handle arch-specific afdo profiles (e.g. `-arm<64>.afdo`) + path := android.ExistentPathForSource(ctx, project, m.Name()+".afdo") + if path.Valid() { + // FIXME: Some profiles only exist internally and are not released to AOSP. + // When generated BUILD files are checked in, we'll run into merge conflict. + // The cc_library_shared target in AOSP won't have reference to an fdo_profile target because + // the profile doesn't exist. Internally, the same cc_library_shared target will + // have reference to the fdo_profile. + // For more context, see b/258682955#comment2 + fdoProfileLabel := "//" + strings.TrimSuffix(project, "/") + ":" + m.Name() + return &bazel.Label{ + Label: fdoProfileLabel, + } } } }