From 3b806d3b88ea9c5573b53ac03ecdae3b6d89c542 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Mon, 11 Mar 2024 15:15:03 -0700 Subject: [PATCH 1/2] Don't use zip files when creating filesystems The end result is a directory that's passed to build_image, so zipping and then unzipping image contents will unnecessarily slow things down. Bug: 329146343 Test: m microdroid --no-skip-soong-tests Change-Id: I98223c60e8144d6c707832fcc03ba8fe94467e7b --- apex/apex_test.go | 2 +- filesystem/filesystem.go | 100 +++++++++++----------------------- filesystem/filesystem_test.go | 4 +- 3 files changed, 36 insertions(+), 70 deletions(-) diff --git a/apex/apex_test.go b/apex/apex_test.go index 19b9d169f..add6083c9 100644 --- a/apex/apex_test.go +++ b/apex/apex_test.go @@ -11022,7 +11022,7 @@ func TestFileSystemShouldSkipApexLibraries(t *testing.T) { } `) - inputs := result.ModuleForTests("myfilesystem", "android_common").Output("deps.zip").Implicits + inputs := result.ModuleForTests("myfilesystem", "android_common").Output("myfilesystem.img").Implicits android.AssertStringListDoesNotContain(t, "filesystem should not have libbar", inputs.Strings(), "out/soong/.intermediates/libbar/android_arm64_armv8-a_shared/libbar.so") diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 795a0aa02..abc656f49 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -183,13 +183,9 @@ func (f *filesystem) GenerateAndroidBuildActions(ctx android.ModuleContext) { ctx.InstallFile(f.installDir, f.installFileName(), f.output) } -// root zip will contain extra files/dirs that are not from the `deps` property. -func (f *filesystem) buildRootZip(ctx android.ModuleContext) android.OutputPath { - rootDir := android.PathForModuleGen(ctx, "root").OutputPath - builder := android.NewRuleBuilder(pctx, ctx) - builder.Command().Text("rm -rf").Text(rootDir.String()) - builder.Command().Text("mkdir -p").Text(rootDir.String()) - +// Copy extra files/dirs that are not from the `deps` property to `rootDir`, checking for conflicts with files +// already in `rootDir`. +func (f *filesystem) buildNonDepsFiles(ctx android.ModuleContext, builder *android.RuleBuilder, rootDir android.OutputPath) { // create dirs and symlinks for _, dir := range f.properties.Dirs { // OutputPath.Join verifies dir @@ -212,7 +208,7 @@ func (f *filesystem) buildRootZip(ctx android.ModuleContext) android.OutputPath // OutputPath.Join verifies name. don't need to verify target. dst := rootDir.Join(ctx, name) - + builder.Command().Textf("(! [ -e %s -o -L %s ] || (echo \"%s already exists from an earlier stage of the build\" && exit 1))", dst, dst, dst) builder.Command().Text("mkdir -p").Text(filepath.Dir(dst.String())) builder.Command().Text("ln -sf").Text(proptools.ShellEscape(target)).Text(dst.String()) } @@ -222,55 +218,33 @@ func (f *filesystem) buildRootZip(ctx android.ModuleContext) android.OutputPath var extraFiles android.OutputPaths if f.buildExtraFiles != nil { extraFiles = f.buildExtraFiles(ctx, rootForExtraFiles) - for _, f := range extraFiles { - rel, _ := filepath.Rel(rootForExtraFiles.String(), f.String()) - if strings.HasPrefix(rel, "..") { - panic(fmt.Errorf("%q is not under %q\n", f, rootForExtraFiles)) - } - } } - // Zip them all - zipOut := android.PathForModuleGen(ctx, "root.zip").OutputPath - zipCommand := builder.Command().BuiltTool("soong_zip") - zipCommand.FlagWithOutput("-o ", zipOut). - FlagWithArg("-C ", rootDir.String()). - Flag("-L 0"). // no compression because this will be unzipped soon - FlagWithArg("-D ", rootDir.String()). - Flag("-d") // include empty directories - if len(extraFiles) > 0 { - zipCommand.FlagWithArg("-C ", rootForExtraFiles.String()) - for _, f := range extraFiles { - zipCommand.FlagWithInput("-f ", f) + for _, f := range extraFiles { + rel, err := filepath.Rel(rootForExtraFiles.String(), f.String()) + if err != nil || strings.HasPrefix(rel, "..") { + ctx.ModuleErrorf("can't make %q relative to %q", f, rootForExtraFiles) + continue } + dst := rootDir.Join(ctx, rel) + builder.Command().Textf("(! [ -e %s -o -L %s ] || (echo \"%s already exists from an earlier stage of the build\" && exit 1))", dst, dst, dst) + builder.Command().Text("mkdir -p").Text(filepath.Dir(dst.String())) + builder.Command().Text("cp -P").Input(f).Output(dst) } - - builder.Command().Text("rm -rf").Text(rootDir.String()) - - builder.Build("zip_root", fmt.Sprintf("zipping root contents for %s", ctx.ModuleName())) - return zipOut } func (f *filesystem) buildImageUsingBuildImage(ctx android.ModuleContext) android.OutputPath { - depsZipFile := android.PathForModuleOut(ctx, "deps.zip").OutputPath - f.entries = f.CopyDepsToZip(ctx, f.gatherFilteredPackagingSpecs(ctx), depsZipFile) - - builder := android.NewRuleBuilder(pctx, ctx) - depsBase := proptools.StringDefault(f.properties.Base_dir, ".") - rebasedDepsZip := android.PathForModuleOut(ctx, "rebased_deps.zip").OutputPath - builder.Command(). - BuiltTool("zip2zip"). - FlagWithInput("-i ", depsZipFile). - FlagWithOutput("-o ", rebasedDepsZip). - Text("**/*:" + proptools.ShellEscape(depsBase)) // zip2zip verifies depsBase - rootDir := android.PathForModuleOut(ctx, "root").OutputPath - rootZip := f.buildRootZip(ctx) - builder.Command(). - BuiltTool("zipsync"). - FlagWithArg("-d ", rootDir.String()). // zipsync wipes this. No need to clear. - Input(rootZip). - Input(rebasedDepsZip) + rebasedDir := rootDir + if f.properties.Base_dir != nil { + rebasedDir = rootDir.Join(ctx, *f.properties.Base_dir) + } + builder := android.NewRuleBuilder(pctx, ctx) + // Wipe the root dir to get rid of leftover files from prior builds + builder.Command().Textf("rm -rf %s && mkdir -p %s", rootDir, rootDir) + f.entries = f.CopySpecsToDir(ctx, builder, f.gatherFilteredPackagingSpecs(ctx), rebasedDir) + + f.buildNonDepsFiles(ctx, builder, rootDir) // run host_init_verifier // Ideally we should have a concept of pluggable linters that verify the generated image. @@ -388,25 +362,17 @@ func (f *filesystem) buildCpioImage(ctx android.ModuleContext, compressed bool) ctx.PropertyErrorf("file_contexts", "file_contexts is not supported for compressed cpio image.") } - depsZipFile := android.PathForModuleOut(ctx, "deps.zip").OutputPath - f.entries = f.CopyDepsToZip(ctx, f.gatherFilteredPackagingSpecs(ctx), depsZipFile) - - builder := android.NewRuleBuilder(pctx, ctx) - depsBase := proptools.StringDefault(f.properties.Base_dir, ".") - rebasedDepsZip := android.PathForModuleOut(ctx, "rebased_deps.zip").OutputPath - builder.Command(). - BuiltTool("zip2zip"). - FlagWithInput("-i ", depsZipFile). - FlagWithOutput("-o ", rebasedDepsZip). - Text("**/*:" + proptools.ShellEscape(depsBase)) // zip2zip verifies depsBase - rootDir := android.PathForModuleOut(ctx, "root").OutputPath - rootZip := f.buildRootZip(ctx) - builder.Command(). - BuiltTool("zipsync"). - FlagWithArg("-d ", rootDir.String()). // zipsync wipes this. No need to clear. - Input(rootZip). - Input(rebasedDepsZip) + rebasedDir := rootDir + if f.properties.Base_dir != nil { + rebasedDir = rootDir.Join(ctx, *f.properties.Base_dir) + } + builder := android.NewRuleBuilder(pctx, ctx) + // Wipe the root dir to get rid of leftover files from prior builds + builder.Command().Textf("rm -rf %s && mkdir -p %s", rootDir, rootDir) + f.entries = f.CopySpecsToDir(ctx, builder, f.gatherFilteredPackagingSpecs(ctx), rebasedDir) + + f.buildNonDepsFiles(ctx, builder, rootDir) output := android.PathForModuleOut(ctx, f.installFileName()).OutputPath cmd := builder.Command(). diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index c44810517..f09ed0838 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -270,7 +270,7 @@ func TestFileSystemShouldInstallCoreVariantIfTargetBuildAppsIsSet(t *testing.T) } `) - inputs := result.ModuleForTests("myfilesystem", "android_common").Output("deps.zip").Implicits + inputs := result.ModuleForTests("myfilesystem", "android_common").Output("myfilesystem.img").Implicits android.AssertStringListContains(t, "filesystem should have libbar even for unbundled build", inputs.Strings(), "out/soong/.intermediates/libbar/android_arm64_armv8-a_shared/libbar.so") @@ -314,7 +314,7 @@ func TestFileSystemWithCoverageVariants(t *testing.T) { `) filesystem := result.ModuleForTests("myfilesystem", "android_common_cov") - inputs := filesystem.Output("deps.zip").Implicits + inputs := filesystem.Output("myfilesystem.img").Implicits android.AssertStringListContains(t, "filesystem should have libfoo(cov)", inputs.Strings(), "out/soong/.intermediates/libfoo/android_arm64_armv8-a_shared_cov/libfoo.so") From 4a2a7c98f6e3a335934b0f3bc7e23ea271f89324 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Tue, 12 Mar 2024 12:44:40 -0700 Subject: [PATCH 2/2] Add include_make_built_files If `include_make_built_files` is set to the name of a partition, the make-built files from that partition will be incorperated into this soong module. This is to ease the transition to soong built filesystems. If any files are present in both the soong-built file list and the make-built one, the soong ones will be preferred. Bug: 329146343 Test: go test Change-Id: I456b283e1189116e699ed75357cc056f5d217688 --- filesystem/filesystem.go | 74 ++++++++++++++++++++++++++++------- filesystem/filesystem_test.go | 17 ++++++++ scripts/Android.bp | 13 ++++++ scripts/merge_directories.py | 60 ++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 14 deletions(-) create mode 100755 scripts/merge_directories.py diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index abc656f49..64a2e235f 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -19,6 +19,7 @@ import ( "fmt" "io" "path/filepath" + "slices" "strings" "android/soong/android" @@ -109,6 +110,12 @@ type filesystemProperties struct { // Mount point for this image. Default is "/" Mount_point *string + + // If set to the name of a partition ("system", "vendor", etc), this filesystem module + // will also include the contents of the make-built staging directories. If any soong + // modules would be installed to the same location as a make module, they will overwrite + // the make version. + Include_make_built_files string } // android_filesystem packages a set of modules and their transitive dependencies into a filesystem @@ -214,22 +221,21 @@ func (f *filesystem) buildNonDepsFiles(ctx android.ModuleContext, builder *andro } // create extra files if there's any - rootForExtraFiles := android.PathForModuleGen(ctx, "root-extra").OutputPath - var extraFiles android.OutputPaths if f.buildExtraFiles != nil { - extraFiles = f.buildExtraFiles(ctx, rootForExtraFiles) - } - - for _, f := range extraFiles { - rel, err := filepath.Rel(rootForExtraFiles.String(), f.String()) - if err != nil || strings.HasPrefix(rel, "..") { - ctx.ModuleErrorf("can't make %q relative to %q", f, rootForExtraFiles) - continue + rootForExtraFiles := android.PathForModuleGen(ctx, "root-extra").OutputPath + extraFiles := f.buildExtraFiles(ctx, rootForExtraFiles) + for _, f := range extraFiles { + rel, err := filepath.Rel(rootForExtraFiles.String(), f.String()) + if err != nil || strings.HasPrefix(rel, "..") { + ctx.ModuleErrorf("can't make %q relative to %q", f, rootForExtraFiles) + } + } + if len(extraFiles) > 0 { + builder.Command().BuiltTool("merge_directories"). + Implicits(extraFiles.Paths()). + Text(rootDir.String()). + Text(rootForExtraFiles.String()) } - dst := rootDir.Join(ctx, rel) - builder.Command().Textf("(! [ -e %s -o -L %s ] || (echo \"%s already exists from an earlier stage of the build\" && exit 1))", dst, dst, dst) - builder.Command().Text("mkdir -p").Text(filepath.Dir(dst.String())) - builder.Command().Text("cp -P").Input(f).Output(dst) } } @@ -245,6 +251,7 @@ func (f *filesystem) buildImageUsingBuildImage(ctx android.ModuleContext) androi f.entries = f.CopySpecsToDir(ctx, builder, f.gatherFilteredPackagingSpecs(ctx), rebasedDir) f.buildNonDepsFiles(ctx, builder, rootDir) + f.addMakeBuiltFiles(ctx, builder, rootDir) // run host_init_verifier // Ideally we should have a concept of pluggable linters that verify the generated image. @@ -362,6 +369,10 @@ func (f *filesystem) buildCpioImage(ctx android.ModuleContext, compressed bool) ctx.PropertyErrorf("file_contexts", "file_contexts is not supported for compressed cpio image.") } + if f.properties.Include_make_built_files != "" { + ctx.PropertyErrorf("include_make_built_files", "include_make_built_files is not supported for compressed cpio image.") + } + rootDir := android.PathForModuleOut(ctx, "root").OutputPath rebasedDir := rootDir if f.properties.Base_dir != nil { @@ -395,6 +406,41 @@ func (f *filesystem) buildCpioImage(ctx android.ModuleContext, compressed bool) return output } +var validPartitions = []string{ + "system", + "userdata", + "cache", + "system_other", + "vendor", + "product", + "system_ext", + "odm", + "vendor_dlkm", + "odm_dlkm", + "system_dlkm", +} + +func (f *filesystem) addMakeBuiltFiles(ctx android.ModuleContext, builder *android.RuleBuilder, rootDir android.Path) { + partition := f.properties.Include_make_built_files + if partition == "" { + return + } + if !slices.Contains(validPartitions, partition) { + ctx.PropertyErrorf("include_make_built_files", "Expected one of %#v, found %q", validPartitions, partition) + return + } + stampFile := fmt.Sprintf("target/product/%s/obj/PACKAGING/%s_intermediates/staging_dir.stamp", ctx.Config().DeviceName(), partition) + fileListFile := fmt.Sprintf("target/product/%s/obj/PACKAGING/%s_intermediates/file_list.txt", ctx.Config().DeviceName(), partition) + stagingDir := fmt.Sprintf("target/product/%s/%s", ctx.Config().DeviceName(), partition) + + builder.Command().BuiltTool("merge_directories"). + Implicit(android.PathForArbitraryOutput(ctx, stampFile)). + Text("--ignore-duplicates"). + FlagWithInput("--file-list", android.PathForArbitraryOutput(ctx, fileListFile)). + Text(rootDir.String()). + Text(android.PathForArbitraryOutput(ctx, stagingDir).String()) +} + var _ android.AndroidMkEntriesProvider = (*filesystem)(nil) // Implements android.AndroidMkEntriesProvider diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index f09ed0838..6d62746ec 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -16,6 +16,7 @@ package filesystem import ( "os" + "path/filepath" "testing" "android/soong/android" @@ -93,6 +94,22 @@ func TestFileSystemDeps(t *testing.T) { } } +func TestIncludeMakeBuiltFiles(t *testing.T) { + result := fixture.RunTestWithBp(t, ` + android_filesystem { + name: "myfilesystem", + include_make_built_files: "system", + } + `) + + 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") + 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) +} + func TestFileSystemFillsLinkerConfigWithStubLibs(t *testing.T) { result := fixture.RunTestWithBp(t, ` android_system_image { diff --git a/scripts/Android.bp b/scripts/Android.bp index e2fd59f17..f36329b1d 100644 --- a/scripts/Android.bp +++ b/scripts/Android.bp @@ -292,3 +292,16 @@ sh_binary_host { name: "keep-flagged-apis", src: "keep-flagged-apis.sh", } + +python_binary_host { + name: "merge_directories", + main: "merge_directories.py", + srcs: [ + "merge_directories.py", + ], + version: { + py3: { + embedded_launcher: true, + }, + }, +} diff --git a/scripts/merge_directories.py b/scripts/merge_directories.py new file mode 100755 index 000000000..3f8631bab --- /dev/null +++ b/scripts/merge_directories.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 +import argparse +import os +import shutil +import sys + +def main(): + parser = argparse.ArgumentParser( + description="Given a list of directories, this script will copy the contents of all of " + "them into the first directory, erroring out if any duplicate files are found." + ) + parser.add_argument( + "--ignore-duplicates", + action="store_true", + help="Don't error out on duplicate files, just skip them. The file from the earliest " + "directory listed on the command line will be the winner." + ) + parser.add_argument( + "--file-list", + help="Path to a text file containing paths relative to in_dir. Only these paths will be " + "copied out of in_dir." + ) + parser.add_argument("out_dir") + parser.add_argument("in_dir") + args = parser.parse_args() + + if not os.path.isdir(args.out_dir): + sys.exit(f"error: {args.out_dir} must be a directory") + if not os.path.isdir(args.in_dir): + sys.exit(f"error: {args.in_dir} must be a directory") + + file_list = None + if args.file_list: + with open(file_list_file, "r") as f: + file_list = f.read().strip().splitlines() + + in_dir = args.in_dir + for root, dirs, files in os.walk(in_dir): + rel_root = os.path.relpath(root, in_dir) + dst_root = os.path.join(args.out_dir, rel_root) + made_parent_dirs = False + for f in files: + src = os.path.join(root, f) + dst = os.path.join(dst_root, f) + p = os.path.normpath(os.path.join(rel_root, f)) + if file_list is not None and p not in file_list: + continue + if os.path.lexists(dst): + if args.ignore_duplicates: + continue + sys.exit(f"error: {p} exists in both {args.out_dir} and {in_dir}") + + if not made_parent_dirs: + os.makedirs(dst_root, exist_ok=True) + made_parent_dirs = True + + shutil.copy2(src, dst, follow_symlinks=False) + +if __name__ == "__main__": + main()