From 2351eacb19faed3867903e1ddfa2992f7a787d02 Mon Sep 17 00:00:00 2001 From: Sam Delmerico Date: Tue, 24 May 2022 17:10:02 +0000 Subject: [PATCH] AIDL source generation accounts for Bazel paths The AIDL source generation rule sets include flags based on the relative path of .aidl sources. For .aidl sources provided by Bazel targets, e.g. in a filegroup, the same directory could be added to the include path twice. Instead we need to ensure that if a Bazel source provides the include path, that we don't add it again from a Soong source. Bug: 229251008 Test: USE_BAZEL_ANALYSIS=1 m api-stubs-docs-non-updatable Change-Id: I4997039003242b43e0e52ccf41729acb4ad11324 --- android/fixture.go | 14 ++++++++++++++ android/paths.go | 12 ++++++++++++ java/base.go | 22 +++++++++++++--------- java/droiddoc.go | 2 +- java/gen.go | 28 +++++++++++++++++++++++----- java/java_test.go | 34 ++++++++++++++++++++++++++++++++++ 6 files changed, 97 insertions(+), 15 deletions(-) diff --git a/android/fixture.go b/android/fixture.go index f71893571..f33e7189d 100644 --- a/android/fixture.go +++ b/android/fixture.go @@ -642,6 +642,20 @@ type TestResult struct { NinjaDeps []string } +type TestPathContext struct { + *TestResult +} + +var _ PathContext = &TestPathContext{} + +func (t *TestPathContext) Config() Config { + return t.TestResult.Config +} + +func (t *TestPathContext) AddNinjaFileDeps(deps ...string) { + panic("unimplemented") +} + func createFixture(t *testing.T, buildDir string, preparers []*simpleFixturePreparer) Fixture { config := TestConfig(buildDir, nil, "", nil) ctx := NewTestContext(config) diff --git a/android/paths.go b/android/paths.go index 74d9f13f4..1f6912589 100644 --- a/android/paths.go +++ b/android/paths.go @@ -1978,6 +1978,18 @@ func PathForTesting(paths ...string) Path { return testPath{basePath{path: p, rel: p}} } +func PathForTestingWithRel(path, rel string) Path { + p, err := validateSafePath(path, rel) + if err != nil { + panic(err) + } + r, err := validatePath(rel) + if err != nil { + panic(err) + } + return testPath{basePath{path: p, rel: r}} +} + // PathsForTesting returns a Path constructed from each element in strs. It should only be used from within tests. func PathsForTesting(strs ...string) Paths { p := make(Paths, len(strs)) diff --git a/java/base.go b/java/base.go index fe92b4754..e38f0b9c4 100644 --- a/java/base.go +++ b/java/base.go @@ -820,7 +820,7 @@ func (j *Module) individualAidlFlags(ctx android.ModuleContext, aidlFile android } func (j *Module) aidlFlags(ctx android.ModuleContext, aidlPreprocess android.OptionalPath, - aidlIncludeDirs android.Paths) (string, android.Paths) { + aidlIncludeDirs android.Paths, aidlSrcs android.Paths) (string, android.Paths) { aidlIncludes := android.PathsForModuleSrc(ctx, j.deviceProperties.Aidl.Local_include_dirs) aidlIncludes = append(aidlIncludes, @@ -830,6 +830,7 @@ func (j *Module) aidlFlags(ctx android.ModuleContext, aidlPreprocess android.Opt var flags []string var deps android.Paths + var includeDirs android.Paths flags = append(flags, j.deviceProperties.Aidl.Flags...) @@ -837,21 +838,24 @@ func (j *Module) aidlFlags(ctx android.ModuleContext, aidlPreprocess android.Opt flags = append(flags, "-p"+aidlPreprocess.String()) deps = append(deps, aidlPreprocess.Path()) } else if len(aidlIncludeDirs) > 0 { - flags = append(flags, android.JoinWithPrefix(aidlIncludeDirs.Strings(), "-I")) + includeDirs = append(includeDirs, aidlIncludeDirs...) } if len(j.exportAidlIncludeDirs) > 0 { - flags = append(flags, android.JoinWithPrefix(j.exportAidlIncludeDirs.Strings(), "-I")) + includeDirs = append(includeDirs, j.exportAidlIncludeDirs...) } if len(aidlIncludes) > 0 { - flags = append(flags, android.JoinWithPrefix(aidlIncludes.Strings(), "-I")) + includeDirs = append(includeDirs, aidlIncludes...) } - flags = append(flags, "-I"+android.PathForModuleSrc(ctx).String()) + includeDirs = append(includeDirs, android.PathForModuleSrc(ctx)) if src := android.ExistentPathForSource(ctx, ctx.ModuleDir(), "src"); src.Valid() { - flags = append(flags, "-I"+src.String()) + includeDirs = append(includeDirs, src.Path()) } + flags = append(flags, android.JoinWithPrefix(includeDirs.Strings(), "-I")) + // add flags for dirs containing AIDL srcs that haven't been specified yet + flags = append(flags, genAidlIncludeFlags(ctx, aidlSrcs, includeDirs)) if Bool(j.deviceProperties.Aidl.Generate_traces) { flags = append(flags, "-t") @@ -935,9 +939,6 @@ func (j *Module) collectBuilderFlags(ctx android.ModuleContext, deps deps) javaB // systemModules flags.systemModules = deps.systemModules - // aidl flags. - flags.aidlFlags, flags.aidlDeps = j.aidlFlags(ctx, deps.aidlPreprocess, deps.aidlIncludeDirs) - return flags } @@ -1046,6 +1047,9 @@ func (j *Module) compile(ctx android.ModuleContext, aaptSrcJar android.Path) { ctx.PropertyErrorf("common_srcs", "common_srcs must be .kt files") } + aidlSrcs := srcFiles.FilterByExt(".aidl") + flags.aidlFlags, flags.aidlDeps = j.aidlFlags(ctx, deps.aidlPreprocess, deps.aidlIncludeDirs, aidlSrcs) + nonGeneratedSrcJars := srcFiles.FilterByExt(".srcjar") srcFiles = j.genSources(ctx, srcFiles, flags) diff --git a/java/droiddoc.go b/java/droiddoc.go index 901419cba..9b1f43b4c 100644 --- a/java/droiddoc.go +++ b/java/droiddoc.go @@ -314,7 +314,7 @@ func (j *Javadoc) genSources(ctx android.ModuleContext, srcFiles android.Paths, outSrcFiles := make(android.Paths, 0, len(srcFiles)) var aidlSrcs android.Paths - aidlIncludeFlags := genAidlIncludeFlags(srcFiles) + aidlIncludeFlags := genAidlIncludeFlags(ctx, srcFiles, android.Paths{}) for _, srcFile := range srcFiles { switch srcFile.Ext() { diff --git a/java/gen.go b/java/gen.go index 1572bf00a..638da255a 100644 --- a/java/gen.go +++ b/java/gen.go @@ -15,6 +15,7 @@ package java import ( + "path/filepath" "strconv" "strings" @@ -116,12 +117,31 @@ func genLogtags(ctx android.ModuleContext, logtagsFile android.Path) android.Pat return javaFile } -func genAidlIncludeFlags(srcFiles android.Paths) string { +// genAidlIncludeFlags returns additional include flags based on the relative path +// of each .aidl file passed in srcFiles. excludeDirs is a list of paths relative to +// the Android checkout root that should not be included in the returned flags. +func genAidlIncludeFlags(ctx android.PathContext, srcFiles android.Paths, excludeDirs android.Paths) string { var baseDirs []string + excludeDirsStrings := excludeDirs.Strings() for _, srcFile := range srcFiles { if srcFile.Ext() == ".aidl" { baseDir := strings.TrimSuffix(srcFile.String(), srcFile.Rel()) - if baseDir != "" && !android.InList(baseDir, baseDirs) { + baseDir = filepath.Clean(baseDir) + baseDirSeen := android.InList(baseDir, baseDirs) || android.InList(baseDir, excludeDirsStrings) + + // For go/bp2build mixed builds, a file may be listed under a + // directory in the Bazel output tree that is symlinked to a + // directory under the android source tree. We should only + // include one copy of this directory so that the AIDL tool + // doesn't find multiple definitions of the same AIDL class. + // This code comes into effect when filegroups are used in mixed builds. + bazelPathPrefix := android.PathForBazelOut(ctx, "").String() + bazelBaseDir, err := filepath.Rel(bazelPathPrefix, baseDir) + bazelBaseDirSeen := err == nil && + android.InList(bazelBaseDir, baseDirs) || + android.InList(bazelBaseDir, excludeDirsStrings) + + if baseDir != "" && !baseDirSeen && !bazelBaseDirSeen { baseDirs = append(baseDirs, baseDir) } } @@ -136,8 +156,6 @@ func (j *Module) genSources(ctx android.ModuleContext, srcFiles android.Paths, var protoSrcs android.Paths var aidlSrcs android.Paths - aidlIncludeFlags := genAidlIncludeFlags(srcFiles) - for _, srcFile := range srcFiles { switch srcFile.Ext() { case ".aidl": @@ -168,7 +186,7 @@ func (j *Module) genSources(ctx android.ModuleContext, srcFiles android.Paths, individualFlags[aidlSrc.String()] = flags } } - srcJarFiles := genAidl(ctx, aidlSrcs, flags.aidlFlags+aidlIncludeFlags, individualFlags, flags.aidlDeps) + srcJarFiles := genAidl(ctx, aidlSrcs, flags.aidlFlags, individualFlags, flags.aidlDeps) outSrcFiles = append(outSrcFiles, srcJarFiles...) } diff --git a/java/java_test.go b/java/java_test.go index bfd97eb0d..7f0cea718 100644 --- a/java/java_test.go +++ b/java/java_test.go @@ -1747,3 +1747,37 @@ func TestImportMixedBuild(t *testing.T) { android.AssertDeepEquals(t, "Implementation/Resources JARs are produced", expectedOutputFiles, android.NormalizePathsForTesting(javaInfo.ImplementationAndResourcesJars)) android.AssertDeepEquals(t, "Implementation JARs are produced", expectedOutputFiles, android.NormalizePathsForTesting(javaInfo.ImplementationJars)) } + +func TestGenAidlIncludeFlagsForMixedBuilds(t *testing.T) { + bazelOutputBaseDir := filepath.Join("out", "bazel") + result := android.GroupFixturePreparers( + PrepareForIntegrationTestWithJava, + android.FixtureModifyConfig(func(config android.Config) { + config.BazelContext = android.MockBazelContext{ + OutputBaseDir: bazelOutputBaseDir, + } + }), + ).RunTest(t) + + ctx := &android.TestPathContext{TestResult: result} + + srcDirectory := filepath.Join("frameworks", "base") + srcDirectoryAlreadyIncluded := filepath.Join("frameworks", "base", "core", "java") + bazelSrcDirectory := android.PathForBazelOut(ctx, srcDirectory) + bazelSrcDirectoryAlreadyIncluded := android.PathForBazelOut(ctx, srcDirectoryAlreadyIncluded) + srcs := android.Paths{ + android.PathForTestingWithRel(bazelSrcDirectory.String(), "bazelAidl.aidl"), + android.PathForTestingWithRel(bazelSrcDirectory.String(), "bazelAidl2.aidl"), + android.PathForTestingWithRel(bazelSrcDirectoryAlreadyIncluded.String(), "bazelAidlExclude.aidl"), + android.PathForTestingWithRel(bazelSrcDirectoryAlreadyIncluded.String(), "bazelAidl2Exclude.aidl"), + } + dirsAlreadyIncluded := android.Paths{ + android.PathForTesting(srcDirectoryAlreadyIncluded), + } + + expectedFlags := " -Iout/bazel/execroot/__main__/frameworks/base" + flags := genAidlIncludeFlags(ctx, srcs, dirsAlreadyIncluded) + if flags != expectedFlags { + t.Errorf("expected flags to be %q; was %q", expectedFlags, flags) + } +}