From e0aa52f6c720305113fb935386ae53bd4ec81d88 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Thu, 18 Jul 2024 15:35:44 -0700 Subject: [PATCH 1/3] Run TestClasspath subtests in parallel Call t.Parallel() to make the subtests of TestClasspath run in parallel, which drops the runtime from 12 seconds to 1.7 seconds. Test: TestClasspath Flag: EXEMPT refactor Change-Id: Id2194f6b5f925b480280e9cf8da5d5767a467430 --- java/sdk_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/java/sdk_test.go b/java/sdk_test.go index 9e8ba6ed0..2dac27af1 100644 --- a/java/sdk_test.go +++ b/java/sdk_test.go @@ -388,7 +388,9 @@ func TestClasspath(t *testing.T) { }, } + t.Parallel() t.Run("basic", func(t *testing.T) { + t.Parallel() testClasspathTestCases(t, classpathTestcases, false) }) @@ -404,6 +406,7 @@ func testClasspathTestCases(t *testing.T, classpathTestcases []classpathTestCase } t.Run(testcase.name, func(t *testing.T) { + t.Parallel() moduleType := "java_library" if testcase.moduleType != "" { moduleType = testcase.moduleType From 3b1c6847c4920392ca00fe32fe3acc17246eda91 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 26 Jul 2024 11:52:57 -0700 Subject: [PATCH 2/3] Make PathForArbitraryOutput return an OutputPath The only place basePath is used as a Path is when being returned from PathForArbitraryOutput. Using it as a Path requires implementing the RelativeToTop() method on it, which then allowed that method to be inherited by SourcePath, breaking the contract on RelativeToTop that specifies that the returned Path is of the same concrete type as the input Path. Make PathForArbitraryOutput return an OutputPath, and update OutputPath to support base paths that are not out/soong. This also fixes RelativeToTop, which was previously not working for PathForArbitraryOutput paths. Test: all soong tests pass Flag: EXEMPT refactor Change-Id: Ie8d8e2290961f35280e97137d2bd641c4d57ab87 --- android/paths.go | 42 +++++++++++++++++++++-------------- android/testing.go | 16 ++++++++----- filesystem/filesystem_test.go | 5 ++--- java/app_set_test.go | 2 +- 4 files changed, 39 insertions(+), 26 deletions(-) diff --git a/android/paths.go b/android/paths.go index dda48dd54..0661b7697 100644 --- a/android/paths.go +++ b/android/paths.go @@ -245,13 +245,13 @@ type Path interface { // A standard build has the following structure: // ../top/ // out/ - make install files go here. - // out/soong - this is the soongOutDir passed to NewTestConfig() + // out/soong - this is the outDir passed to NewTestConfig() // ... - the source files // // This function converts a path so that it appears relative to the ../top/ directory, i.e. - // * Make install paths, which have the pattern "soongOutDir/../" are converted into the top + // * Make install paths, which have the pattern "outDir/../" are converted into the top // relative path "out/" - // * Soong install paths and other writable paths, which have the pattern "soongOutDir/" are + // * Soong install paths and other writable paths, which have the pattern "outDir/soong/" are // converted into the top relative path "out/soong/". // * Source paths are already relative to the top. // * Phony paths are not relative to anything. @@ -261,8 +261,9 @@ type Path interface { } const ( - OutDir = "out" - OutSoongDir = OutDir + "/soong" + testOutDir = "out" + testOutSoongSubDir = "/soong" + TestOutSoongDir = testOutDir + testOutSoongSubDir ) // WritablePath is a type of path that can be used as an output for build rules. @@ -1218,11 +1219,13 @@ func PathForSource(ctx PathContext, pathComponents ...string) SourcePath { // PathForArbitraryOutput creates a path for the given components. Unlike PathForOutput, // the path is relative to the root of the output folder, not the out/soong folder. func PathForArbitraryOutput(ctx PathContext, pathComponents ...string) Path { - p, err := validatePath(pathComponents...) + path, err := validatePath(pathComponents...) if err != nil { reportPathError(ctx, err) } - return basePath{path: filepath.Join(ctx.Config().OutDir(), p)} + fullPath := filepath.Join(ctx.Config().OutDir(), path) + path = fullPath[len(fullPath)-len(path):] + return OutputPath{basePath{path, ""}, ctx.Config().OutDir(), fullPath} } // MaybeExistentPathForSource joins the provided path components and validates that the result @@ -1325,8 +1328,8 @@ func (p SourcePath) OverlayPath(ctx ModuleMissingDepsPathContext, path Path) Opt type OutputPath struct { basePath - // The soong build directory, i.e. Config.SoongOutDir() - soongOutDir string + // The base out directory for this path, either Config.SoongOutDir() or Config.OutDir() + outDir string fullPath string } @@ -1334,7 +1337,7 @@ type OutputPath struct { func (p OutputPath) GobEncode() ([]byte, error) { w := new(bytes.Buffer) encoder := gob.NewEncoder(w) - err := errors.Join(encoder.Encode(p.basePath), encoder.Encode(p.soongOutDir), encoder.Encode(p.fullPath)) + err := errors.Join(encoder.Encode(p.basePath), encoder.Encode(p.outDir), encoder.Encode(p.fullPath)) if err != nil { return nil, err } @@ -1345,7 +1348,7 @@ func (p OutputPath) GobEncode() ([]byte, error) { func (p *OutputPath) GobDecode(data []byte) error { r := bytes.NewBuffer(data) decoder := gob.NewDecoder(r) - err := errors.Join(decoder.Decode(&p.basePath), decoder.Decode(&p.soongOutDir), decoder.Decode(&p.fullPath)) + err := errors.Join(decoder.Decode(&p.basePath), decoder.Decode(&p.outDir), decoder.Decode(&p.fullPath)) if err != nil { return err } @@ -1365,7 +1368,7 @@ func (p OutputPath) WithoutRel() OutputPath { } func (p OutputPath) getSoongOutDir() string { - return p.soongOutDir + return p.outDir } func (p OutputPath) RelativeToTop() Path { @@ -1373,8 +1376,13 @@ func (p OutputPath) RelativeToTop() Path { } func (p OutputPath) outputPathRelativeToTop() OutputPath { - p.fullPath = StringPathRelativeToTop(p.soongOutDir, p.fullPath) - p.soongOutDir = OutSoongDir + p.fullPath = StringPathRelativeToTop(p.outDir, p.fullPath) + if strings.HasSuffix(p.outDir, testOutSoongSubDir) { + p.outDir = TestOutSoongDir + } else { + // Handle the PathForArbitraryOutput case + p.outDir = testOutDir + } return p } @@ -1420,7 +1428,7 @@ func PathForOutput(ctx PathContext, pathComponents ...string) OutputPath { return OutputPath{basePath{path, ""}, ctx.Config().soongOutDir, fullPath} } -// PathsForOutput returns Paths rooted from soongOutDir +// PathsForOutput returns Paths rooted from outDir func PathsForOutput(ctx PathContext, paths []string) WritablePaths { ret := make(WritablePaths, len(paths)) for i, path := range paths { @@ -1751,9 +1759,9 @@ func ensureTestOnly() { func (p InstallPath) RelativeToTop() Path { ensureTestOnly() if p.makePath { - p.soongOutDir = OutDir + p.soongOutDir = testOutDir } else { - p.soongOutDir = OutSoongDir + p.soongOutDir = TestOutSoongDir } p.fullPath = filepath.Join(p.soongOutDir, p.path) return p diff --git a/android/testing.go b/android/testing.go index e39a1a749..dae787b7f 100644 --- a/android/testing.go +++ b/android/testing.go @@ -822,15 +822,15 @@ func newBaseTestingComponent(config Config, provider testBuildProvider) baseTest // containing at most one instance of the temporary build directory at the start of the path while // this assumes that there can be any number at any position. func normalizeStringRelativeToTop(config Config, s string) string { - // The soongOutDir usually looks something like: /tmp/testFoo2345/001 + // The outDir usually looks something like: /tmp/testFoo2345/001 // - // Replace any usage of the soongOutDir with out/soong, e.g. replace "/tmp/testFoo2345/001" with + // Replace any usage of the outDir with out/soong, e.g. replace "/tmp/testFoo2345/001" with // "out/soong". outSoongDir := filepath.Clean(config.soongOutDir) re := regexp.MustCompile(`\Q` + outSoongDir + `\E\b`) s = re.ReplaceAllString(s, "out/soong") - // Replace any usage of the soongOutDir/.. with out, e.g. replace "/tmp/testFoo2345" with + // Replace any usage of the outDir/.. with out, e.g. replace "/tmp/testFoo2345" with // "out". This must come after the previous replacement otherwise this would replace // "/tmp/testFoo2345/001" with "out/001" instead of "out/soong". outDir := filepath.Dir(outSoongDir) @@ -1234,8 +1234,14 @@ func StringPathRelativeToTop(soongOutDir string, path string) string { } if isRel { - // The path is in the soong out dir so indicate that in the relative path. - return filepath.Join("out/soong", rel) + if strings.HasSuffix(soongOutDir, testOutSoongSubDir) { + // The path is in the soong out dir so indicate that in the relative path. + return filepath.Join(TestOutSoongDir, rel) + } else { + // Handle the PathForArbitraryOutput case + return filepath.Join(testOutDir, rel) + + } } // Check to see if the path is relative to the top level out dir. diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 2dc8c21e0..8c0d11178 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -16,7 +16,6 @@ package filesystem import ( "os" - "path/filepath" "testing" "android/soong/android" @@ -147,8 +146,8 @@ func TestIncludeMakeBuiltFiles(t *testing.T) { output := result.ModuleForTests("myfilesystem", "android_common").Output("myfilesystem.img") - stampFile := filepath.Join(result.Config.OutDir(), "target/product/test_device/obj/PACKAGING/system_intermediates/staging_dir.stamp") - fileListFile := filepath.Join(result.Config.OutDir(), "target/product/test_device/obj/PACKAGING/system_intermediates/file_list.txt") + stampFile := "out/target/product/test_device/obj/PACKAGING/system_intermediates/staging_dir.stamp" + fileListFile := "out/target/product/test_device/obj/PACKAGING/system_intermediates/file_list.txt" android.AssertStringListContains(t, "deps of filesystem must include the staging dir stamp file", output.Implicits.Strings(), stampFile) android.AssertStringListContains(t, "deps of filesystem must include the staging dir file list", output.Implicits.Strings(), fileListFile) } diff --git a/java/app_set_test.go b/java/app_set_test.go index 10bc5de92..c02b3593b 100644 --- a/java/app_set_test.go +++ b/java/app_set_test.go @@ -56,7 +56,7 @@ func TestAndroidAppSet(t *testing.T) { mkEntries := android.AndroidMkEntriesForTest(t, result.TestContext, module.Module())[0] actualInstallFile := mkEntries.EntryMap["LOCAL_APK_SET_INSTALL_FILE"] expectedInstallFile := []string{ - strings.Replace(params.ImplicitOutputs[0].String(), android.OutSoongDir, result.Config.SoongOutDir(), 1), + strings.Replace(params.ImplicitOutputs[0].String(), android.TestOutSoongDir, result.Config.SoongOutDir(), 1), } if !reflect.DeepEqual(actualInstallFile, expectedInstallFile) { t.Errorf("Unexpected LOCAL_APK_SET_INSTALL_FILE value: '%s', expected: '%s',", From bd73d0db41ac84144c60e7719df106bda8efb663 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Fri, 26 Jul 2024 12:00:33 -0700 Subject: [PATCH 3/3] Move RelativeToTop out of basePath RelativeToTop is documented as returning the same concrete type that was passed in, which wasn't true for SourcePath. Implement it in SourcePath and delete it from basePath. Test: all soong tests pass Flag: EXEMPT refactor Change-Id: I5f2fd1237b964aa10565f7c6b34122dd6972fda9 --- android/paths.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/android/paths.go b/android/paths.go index 0661b7697..dad70f770 100644 --- a/android/paths.go +++ b/android/paths.go @@ -1119,11 +1119,6 @@ func (p basePath) withRel(rel string) basePath { return p } -func (p basePath) RelativeToTop() Path { - ensureTestOnly() - return p -} - // SourcePath is a Path representing a file path rooted from SrcDir type SourcePath struct { basePath @@ -1136,6 +1131,11 @@ func (p SourcePath) withRel(rel string) SourcePath { return p } +func (p SourcePath) RelativeToTop() Path { + ensureTestOnly() + return p +} + // safePathForSource is for paths that we expect are safe -- only for use by go // code that is embedding ninja variables in paths func safePathForSource(ctx PathContext, pathComponents ...string) (SourcePath, error) {