From 6cc53d9b518774319db5f2223f8634fa293b2399 Mon Sep 17 00:00:00 2001 From: Kevin Meinhardt Date: Mon, 27 Jan 2025 10:50:01 +0100 Subject: [PATCH] Migrate build scripts to python with single script to orchestrate during make up (#23011) * Migrate build scripts to python with single script to orchestrate during make up - Replaced the locale compilation script from a shell script to a Python script (compile_locales.py) for better error handling and parallel processing. - Updated the update_assets target in Makefile-docker to use the new update_assets.py script. - Removed the obsolete compile-mo.sh script. - Introduced sync_host_files.py to streamline dependency updates and asset synchronization. * TMP: add tests --- Dockerfile | 9 ++- Makefile-docker | 12 +--- Makefile-os | 8 +-- locale/compile-mo.sh | 37 ------------- scripts/compile_locales.py | 60 ++++++++++++++++++++ scripts/sync_host_files.py | 22 ++++++++ scripts/update_assets.py | 57 +++++++++++++++++++ src/olympia/lib/settings_base.py | 2 +- tests/make/test_compile_locales.py | 89 ++++++++++++++++++++++++++++++ tests/make/test_sync_host_files.py | 36 ++++++++++++ tests/make/test_update_assets.py | 76 +++++++++++++++++++++++++ 11 files changed, 354 insertions(+), 54 deletions(-) delete mode 100755 locale/compile-mo.sh create mode 100755 scripts/compile_locales.py create mode 100755 scripts/sync_host_files.py create mode 100755 scripts/update_assets.py create mode 100644 tests/make/test_compile_locales.py create mode 100644 tests/make/test_sync_host_files.py create mode 100644 tests/make/test_update_assets.py diff --git a/Dockerfile b/Dockerfile index 21dcad79a82c..bbf5967e18d6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -146,6 +146,9 @@ EOF FROM base AS development +# Copy build info from info +COPY --from=info ${BUILD_INFO} ${BUILD_INFO} + FROM base AS locales ARG LOCALE_DIR=${HOME}/locale # Compile locales @@ -155,7 +158,7 @@ COPY --chown=olympia:olympia locale ${LOCALE_DIR} RUN \ --mount=type=bind,source=requirements/locale.txt,target=${HOME}/requirements/locale.txt \ --mount=type=bind,source=Makefile-docker,target=${HOME}/Makefile-docker \ - --mount=type=bind,source=locale/compile-mo.sh,target=${HOME}/compile-mo.sh \ + --mount=type=bind,source=scripts/compile_locales.py,target=${HOME}/scripts/compile_locales.py \ make -f Makefile-docker compile_locales # More efficient caching by mounting the exact files we need @@ -175,10 +178,10 @@ COPY --chown=olympia:olympia static/ ${HOME}/static/ RUN \ --mount=type=bind,src=src,target=${HOME}/src \ --mount=type=bind,src=Makefile-docker,target=${HOME}/Makefile-docker \ + --mount=type=bind,src=scripts/update_assets.py,target=${HOME}/scripts/update_assets.py \ --mount=type=bind,src=manage.py,target=${HOME}/manage.py \ < settings_local.py -DJANGO_SETTINGS_MODULE="settings_local" make -f Makefile-docker update_assets +make -f Makefile-docker update_assets EOF FROM base AS production diff --git a/Makefile-docker b/Makefile-docker index 87c2ef8c6bc2..91ff05f2d683 100644 --- a/Makefile-docker +++ b/Makefile-docker @@ -72,14 +72,8 @@ data_load: ./manage.py data_load $(ARGS) .PHONY: update_assets -update_assets: - # Copy files required in compress_assets to the static folder - # If changing this here, make sure to adapt tests in amo/test_commands.py - $(PYTHON_COMMAND) manage.py compress_assets - $(PYTHON_COMMAND) manage.py generate_jsi18n_files - # Collect static files: This MUST be run last or files will be missing - $(PYTHON_COMMAND) manage.py collectstatic --noinput - +update_assets: ## Update the static assets + $(HOME)/scripts/update_assets.py .PHONY: update_deps update_deps: ## Update the dependencies @@ -218,7 +212,7 @@ extract_locales: ## extracts and merges translation strings .PHONE: compile_locales compile_locales: ## compiles translation strings $(PIP_COMMAND) install --progress-bar=off --no-deps -r requirements/locale.txt - ./locale/compile-mo.sh ./locale/ + $(HOME)/scripts/compile_locales.py .PHONY: help_submake help_submake: diff --git a/Makefile-os b/Makefile-os index 96c888fd6433..e4dc9c6f1bdd 100644 --- a/Makefile-os +++ b/Makefile-os @@ -161,17 +161,17 @@ docker_clean_build_cache: ## Remove buildx build cache .PHONY: clean_docker clean_docker: docker_compose_down docker_mysqld_volume_remove docker_clean_images docker_clean_volumes docker_clean_build_cache ## Remove all docker resources taking space on the host machine -.PHONY: docker_update_deps -docker_update_deps: docker_mysqld_volume_create ## Update the dependencies in the container based on the docker tag and target +.PHONY: docker_sync_host +docker_sync_host: docker_mysqld_volume_create ## Update the dependencies in the container based on the docker tag and target docker compose run \ --rm \ --no-deps \ $(DOCKER_RUN_ARGS) \ web \ - make update_deps + ./scripts/sync_host_files.py .PHONY: up_pre -up_pre: setup docker_pull_or_build docker_update_deps ## Pre-up the environment, setup files, volumes and host state +up_pre: setup docker_pull_or_build docker_sync_host ## Pre-up the environment, setup files, volumes and host state .PHONY: up_start up_start: docker_mysqld_volume_create ## Start the docker containers diff --git a/locale/compile-mo.sh b/locale/compile-mo.sh deleted file mode 100755 index d50a894a7f19..000000000000 --- a/locale/compile-mo.sh +++ /dev/null @@ -1,37 +0,0 @@ -#!/bin/bash -# syntax: -# compile-mo.sh locale-dir/ - -# Make this script fail if any command exits wit exit code != 0 -set -ue - -function process_po_file() { - pofile=$1 - dir=$(dirname "$pofile") - lang=$(echo "$pofile" | cut -d "/" -f2) - stem=$(basename "$pofile" .po) - touch "${dir}/${stem}.mo" - dennis-cmd lint --errorsonly "$pofile" && msgfmt -o "${dir}/${stem}.mo" "$pofile" -} - -# We are spawning sub processes with `xargs` -# and the function needs to be available in that sub process -export -f process_po_file - -function usage() { - echo "syntax:" - echo "compile-mo.sh locale-dir/" - exit 1 -} - -# check if file and dir are there -if [[ ($# -ne 1) || (! -d "$1") ]]; then usage; fi - -# Ensure dennis-cmd cli is available in the environment -hash dennis-cmd - -echo "compiling django.po..." -find $1 -type f -name "django.po" -print0 | xargs -0 -n1 -P4 bash -c 'process_po_file "$@"' _ - -echo "compiling djangojs.po..." -find $1 -type f -name "djangojs.po" -print0 | xargs -0 -n1 -P4 bash -c 'process_po_file "$@"' _ diff --git a/scripts/compile_locales.py b/scripts/compile_locales.py new file mode 100755 index 000000000000..23df4783915c --- /dev/null +++ b/scripts/compile_locales.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python3 + +import os +import subprocess +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + + +def process_po_file(pofile, attempt=1): + """Process a single .po file, creating corresponding .mo file.""" + pofile_path = Path(pofile) + print('processing', pofile_path.as_posix()) + mo_path = pofile_path.with_suffix('.mo') + + mo_path.touch() + + try: + # Run dennis-cmd lint + subprocess.run( + ['dennis-cmd', 'lint', '--errorsonly', pofile], + capture_output=True, + check=False, + ) + # If lint passes, run msgfmt + subprocess.run(['msgfmt', '-o', mo_path, pofile], check=True) + return + except subprocess.CalledProcessError as e: + if attempt < 3: + print(f'Failed attempt {attempt} for {pofile}, retrying...') + return process_po_file(pofile, attempt=attempt + 1) + raise e + + +def compile_locales(): + # Ensure 'dennis' is installed + import dennis as _dennis # type: ignore # noqa: F401 + + HOME = os.environ.get('HOME') + + locale_dir = Path(HOME) / 'locale' + + print(f'Compiling locales in {locale_dir}') + + # Collect all files first + django_files = [] + djangojs_files = [] + for root, _, files in locale_dir.walk(): + for file in files: + if file == 'django.po': + django_files.append(root / file) + elif file == 'djangojs.po': + djangojs_files.append(root / file) + + # Process django.po files in parallel + with ThreadPoolExecutor() as executor: + executor.map(process_po_file, django_files + djangojs_files) + + +if __name__ == '__main__': + compile_locales() diff --git a/scripts/sync_host_files.py b/scripts/sync_host_files.py new file mode 100755 index 000000000000..503600e20ab1 --- /dev/null +++ b/scripts/sync_host_files.py @@ -0,0 +1,22 @@ +#!/usr/bin/env python3 + +import json +import os +import subprocess + + +def sync_host_files(): + BUILD_INFO = os.environ.get('BUILD_INFO') + + subprocess.run(['make', 'update_deps'], check=True) + + with open(BUILD_INFO, 'r') as f: + build_info = json.load(f) + + if build_info.get('target') == 'production': + subprocess.run(['make', 'compile_locales'], check=True) + subprocess.run(['make', 'update_assets'], check=True) + + +if __name__ == '__main__': + sync_host_files() diff --git a/scripts/update_assets.py b/scripts/update_assets.py new file mode 100755 index 000000000000..8f6b6c8d157d --- /dev/null +++ b/scripts/update_assets.py @@ -0,0 +1,57 @@ +#!/usr/bin/env python3 + +import argparse +import os +import shutil +import subprocess +from pathlib import Path + + +def clean_static_dirs(verbose: bool = False): + HOME = os.environ.get('HOME') + STATIC_DIRS = ['static-build', 'site-static'] + + for directory in STATIC_DIRS: + path = Path(HOME) / directory + path.mkdir(parents=True, exist_ok=True) + for entry in path.iterdir(): + entry_path = entry.as_posix() + if verbose: + print(f'Removing {entry_path}') + if entry.is_dir(): + shutil.rmtree(entry_path) + else: + os.remove(entry_path) + + +def update_assets(verbose: bool = False): + clean_static_dirs(verbose) + + script_prefix = ['python3', 'manage.py'] + + environment = os.environ.copy() + # Always run in production mode without any development settings + environment['DJANGO_SETTINGS_MODULE'] = 'olympia.lib.settings_base' + + subprocess.run( + script_prefix + ['compress_assets'], + check=True, + env=environment, + ) + subprocess.run( + script_prefix + ['generate_jsi18n_files'], + check=True, + env=environment, + ) + subprocess.run( + script_prefix + ['collectstatic', '--noinput'], + check=True, + env=environment, + ) + + +if __name__ == '__main__': + parser = argparse.ArgumentParser() + parser.add_argument('--verbose', action='store_true') + args = parser.parse_args() + update_assets(args.verbose) diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index 4bd6faee9c2d..3c9d162bb6a2 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -1331,7 +1331,7 @@ def read_only_mode(env): NODE_PACKAGE_JSON = os.path.join('/', 'deps', 'package.json') NODE_PACKAGE_MANAGER_INSTALL_OPTIONS = ['--dry-run'] -STATIC_BUILD_PATH = os.path.join('/', 'data', 'olympia', 'static-build') +STATIC_BUILD_PATH = path('static-build') STATICFILES_DIRS = ( path('static'), diff --git a/tests/make/test_compile_locales.py b/tests/make/test_compile_locales.py new file mode 100644 index 000000000000..f86dc872a79e --- /dev/null +++ b/tests/make/test_compile_locales.py @@ -0,0 +1,89 @@ +import subprocess +import sys +import tempfile +from pathlib import Path +from unittest import TestCase, mock +from unittest.mock import Mock, patch + +import pytest + +from scripts.compile_locales import compile_locales, process_po_file +from tests import override_env + + +@pytest.mark.needs_locales_compilation +class TestCompileLocales(TestCase): + def setUp(self): + self.home_dir = Path(tempfile.mkdtemp()) + self.locale_dir = (self.home_dir / 'locale').mkdir() + + @patch.dict(sys.modules, {'dennis': None}) + def test_dennis_not_installed(self): + """Test that the script raises when dennis is not installed""" + self.assertRaises(ImportError, compile_locales) + + @patch.dict(sys.modules, {'dennis': Mock()}) + @patch('scripts.compile_locales.ThreadPoolExecutor') + def test_process_po_file(self, mock_executor): + """Test that the script processes po files""" + # Create po files + django_po = self.home_dir / 'locale' / 'django.po' + django_po.touch() + djangojs_po = self.home_dir / 'locale' / 'djangojs.po' + djangojs_po.touch() + + # Setup ThreadPoolExecutor mock + mock_executor_instance = Mock() + mock_executor.return_value.__enter__.return_value = mock_executor_instance + + with override_env(HOME=self.home_dir.as_posix()): + compile_locales() + + # Get the actual arguments passed to map + actual_args = mock_executor_instance.map.call_args[0] + self.assertEqual(actual_args[0], process_po_file) + self.assertEqual(list(actual_args[1]), [django_po, djangojs_po]) + + +class TestProcessPoFile(TestCase): + def setUp(self): + self.pofile = Path(tempfile.mkdtemp()) / 'django.po' + + mock_subprocess = patch('scripts.compile_locales.subprocess.run') + self.mock_subprocess = mock_subprocess.start() + self.addCleanup(mock_subprocess.stop) + + def test_process_po_file(self): + process_po_file(self.pofile.as_posix()) + self.assertTrue(self.pofile.with_suffix('.mo').exists()) + + assert self.mock_subprocess.call_args_list == [ + mock.call( + ['dennis-cmd', 'lint', '--errorsonly', self.pofile.as_posix()], + capture_output=True, + check=False, + ), + mock.call( + [ + 'msgfmt', + '-o', + self.pofile.with_suffix('.mo'), + self.pofile.as_posix(), + ], + check=True, + ), + ] + + def test_process_po_file_retries(self): + self.mock_subprocess.side_effect = subprocess.CalledProcessError( + returncode=1, + cmd=['dennis-cmd', 'lint', '--errorsonly', self.pofile.as_posix()], + ) + + with self.assertRaises(subprocess.CalledProcessError): + process_po_file(self.pofile.as_posix()) + + self.assertTrue(self.pofile.with_suffix('.mo').exists()) + + # We expect 3 attempts to process the file + self.assertEqual(self.mock_subprocess.call_count, 3) diff --git a/tests/make/test_sync_host_files.py b/tests/make/test_sync_host_files.py new file mode 100644 index 000000000000..66e8dd5e188a --- /dev/null +++ b/tests/make/test_sync_host_files.py @@ -0,0 +1,36 @@ +import json +import tempfile +from pathlib import Path +from unittest import TestCase, mock + +from scripts.sync_host_files import sync_host_files +from tests import override_env + + +@mock.patch('scripts.sync_host_files.subprocess.run') +class TestSyncHostFiles(TestCase): + def test_sync_host_files(self, mock_subprocess): + sync_host_files() + + mock_subprocess.assert_has_calls( + [ + mock.call(['make', 'update_deps'], check=True), + # mock.call(['make', 'compile_locales'], check=True), + # mock.call(['make', 'update_assets'], check=True), + ] + ) + + def test_sync_host_files_production(self, mock_subprocess): + mock_build = Path(tempfile.mktemp()) + mock_build.write_text(json.dumps({'target': 'production'})) + + with override_env(BUILD_INFO=mock_build.as_posix()): + sync_host_files() + + mock_subprocess.assert_has_calls( + [ + mock.call(['make', 'update_deps'], check=True), + mock.call(['make', 'compile_locales'], check=True), + mock.call(['make', 'update_assets'], check=True), + ] + ) diff --git a/tests/make/test_update_assets.py b/tests/make/test_update_assets.py new file mode 100644 index 000000000000..8a73ab3fb190 --- /dev/null +++ b/tests/make/test_update_assets.py @@ -0,0 +1,76 @@ +import tempfile +from pathlib import Path +from unittest import TestCase, mock + +from scripts.update_assets import clean_static_dirs, update_assets +from tests import override_env + + +class TestUpdateAssets(TestCase): + def setUp(self): + self.mocks = {} + for name in ['clean_static_dirs', 'subprocess.run']: + patch = mock.patch(f'scripts.update_assets.{name}') + self.mocks[name] = patch.start() + self.addCleanup(patch.stop) + + def test_update_assets(self): + update_assets() + + assert self.mocks['clean_static_dirs'].call_count == 1 + + assert self.mocks['subprocess.run'].call_args_list == [ + mock.call( + ['python3', 'manage.py', 'compress_assets'], check=True, env=mock.ANY + ), + mock.call( + ['python3', 'manage.py', 'generate_jsi18n_files'], + check=True, + env=mock.ANY, + ), + mock.call( + ['python3', 'manage.py', 'collectstatic', '--noinput'], + check=True, + env=mock.ANY, + ), + ] + + for call in self.mocks['subprocess.run'].call_args_list: + assert ( + call.kwargs['env']['DJANGO_SETTINGS_MODULE'] + == 'olympia.lib.settings_base' + ) + + def test_update_assets_with_verbose(self): + update_assets(verbose=True) + + assert self.mocks['clean_static_dirs'].call_args_list == [ + mock.call(True), + ] + + +class TestCleanStaticDirs(TestCase): + def setUp(self): + self.home = Path(tempfile.mkdtemp()) + + def _run_clean_static_dirs(self, verbose=False): + with override_env(HOME=self.home.as_posix()): + clean_static_dirs(verbose=verbose) + + def test_creates_dirs(self): + self._run_clean_static_dirs() + + assert self.home.joinpath('static-build').exists() + assert self.home.joinpath('site-static').exists() + + def test_empties_dirs(self): + self.home.joinpath('static-build').mkdir() + (self.home / 'static-build' / 'test.txt').touch() + + self.home.joinpath('site-static').mkdir() + (self.home / 'site-static' / 'test.txt').touch() + + self._run_clean_static_dirs() + + assert not (self.home / 'static-build' / 'test.txt').exists() + assert not (self.home / 'site-static' / 'test.txt').exists()