From c6eb081243dd017211d8c7de5b82be7a9ecd4b9c Mon Sep 17 00:00:00 2001 From: Oscar Vestlie Date: Thu, 13 Feb 2025 15:33:49 -0800 Subject: [PATCH] Package dependencies from source dir for android (#4855) Previously these dependencies were ignored. Many test targets depend on test data from the source dir. b/354062546 --- .../actions/upload_test_artifacts/action.yaml | 5 +- cobalt/build/archive_test_artifacts.py | 140 +++++++++++------- .../android-arm/sql_unittests_filter.json | 5 - 3 files changed, 87 insertions(+), 63 deletions(-) delete mode 100644 cobalt/testing/filters/android-arm/sql_unittests_filter.json diff --git a/.github/actions/upload_test_artifacts/action.yaml b/.github/actions/upload_test_artifacts/action.yaml index 8469171cd00..6ea5c3cd7fa 100644 --- a/.github/actions/upload_test_artifacts/action.yaml +++ b/.github/actions/upload_test_artifacts/action.yaml @@ -25,8 +25,9 @@ runs: # Put test targets json file in the out folder for the archiving script to pick up. cp "${{ inputs.test_targets_json_file }}" out/${{ matrix.platform }}_${{ matrix.config }}/ - time ./cobalt/build/archive_test_artifacts.py \ - --source out/${{ matrix.platform }}_${{ matrix.config }}/ \ + time vpython3 -u ./cobalt/build/archive_test_artifacts.py \ + --source-dir ${GITHUB_WORKSPACE}/src \ + --out-dir out/${{ matrix.platform }}_${{ matrix.config }}/ \ --destination ${GITHUB_WORKSPACE}/artifacts \ --platform ${{ matrix.platform }} \ --targets $(cat "${{ inputs.test_targets_json_file }}" | jq -cr '.test_targets | join(",")') diff --git a/cobalt/build/archive_test_artifacts.py b/cobalt/build/archive_test_artifacts.py index 391f8065a7f..f66b1280367 100755 --- a/cobalt/build/archive_test_artifacts.py +++ b/cobalt/build/archive_test_artifacts.py @@ -19,99 +19,124 @@ import shutil import subprocess import tempfile -from typing import List, Optional +from typing import List, Tuple # Path prefixes that contain files we don't need to run tests. _EXCLUDE_DIRS = [ - '../../', + 'obj/', 'lib.java/', './exe.unstripped/', './lib.unstripped/', + '../../third_party/jdk/', ] -def _make_tar(archive_path: str, - file_list: str, - base_path: Optional[str] = None): +def _make_tar(archive_path: str, file_lists: List[Tuple[str, str]]): """Creates the tar file. Uses tar command instead of tarfile for performance. """ print(f'Creating {os.path.basename(archive_path)}') - # Create temporary file to hold file list to not blow out the commandline. - with tempfile.NamedTemporaryFile(mode='w', encoding='utf-8') as temp_file: - temp_file.write('\n'.join(sorted(file_list))) - temp_file.flush() - base_path_arg = ['-C', base_path] if base_path else [] - tar_cmd = [ - 'tar', '-I gzip -1', '-cvf', archive_path, *base_path_arg, '-T', - temp_file.name - ] - subprocess.check_call(tar_cmd) - - -def create_archive(targets: List[str], source_dir: str, destination_dir: str, - platform: str, uber_archive: bool): + tar_cmd = ['tar', '-I gzip -1', '-cvf', archive_path] + tmp_files = [] + for file_list, base_dir in file_lists: + # Create temporary file to hold file list to not blow out the commandline. + # It will get cleaned up via implicit close. + # pylint: disable=consider-using-with + tmp_file = tempfile.NamedTemporaryFile(mode='w', encoding='utf-8') + tmp_files.append(tmp_file) + tmp_file.write('\n'.join(sorted(file_list))) + tmp_file.flush() + base_dir_arg = ['-C', base_dir] if base_dir else [] + tar_cmd += [*base_dir_arg, '-T', tmp_file.name] + + print(f'Running `{" ".join(tar_cmd)}`') # pylint: disable=inconsistent-quotes + subprocess.check_call(tar_cmd) + + +def create_archive( + *, + targets: List[str], + source_dir: str, + out_dir: str, + destination_dir: str, + platform: str, +): """Main logic. Collects runtime dependencies from the source directory for each target.""" - tar_root = '.' if platform.startswith('android') else source_dir - deps = set() + # TODO(oxv): Move logic behind parameters instead of using platform. + is_linux = platform.startswith('linux') + is_android = platform.startswith('android') + + tar_root = '.' if is_android else out_dir + combined_deps = set() # TODO(oxv): Make output from build step instead. # Add test_targets.json to archive so that test runners know what to run. - deps.add(os.path.relpath(os.path.join(tar_root, 'test_targets.json'))) + combined_deps.add( + os.path.relpath(os.path.join(tar_root, 'test_targets.json'))) for target in targets: target_path, target_name = target.split(':') # Paths are configured in test.gni: # https://github.com/youtube/cobalt/blob/main/testing/test.gni - if platform.startswith('android'): + if is_android: deps_file = os.path.join( - source_dir, 'gen.runtime', target_path, + out_dir, 'gen.runtime', target_path, f'{target_name}__test_runner_script.runtime_deps') else: - deps_file = os.path.join(source_dir, f'{target_name}.runtime_deps') + deps_file = os.path.join(out_dir, f'{target_name}.runtime_deps') with open(deps_file, 'r', encoding='utf-8') as runtime_deps_file: # The paths in the runtime_deps files are relative to the out folder. # Android tests expects files to be relative to the out folder in the # archive whereas Linux tests expect it relative to the source root. - # TODO(oxv): Pass as argument? - target_deps = { - os.path.relpath(os.path.join(tar_root, line.strip())) - for line in runtime_deps_file - if not (platform.startswith('android') and any( - line.startswith(path) for path in _EXCLUDE_DIRS)) - } - deps |= target_deps - - if not uber_archive: - output_path = os.path.join(destination_dir, f'{target_name}_deps.tar.gz') - base_path = source_dir if platform.startswith('android') else None - _make_tar(output_path, deps, base_path) - deps = set([os.path.relpath(os.path.join(tar_root, 'test_targets.json'))]) - - if uber_archive: + # Files that are to be picked up from the source root should be + # include in the archive without the prefix. + target_deps = set() + target_src_root_deps = set() + for line in runtime_deps_file: + if any(line.startswith(path) for path in _EXCLUDE_DIRS): + continue + + if is_android and line.startswith('../../'): + target_src_root_deps.add(line[6:]) + else: + rel_path = os.path.relpath(os.path.join(tar_root, line.strip())) + target_deps.add(rel_path) + combined_deps |= target_deps + + # Android tests and deps are bundled into one tar file per target. + if is_android: + output_path = os.path.join(destination_dir, + f'{target_name}_deps.tar.gz') + _make_tar(output_path, [(combined_deps, out_dir), + (target_src_root_deps, source_dir)]) + combined_deps = set( + [os.path.relpath(os.path.join(tar_root, 'test_targets.json'))]) + + # Linux tests and deps are all bundled into a single tar file. + if is_linux: output_path = os.path.join(destination_dir, 'test_artifacts.tar.gz') - _make_tar(output_path, deps) + _make_tar(output_path, [(combined_deps, source_dir)]) -def copy_apks(targets: List[str], source_dir: str, destination_dir: str): - """Copies the target APKs from the source directory to the destination. - The path to the APK in the source directory (assumed here to be the out - directory) is defined in build/config/android/rules.gni - """ +def copy_apks(targets: List[str], out_dir: str, destination_dir: str): + """Copies the target APKs from the out directory to the destination.""" for target in targets: _, target_name = target.split(':') - apk_path = f'{source_dir}/{target_name}_apk/{target_name}-debug.apk' + # The path to the APK in the out directory is defined in + # build/config/android/rules.gni. + apk_path = f'{out_dir}/{target_name}_apk/{target_name}-debug.apk' shutil.copy2(apk_path, destination_dir) def main(): parser = argparse.ArgumentParser() parser.add_argument( - '-s', - '--source-dir', + '-s', '--source-dir', required=True, help='The path to the source root.') + parser.add_argument( + '-o', + '--out-dir', required=True, - help='The path to where the artifacts are stored. ' - 'Typically the out folder.') + help='The path to where the build artifacts are stored.') parser.add_argument( '-d', '--destination-dir', @@ -130,12 +155,15 @@ def main(): 'e.g. path/to:target_name,other/path/to:target_name.') args = parser.parse_args() - uber_archive = args.platform.startswith('linux') - create_archive(args.targets, args.source_dir, args.destination_dir, - args.platform, uber_archive) + create_archive( + targets=args.targets, + source_dir=args.source_dir, + out_dir=args.out_dir, + destination_dir=args.destination_dir, + platform=args.platform) if args.platform.startswith('android'): - copy_apks(args.targets, args.source_dir, args.destination_dir) + copy_apks(args.targets, args.out_dir, args.destination_dir) if __name__ == '__main__': diff --git a/cobalt/testing/filters/android-arm/sql_unittests_filter.json b/cobalt/testing/filters/android-arm/sql_unittests_filter.json deleted file mode 100644 index b5fefefe17e..00000000000 --- a/cobalt/testing/filters/android-arm/sql_unittests_filter.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "failing_tests": [ - "SQLRecoveryTest.Bug387868" - ] -}