From f5245ce53daa2255cd39b7664379368044da80f6 Mon Sep 17 00:00:00 2001 From: Paul Duffin Date: Mon, 17 Jan 2022 16:36:52 +0000 Subject: [PATCH] Extract logic for building the snapshots to improve testability Previously, the produce_dist() method of SdkDistProducer was not testable because it attempted to invoke Soong to build the sdk snapshots. So, the TestPopulateDist tested the populate_dist() method instead. That was a problem because it meant that future changes to the produce_dist() method could not be tested, at least not without duplicating its functionality. This change extracts the logic for building the snapshots (i.e. the code that invokes soong to build the snapshot files) from the SdkDistProducer into a separate SnapshotBuilder class. That allows the test to substitute a FakeSnapshotBuilder that generates some fake sdk snapshot zip files and call produce_dist() instead of calling populate_dist() and populate_stubs(). It also renames the test to TestProduceDist to reflect that change. This is part of a larger refactoring to improve the testability of the mainline_modules_sdks.py file in preparation for adding support for building build release specific snapshots. Test: atest mainline_modules_sdks_test packages/modules/common/build/mainline_modules_sdks.sh tree out/dist/mainline-sdks out/dist/stubs - check that it is identical to before this change Bug: 204763318 Change-Id: Icf2e4b0200cc53863e45cf68208fbc8ec13c6f2c --- build/mainline_modules_sdks.py | 134 +++++++++++++++++----------- build/mainline_modules_sdks_test.py | 49 +++++++--- 2 files changed, 116 insertions(+), 67 deletions(-) diff --git a/build/mainline_modules_sdks.py b/build/mainline_modules_sdks.py index 9b2284f..c4ada92 100755 --- a/build/mainline_modules_sdks.py +++ b/build/mainline_modules_sdks.py @@ -195,6 +195,67 @@ class SubprocessRunner: *args, check=True, stdout=self.stdout, stderr=self.stderr, **kwargs) +@dataclasses.dataclass() +class SnapshotBuilder: + """Builds sdk snapshots""" + + # Used to run subprocesses for building snapshots. + subprocess_runner: SubprocessRunner + + # The OUT_DIR environment variable. + out_dir: str + + def get_mainline_sdks_path(self): + """Get the path to the Soong mainline-sdks directory""" + return os.path.join(self.out_dir, "soong/mainline-sdks") + + def get_sdk_path(self, sdk_name, sdk_version): + """Get the path to the sdk snapshot zip file produced by soong""" + return os.path.join(self.get_mainline_sdks_path(), + f"{sdk_name}-{sdk_version}.zip") + + def build_snapshots(self, sdk_versions, modules): + # Build the SDKs once for each version. + for sdk_version in sdk_versions: + # Compute the paths to all the Soong generated sdk snapshot files + # required by this script. + paths = [ + self.get_sdk_path(sdk, sdk_version) + for module in modules + for sdk in module.sdks + ] + + # TODO(ngeoffray): remove SOONG_ALLOW_MISSING_DEPENDENCIES, but we + # currently break without it. + # + # Set SOONG_SDK_SNAPSHOT_USE_SRCJAR to generate .srcjars inside sdk + # zip files as expected by prebuilt drop. + extraEnv = { + "SOONG_ALLOW_MISSING_DEPENDENCIES": "true", + "SOONG_SDK_SNAPSHOT_USE_SRCJAR": "true", + "SOONG_SDK_SNAPSHOT_VERSION": sdk_version, + } + # Unless explicitly specified in the calling environment set + # TARGET_BUILD_VARIANT=user. + # This MUST be identical to the TARGET_BUILD_VARIANT used to build + # the corresponding APEXes otherwise it could result in different + # hidden API flags, see http://b/202398851#comment29 for more info. + targetBuildVariant = os.environ.get("TARGET_BUILD_VARIANT", "user") + cmd = [ + "build/soong/soong_ui.bash", + "--make-mode", + "--soong-only", + f"TARGET_BUILD_VARIANT={targetBuildVariant}", + "TARGET_PRODUCT=mainline_sdk", + "MODULE_BUILD_FROM_SOURCE=true", + "out/soong/apex/depsinfo/new-allowed-deps.txt.check", + ] + paths + print_command(extraEnv, cmd) + env = os.environ.copy() + env.update(extraEnv) + self.subprocess_runner.run(cmd, env=env) + + @dataclasses.dataclass(frozen=True) class MainlineModule: """Represents a mainline module""" @@ -311,10 +372,10 @@ class SdkDistProducer: """ # Used to run subprocesses for this. - subprocess_runner: SubprocessRunner = SubprocessRunner() + subprocess_runner: SubprocessRunner - # The OUT_DIR environment variable. - out_dir: str = "uninitialized-out" + # Builds sdk snapshots + snapshot_builder: SnapshotBuilder # The DIST_DIR environment variable. dist_dir: str = "uninitialized-dist" @@ -323,60 +384,17 @@ class SdkDistProducer: # transformed to document where the changes came from. script: str = sys.argv[0] - def get_sdk_path(self, sdk_name, sdk_version): - """Get the path to the sdk snapshot zip file produced by soong""" - return os.path.join(self.out_dir, "soong/mainline-sdks", - f"{sdk_name}-{sdk_version}.zip") - def produce_dist(self, modules): sdk_versions = SDK_VERSIONS self.build_sdks(sdk_versions, modules) self.populate_dist(sdk_versions, modules) def build_sdks(self, sdk_versions, modules): - # Build the SDKs once for each version. - for sdk_version in sdk_versions: - # Compute the paths to all the Soong generated sdk snapshot files - # required by this script. - paths = [ - self.get_sdk_path(sdk, sdk_version) - for module in modules - for sdk in module.sdks - ] - - # TODO(ngeoffray): remove SOONG_ALLOW_MISSING_DEPENDENCIES, but we - # currently break without it. - # - # Set SOONG_SDK_SNAPSHOT_USE_SRCJAR to generate .srcjars inside sdk - # zip files as expected by prebuilt drop. - extraEnv = { - "SOONG_ALLOW_MISSING_DEPENDENCIES": "true", - "SOONG_SDK_SNAPSHOT_USE_SRCJAR": "true", - "SOONG_SDK_SNAPSHOT_VERSION": sdk_version, - } - # Unless explicitly specified in the calling environment set - # TARGET_BUILD_VARIANT=user. - # This MUST be identical to the TARGET_BUILD_VARIANT used to build - # the corresponding APEXes otherwise it could result in different - # hidden API flags, see http://b/202398851#comment29 for more info. - targetBuildVariant = os.environ.get("TARGET_BUILD_VARIANT", "user") - cmd = [ - "build/soong/soong_ui.bash", - "--make-mode", - "--soong-only", - f"TARGET_BUILD_VARIANT={targetBuildVariant}", - "TARGET_PRODUCT=mainline_sdk", - "MODULE_BUILD_FROM_SOURCE=true", - "out/soong/apex/depsinfo/new-allowed-deps.txt.check", - ] + paths - print_command(extraEnv, cmd) - env = os.environ.copy() - env.update(extraEnv) - self.subprocess_runner.run(cmd, env=env) + self.snapshot_builder.build_snapshots(sdk_versions, modules) def unzip_current_stubs(self, sdk_name, apex_name): """Unzips stubs for "current" into {producer.dist_dir}/stubs/{apex}.""" - sdk_path = self.get_sdk_path(sdk_name, "current") + sdk_path = self.snapshot_builder.get_sdk_path(sdk_name, "current") dest_dir = os.path.join(self.dist_dir, "stubs", apex_name) print( f"Extracting java_sdk_library files from {sdk_path} to {dest_dir}") @@ -416,7 +434,8 @@ class SdkDistProducer: sdk_dist_dir = os.path.join(sdks_dist_dir, sdk_version, apex, subdir) - sdk_path = self.get_sdk_path(sdk, sdk_version) + sdk_path = self.snapshot_builder.get_sdk_path( + sdk, sdk_version) self.dist_sdk_snapshot_zip(sdk_path, sdk_dist_dir, module.transformations()) @@ -508,11 +527,20 @@ def apply_transformations(producer, tmp_dir, transformations): def create_producer(): + # Variables initialized from environment variables that are set by the + # calling mainline_modules_sdks.sh. + out_dir = os.environ["OUT_DIR"] + dist_dir = os.environ["DIST_DIR"] + + subprocess_runner = SubprocessRunner() + snapshot_builder = SnapshotBuilder( + subprocess_runner=subprocess_runner, + out_dir=out_dir, + ) return SdkDistProducer( - # Variables initialized from environment variables that are set by the - # calling mainline_modules_sdks.sh. - out_dir=os.environ["OUT_DIR"], - dist_dir=os.environ["DIST_DIR"], + subprocess_runner=subprocess_runner, + snapshot_builder=snapshot_builder, + dist_dir=dist_dir, ) diff --git a/build/mainline_modules_sdks_test.py b/build/mainline_modules_sdks_test.py index 5760136..34e6b5a 100644 --- a/build/mainline_modules_sdks_test.py +++ b/build/mainline_modules_sdks_test.py @@ -14,7 +14,6 @@ # See the License for the specific language governing permissions and # limitations under the License. """Unit tests for mainline_modules_sdks.py.""" - from pathlib import Path import os import tempfile @@ -24,12 +23,15 @@ import zipfile import mainline_modules_sdks as mm -class TestPopulateDist(unittest.TestCase): +class FakeSnapshotBuilder(mm.SnapshotBuilder): + """A fake snapshot builder that does not run the build. - def create_snapshot_file(self, out_dir, name, version): - sdks_out_dir = Path(out_dir, "soong/mainline-sdks") - sdks_out_dir.mkdir(parents=True, exist_ok=True) - zip_file = Path(sdks_out_dir, f"{name}-{version}.zip") + This skips the whole build process and just creates some fake sdk + modules. + """ + + def create_snapshot_file(self, name, version): + zip_file = Path(self.get_sdk_path(name, version)) with zipfile.ZipFile(zip_file, "w") as z: z.writestr("Android.bp", "") if name.endswith("-sdk"): @@ -38,6 +40,19 @@ class TestPopulateDist(unittest.TestCase): z.writestr("sdk_library/public/lib.jar", "") z.writestr("sdk_library/public/api.txt", "") + def build_snapshots(self, sdk_versions, modules): + # Create input file structure. + sdks_out_dir = Path(self.get_mainline_sdks_path()) + sdks_out_dir.mkdir(parents=True, exist_ok=True) + # Create a fake sdk zip file for each module. + for module in modules: + for sdk in module.sdks: + for sdk_version in sdk_versions: + self.create_snapshot_file(sdk, sdk_version) + + +class TestProduceDist(unittest.TestCase): + def test(self): """Verify the dist/mainline-sdks directory is populated correctly""" with tempfile.TemporaryDirectory() as tmp_dir: @@ -51,18 +66,20 @@ class TestPopulateDist(unittest.TestCase): mm.MAINLINE_MODULES_BY_APEX["com.android.ipsec"], ] - # Create input file structure. - for module in modules: - for sdk in module.sdks: - self.create_snapshot_file(tmp_out_dir, sdk, "current") + subprocess_runner = mm.SubprocessRunner() + + snapshot_builder = FakeSnapshotBuilder( + subprocess_runner=subprocess_runner, + out_dir=tmp_out_dir, + ) producer = mm.SdkDistProducer( - out_dir=tmp_out_dir, + subprocess_runner=subprocess_runner, + snapshot_builder=snapshot_builder, dist_dir=tmp_dist_dir, ) - sdk_versions = ["current"] - producer.populate_dist(sdk_versions, modules) + producer.produce_dist(modules) files = [] for abs_dir, _, filenames in os.walk(tmp_dist_dir): @@ -114,7 +131,11 @@ def readTestData(relative_path): class TestSoongConfigBoilerplateInserter(unittest.TestCase): def apply_transformations(self, src, transformations, expected): - producer = mm.SdkDistProducer(script=self._testMethodName) + producer = mm.SdkDistProducer( + subprocess_runner=None, + snapshot_builder=None, + script=self._testMethodName, + ) with tempfile.TemporaryDirectory() as tmp_dir: path = os.path.join(tmp_dir, "Android.bp")