From 56c69ea9cf389e3002c352e041abee3706ea0816 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 6 May 2024 14:50:17 -0400 Subject: [PATCH 1/2] revert: revert: build: finish replacing paver assets This reverts commit 4c0284b87dbca6363040a18fd5fa67297df85afa. --- cms/envs/common.py | 37 +- .../0017-reimplement-asset-processing.rst | 52 +-- lms/envs/common.py | 19 +- .../management/commands/compile_sass.py | 124 +++--- .../djangoapps/theming/tests/test_commands.py | 56 +-- pavelib/assets.py | 321 +++++++-------- pavelib/paver_tests/test_assets.py | 387 +++++------------- scripts/compile_sass.py | 8 +- scripts/watch_sass.sh | 4 +- 9 files changed, 375 insertions(+), 633 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 39d5c7d13251..6aa2dfb835b1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -39,6 +39,7 @@ # pylint: disable=unused-import, useless-suppression, wrong-import-order, wrong-import-position import importlib.util +import json import os import sys @@ -1275,14 +1276,11 @@ # Static content STATIC_URL = '/static/studio/' -STATIC_ROOT = ENV_ROOT / "staticfiles" / 'studio' +STATIC_ROOT = os.environ.get('STATIC_ROOT_CMS', ENV_ROOT / 'staticfiles' / 'studio') STATICFILES_DIRS = [ COMMON_ROOT / "static", PROJECT_ROOT / "static", - - # This is how you would use the textbook images locally - # ("book", ENV_ROOT / "book_images"), ] # Locale/Internationalization @@ -1322,11 +1320,16 @@ EMBARGO_SITE_REDIRECT_URL = None ##### custom vendor plugin variables ##### -# JavaScript code can access this data using `process.env.JS_ENV_EXTRA_CONFIG` -# One of the current use cases for this is enabling custom TinyMCE plugins -# (TINYMCE_ADDITIONAL_PLUGINS) and overriding the TinyMCE configuration -# (TINYMCE_CONFIG_OVERRIDES). -JS_ENV_EXTRA_CONFIG = {} + +# .. setting_name: JS_ENV_EXTRA_CONFIG +# .. setting_default: {} +# .. setting_description: JavaScript code can access this dictionary using `process.env.JS_ENV_EXTRA_CONFIG` +# One of the current use cases for this is enabling custom TinyMCE plugins +# (TINYMCE_ADDITIONAL_PLUGINS) and overriding the TinyMCE configuration (TINYMCE_CONFIG_OVERRIDES). +# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer +# use Django settings. Please set the JS_ENV_EXTRA_CONFIG environment variable to an equivalent JSON +# string instead. For details, see: https://github.com/openedx/edx-platform/issues/31895 +JS_ENV_EXTRA_CONFIG = json.loads(os.environ.get('JS_ENV_EXTRA_CONFIG', '{}')) ############################### PIPELINE ####################################### @@ -1503,7 +1506,14 @@ 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-worker-stats.json') } } -WEBPACK_CONFIG_PATH = 'webpack.prod.config.js' + +# .. setting_name: WEBPACK_CONFIG_PATH +# .. setting_default: "webpack.prod.config.js" +# .. setting_description: Path to the Webpack configuration file. Used by Paver scripts. +# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer +# use Django settings. Please set the WEBPACK_CONFIG_PATH environment variable instead. For details, +# see: https://github.com/openedx/edx-platform/issues/31895 +WEBPACK_CONFIG_PATH = os.environ.get('WEBPACK_CONFIG_PATH', 'webpack.prod.config.js') ############################ SERVICE_VARIANT ################################## @@ -2180,8 +2190,11 @@ # .. setting_name: COMPREHENSIVE_THEME_DIRS # .. setting_default: [] -# .. setting_description: See LMS annotation. -COMPREHENSIVE_THEME_DIRS = [] +# .. setting_description: A list of paths to directories, each of which will +# be searched for comprehensive themes. Do not override this Django setting directly. +# Instead, set the COMPREHENSIVE_THEME_DIRS environment variable, using colons (:) to +# separate paths. +COMPREHENSIVE_THEME_DIRS = os.environ.get("COMPREHENSIVE_THEME_DIRS", "").split(":") # .. setting_name: COMPREHENSIVE_THEME_LOCALE_PATHS # .. setting_default: [] diff --git a/docs/decisions/0017-reimplement-asset-processing.rst b/docs/decisions/0017-reimplement-asset-processing.rst index 1048edf4c075..e689c34eb9cf 100644 --- a/docs/decisions/0017-reimplement-asset-processing.rst +++ b/docs/decisions/0017-reimplement-asset-processing.rst @@ -11,16 +11,14 @@ Overview Status ****** -**Provisional** +**Accepted** -This was `originally authored `_ in March 2023. We `modified it in July 2023 `_ based on learnings from the implementation process. +This was `originally authored `_ in March 2023. We `modified it in July 2023 `_ based on learnings from the implementation process, and then `modified and it again in May 2024 `_ to make the migration easier for operators to understand. -The status will be moved to *Accepted* upon completion of reimplementation. Related work: +Related deprecation tickets: * `[DEPR]: Asset processing in Paver `_ -* `Process edx-platform assets without Paver `_ -* `Process edx-platform assets without Python `_ - +* `[DEPR]: Paver `_ Context ******* @@ -92,7 +90,6 @@ Three particular issues have surfaced in Developer Experience Working Group disc All of these potential solutions would involve refactoring or entirely replacing parts of the current asset processing system. - Decision ******** @@ -114,6 +111,9 @@ Reimplementation Specification Commands and stages ------------------- +**May 2024 update:** See the `static assets reference <../references/static-assets.rst>`_ for +the latest commands. + The three top-level edx-platform asset processing actions are *build*, *collect*, and *watch*. The build action can be further broken down into five stages. Here is how those actions and stages will be reimplemented: @@ -226,6 +226,9 @@ The three top-level edx-platform asset processing actions are *build*, *collect* Build Configuration ------------------- +**May 2024 update:** See the `static assets reference <../references/static-assets.rst>`_ for +the latest configuration settings. + To facilitate a generally Python-free build reimplementation, we will require that certain Django settings now be specified as environment variables, which can be passed to the build like so:: MY_ENV_VAR="my value" npm run build # Set for the whole build. @@ -266,7 +269,7 @@ Some of these options will remain as Django settings because they are used in ed * - ``COMPREHENSIVE_THEME_DIRS`` - Directories that will be searched when compiling themes. - ``COMPREHENSIVE_THEME_DIRS`` - - ``EDX_PLATFORM_THEME_DIRS`` + - ``COMPREHENSIVE_THEME_DIRS`` Migration ========= @@ -285,37 +288,16 @@ As a consequence of this ADR, Tutor will either need to: * reimplement the script as a thin wrapper around the new asset processing commands, or * deprecate and remove the script. -Either way, the migration path is straightforward: - -.. list-table:: - :header-rows: 1 - - * - Existing Tutor-provided command - - New upstream command - * - ``openedx-assets build`` - - ``npm run build`` - * - ``openedx-assets npm`` - - ``scripts/copy-node-modules.sh # (automatically invoked by 'npm install'!)`` - * - ``openedx-assets xmodule`` - - (no longer needed) - * - ``openedx-assets common`` - - ``npm run compile-sass -- --skip-themes`` - * - ``openedx-assets themes`` - - ``npm run compile-sass -- --skip-default`` - * - ``openedx-assets webpack`` - - ``npm run webpack`` - * - ``openedx-assets collect`` - - ``./manage.py [lms|cms] collectstatic --noinput`` - * - ``openedx-assets watch-themes`` - - ``npm run watch`` - -The options accepted by ``openedx-assets`` will all be valid inputs to ``scripts/build-assets.sh``. +**May 2024 update:** The ``openedx-assets`` script will be removed from Tutor, +with migration instructions documented in +`Tutor's changelog `_. non-Tutor migration guide ------------------------- -Operators using distributions other than Tutor should refer to the upstream edx-platform changes described above in **Reimplementation Specification**, and adapt them accordingly to their distribution. - +A migration guide for site operators who are directly referencing Paver will be +included in the +`Paver deprecation ticket `_. See also ******** diff --git a/lms/envs/common.py b/lms/envs/common.py index f6266fdaa3cb..02e52ea5a317 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1928,7 +1928,7 @@ def _make_mako_template_dirs(settings): # Static content STATIC_URL = '/static/' -STATIC_ROOT = ENV_ROOT / "staticfiles" +STATIC_ROOT = os.environ.get('STATIC_ROOT_LMS', ENV_ROOT / "staticfiles") STATIC_URL_BASE = '/static/' STATICFILES_DIRS = [ @@ -2822,7 +2822,14 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-worker-stats.json') } } -WEBPACK_CONFIG_PATH = 'webpack.prod.config.js' + +# .. setting_name: WEBPACK_CONFIG_PATH +# .. setting_default: "webpack.prod.config.js" +# .. setting_description: Path to the Webpack configuration file. Used by Paver scripts. +# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer +# use Django settings. Please set the WEBPACK_CONFIG_PATH environment variable instead. For details, +# see: https://github.com/openedx/edx-platform/issues/31895 +WEBPACK_CONFIG_PATH = os.environ.get('WEBPACK_CONFIG_PATH', 'webpack.prod.config.js') ########################## DJANGO DEBUG TOOLBAR ############################### @@ -4549,9 +4556,11 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # .. setting_name: COMPREHENSIVE_THEME_DIRS # .. setting_default: [] -# .. setting_description: A list of directories containing themes folders, -# each entry should be a full path to the directory containing the theme folder. -COMPREHENSIVE_THEME_DIRS = [] +# .. setting_description: A list of paths to directories, each of which will +# be searched for comprehensive themes. Do not override this Django setting directly. +# Instead, set the COMPREHENSIVE_THEME_DIRS environment variable, using colons (:) to +# separate paths. +COMPREHENSIVE_THEME_DIRS = os.environ.get("COMPREHENSIVE_THEME_DIRS", "").split(":") # .. setting_name: COMPREHENSIVE_THEME_LOCALE_PATHS # .. setting_default: [] diff --git a/openedx/core/djangoapps/theming/management/commands/compile_sass.py b/openedx/core/djangoapps/theming/management/commands/compile_sass.py index 765ef98aeac3..eecbe684b528 100644 --- a/openedx/core/djangoapps/theming/management/commands/compile_sass.py +++ b/openedx/core/djangoapps/theming/management/commands/compile_sass.py @@ -1,13 +1,14 @@ """ Management command for compiling sass. -""" +DEPRECATED in favor of `npm run compile-sass`. +""" +import shlex -from django.core.management import BaseCommand, CommandError -from paver.easy import call_task +from django.core.management import BaseCommand +from django.conf import settings -from openedx.core.djangoapps.theming.helpers import get_theme_base_dirs, get_themes, is_comprehensive_theming_enabled -from pavelib.assets import ALL_SYSTEMS +from pavelib.assets import run_deprecated_command_wrapper class Command(BaseCommand): @@ -15,7 +16,7 @@ class Command(BaseCommand): Compile theme sass and collect theme assets. """ - help = 'Compile and collect themed assets...' + help = "DEPRECATED. Use 'npm run compile-sass' instead." # NOTE (CCB): This allows us to compile static assets in Docker containers without database access. requires_system_checks = [] @@ -28,7 +29,7 @@ def add_arguments(self, parser): parser (django.core.management.base.CommandParser): parsed for parsing command line arguments. """ parser.add_argument( - 'system', type=str, nargs='*', default=ALL_SYSTEMS, + 'system', type=str, nargs='*', default=["lms", "cms"], help="lms or studio", ) @@ -55,7 +56,7 @@ def add_arguments(self, parser): '--force', action='store_true', default=False, - help="Force full compilation", + help="DEPRECATED. Full recompilation is now always forced.", ) parser.add_argument( '--debug', @@ -64,77 +65,48 @@ def add_arguments(self, parser): help="Disable Sass compression", ) - @staticmethod - def parse_arguments(*args, **options): # pylint: disable=unused-argument - """ - Parse and validate arguments for compile_sass command. - - Args: - *args: Positional arguments passed to the update_assets command - **options: optional arguments passed to the update_assets command - Returns: - A tuple containing parsed values for themes, system, source comments and output style. - 1. system (list): list of system names for whom to compile theme sass e.g. 'lms', 'cms' - 2. theme_dirs (list): list of Theme objects - 3. themes (list): list of Theme objects - 4. force (bool): Force full compilation - 5. debug (bool): Disable Sass compression - """ - system = options.get("system", ALL_SYSTEMS) - given_themes = options.get("themes", ["all"]) - theme_dirs = options.get("theme_dirs", None) - - force = options.get("force", True) - debug = options.get("debug", True) - - if theme_dirs: - available_themes = {} - for theme_dir in theme_dirs: - available_themes.update({t.theme_dir_name: t for t in get_themes(theme_dir)}) - else: - theme_dirs = get_theme_base_dirs() - available_themes = {t.theme_dir_name: t for t in get_themes()} - - if 'no' in given_themes or 'all' in given_themes: - # Raise error if 'all' or 'no' is present and theme names are also given. - if len(given_themes) > 1: - raise CommandError("Invalid themes value, It must either be 'all' or 'no' or list of themes.") - # Raise error if any of the given theme name is invalid - # (theme name would be invalid if it does not exist in themes directory) - elif (not set(given_themes).issubset(list(available_themes.keys()))) and is_comprehensive_theming_enabled(): - raise CommandError( - "Given themes '{themes}' do not exist inside any of the theme directories '{theme_dirs}'".format( - themes=", ".join(set(given_themes) - set(available_themes.keys())), - theme_dirs=theme_dirs, - ), - ) - - if "all" in given_themes: - themes = list(available_themes.values()) - elif "no" in given_themes: - themes = [] - else: - # convert theme names to Theme objects, this will remove all themes if theming is disabled - themes = [available_themes.get(theme) for theme in given_themes if theme in available_themes] - - return system, theme_dirs, themes, force, debug - def handle(self, *args, **options): """ Handle compile_sass command. """ - system, theme_dirs, themes, force, debug = self.parse_arguments(*args, **options) - themes = [theme.theme_dir_name for theme in themes] - - if options.get("themes", None) and not is_comprehensive_theming_enabled(): - # log a warning message to let the user know that asset compilation for themes is skipped - self.stdout.write( - self.style.WARNING( - "Skipping theme asset compilation: enable theming to process themed assets" + systems = set( + {"lms": "lms", "cms": "cms", "studio": "cms"}[sys] + for sys in options.get("system", ["lms", "cms"]) + ) + theme_dirs = options.get("theme_dirs", settings.COMPREHENSIVE_THEME_DIRS) or [] + themes_option = options.get("themes", []) # '[]' means 'all' + if not settings.ENABLE_COMPREHENSIVE_THEMING: + compile_themes = False + themes = [] + elif "no" in themes_option: + compile_themes = False + themes = [] + elif "all" in themes_option: + compile_themes = True + themes = [] + else: + compile_themes = True + themes = themes_option + run_deprecated_command_wrapper( + old_command="./manage.py [lms|cms] compile_sass", + ignored_old_flags=list(set(["force"]) & set(options)), + new_command=shlex.join([ + "npm", + "run", + ("compile-sass-dev" if options.get("debug") else "compile-sass"), + "--", + *(["--skip-lms"] if "lms" not in systems else []), + *(["--skip-cms"] if "cms" not in systems else []), + *(["--skip-themes"] if not compile_themes else []), + *( + arg + for theme_dir in theme_dirs + for arg in ["--theme-dir", str(theme_dir)] ), - ) - - call_task( - 'pavelib.assets.compile_sass', - options={'system': system, 'theme_dirs': theme_dirs, 'themes': themes, 'force': force, 'debug': debug}, + *( + arg + for theme in themes + for arg in ["--theme", theme] + ), + ]), ) diff --git a/openedx/core/djangoapps/theming/tests/test_commands.py b/openedx/core/djangoapps/theming/tests/test_commands.py index b4abee9bed0e..7ca56f604a41 100644 --- a/openedx/core/djangoapps/theming/tests/test_commands.py +++ b/openedx/core/djangoapps/theming/tests/test_commands.py @@ -2,57 +2,23 @@ Tests for Management commands of comprehensive theming. """ -import pytest -from django.core.management import CommandError, call_command -from django.test import TestCase +from django.core.management import call_command +from django.test import TestCase, override_settings +from unittest.mock import patch -from openedx.core.djangoapps.theming.helpers import get_themes -from openedx.core.djangoapps.theming.management.commands.compile_sass import Command +import pavelib.assets class TestUpdateAssets(TestCase): """ Test comprehensive theming helper functions. """ - def setUp(self): - super().setUp() - self.themes = get_themes() - def test_errors_for_invalid_arguments(self): - """ - Test update_asset command. - """ - # make sure error is raised for invalid theme list - with pytest.raises(CommandError): - call_command("compile_sass", themes=["all", "test-theme"]) - - # make sure error is raised for invalid theme list - with pytest.raises(CommandError): - call_command("compile_sass", themes=["no", "test-theme"]) - - # make sure error is raised for invalid theme list - with pytest.raises(CommandError): - call_command("compile_sass", themes=["all", "no"]) - - # make sure error is raised for invalid theme list - with pytest.raises(CommandError): - call_command("compile_sass", themes=["test-theme", "non-existing-theme"]) - - def test_parse_arguments(self): - """ - Test parse arguments method for update_asset command. - """ - # make sure compile_sass picks all themes when called with 'themes=all' option - parsed_args = Command.parse_arguments(themes=["all"]) - self.assertCountEqual(parsed_args[2], get_themes()) - - # make sure compile_sass picks no themes when called with 'themes=no' option - parsed_args = Command.parse_arguments(themes=["no"]) - self.assertCountEqual(parsed_args[2], []) - - # make sure compile_sass picks only specified themes - parsed_args = Command.parse_arguments(themes=["test-theme"]) - self.assertCountEqual( - parsed_args[2], - [theme for theme in get_themes() if theme.theme_dir_name == "test-theme"] + @patch.object(pavelib.assets, 'sh') + @override_settings(COMPREHENSIVE_THEME_DIRS='common/test') + def test_deprecated_wrapper(self, mock_sh): + call_command('compile_sass', '--themes', 'fake-theme1', 'fake-theme2') + assert mock_sh.called_once_with( + "npm run compile-sass -- " + + "--theme-dir common/test --theme fake-theme-1 --theme fake-theme-2" ) diff --git a/pavelib/assets.py b/pavelib/assets.py index 466ffc9ed919..f437b6427f93 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -1,5 +1,9 @@ """ Asset compilation and collection. + +This entire module is DEPRECATED. In Redwood, it exists just as a collection of temporary compatibility wrappers. +In Sumac, this module will be deleted. To migrate, follow the advice in the printed warnings and/or read the +instructions on the DEPR ticket: https://github.com/openedx/edx-platform/issues/31895 """ import argparse @@ -13,31 +17,48 @@ from paver import tasks from paver.easy import call_task, cmdopts, consume_args, needs, no_help, sh, task from watchdog.events import PatternMatchingEventHandler -from watchdog.observers import Observer # pylint disable=unused-import # Used by Tutor. Remove after Sumac cut. +from watchdog.observers import Observer # pylint: disable=unused-import # Used by Tutor. Remove after Sumac cut. -from .utils.cmd import cmd, django_cmd +from .utils.cmd import django_cmd from .utils.envs import Env -from .utils.process import run_background_process from .utils.timer import timed -# setup baseline paths - -ALL_SYSTEMS = ['lms', 'studio'] - -LMS = 'lms' -CMS = 'cms' SYSTEMS = { - 'lms': LMS, - 'cms': CMS, - 'studio': CMS + 'lms': 'lms', + 'cms': 'cms', + 'studio': 'cms', } -# Collectstatic log directory setting -COLLECTSTATIC_LOG_DIR_ARG = 'collect_log_dir' +WARNING_SYMBOLS = "⚠️ " * 50 # A row of 'warning' emoji to catch CLI users' attention + -# Webpack command -WEBPACK_COMMAND = 'STATIC_ROOT_LMS={static_root_lms} STATIC_ROOT_CMS={static_root_cms} webpack {options}' +def run_deprecated_command_wrapper(*, old_command, ignored_old_flags, new_command): + """ + Run the new version of shell command, plus a warning that the old version is deprecated. + """ + depr_warning = ( + "\n" + + f"{WARNING_SYMBOLS}\n" + + "\n" + + f"WARNING: '{old_command}' is DEPRECATED! It will be removed before Sumac.\n" + + "The command you ran is now just a temporary wrapper around a new,\n" + + "supported command, which you should use instead:\n" + + "\n" + + f"\t{new_command}\n" + + "\n" + + "Details: https://github.com/openedx/edx-platform/issues/31895\n" + + "".join( + f" WARNING: ignored deprecated paver flag '{flag}'\n" + for flag in ignored_old_flags + ) + + f"{WARNING_SYMBOLS}\n" + + "\n" + ) + # Print deprecation warning twice so that it's more likely to be seen in the logs. + print(depr_warning) + sh(new_command) + print(depr_warning) def debounce(seconds=1): @@ -45,6 +66,8 @@ def debounce(seconds=1): Prevents the decorated function from being called more than every `seconds` seconds. Waits until calls stop coming in before calling the decorated function. + + This is DEPRECATED. It exists in Redwood just to ease the transition for Tutor. """ def decorator(func): func.timer = None @@ -66,6 +89,8 @@ def call(): class SassWatcher(PatternMatchingEventHandler): """ Watches for sass file changes + + This is DEPRECATED. It exists in Redwood just to ease the transition for Tutor. """ ignore_directories = True patterns = ['*.scss'] @@ -102,7 +127,7 @@ def on_any_event(self, event): ('system=', 's', 'The system to compile sass for (defaults to all)'), ('theme-dirs=', '-td', 'Theme dirs containing all themes (defaults to None)'), ('themes=', '-t', 'The theme to compile sass for (defaults to None)'), - ('debug', 'd', 'DEPRECATED. Debug mode is now determined by NODE_ENV.'), + ('debug', 'd', 'Whether to use development settings'), ('force', '', 'DEPRECATED. Full recompilation is now always forced.'), ]) @timed @@ -143,16 +168,18 @@ def compile_sass(options): This is a DEPRECATED COMPATIBILITY WRAPPER. Use `npm run compile-sass` instead. """ - systems = set(get_parsed_option(options, 'system', ALL_SYSTEMS)) - command = shlex.join( - [ + systems = [SYSTEMS[sys] for sys in get_parsed_option(options, 'system', ['lms', 'cms'])] # normalize studio->cms + run_deprecated_command_wrapper( + old_command="paver compile_sass", + ignored_old_flags=(set(["--force"]) & set(options)), + new_command=shlex.join([ "npm", "run", - "compile-sass", + ("compile-sass-dev" if options.get("debug") else "compile-sass"), "--", *(["--dry"] if tasks.environment.dry_run else []), - *(["--skip-lms"] if not systems & {"lms"} else []), - *(["--skip-cms"] if not systems & {"cms", "studio"} else []), + *(["--skip-lms"] if "lms" not in systems else []), + *(["--skip-cms"] if "cms" not in systems else []), *( arg for theme_dir in get_parsed_option(options, 'theme_dirs', []) @@ -160,77 +187,50 @@ def compile_sass(options): ), *( arg - for theme in get_parsed_option(options, "theme", []) + for theme in get_parsed_option(options, "themes", []) for arg in ["--theme", theme] ), - ] - ) - depr_warning = ( - "\n" + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" + - "WARNING: 'paver compile_sass' is DEPRECATED! It will be removed before Sumac.\n" + - "The command you ran is now just a temporary wrapper around a new,\n" + - "supported command, which you should use instead:\n" + - "\n" + - f"\t{command}\n" + - "\n" + - "Details: https://github.com/openedx/edx-platform/issues/31895\n" + - "\n" + - ("WARNING: ignoring deprecated flag '--debug'\n" if options.get("debug") else "") + - ("WARNING: ignoring deprecated flag '--force'\n" if options.get("force") else "") + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" + ]), ) - # Print deprecation warning twice so that it's more likely to be seen in the logs. - print(depr_warning) - sh(command) - print(depr_warning) -def _compile_sass(system, theme, _debug, _force, _timing_info): +def _compile_sass(system, theme, debug, force, _timing_info): """ This is a DEPRECATED COMPATIBILITY WRAPPER It exists to ease the transition for Tutor in Redwood, which directly imported and used this function. """ - command = shlex.join( - [ + run_deprecated_command_wrapper( + old_command="pavelib.assets:_compile_sass", + ignored_old_flags=(set(["--force"]) if force else set()), + new_command=[ "npm", "run", - "compile-sass", + ("compile-sass-dev" if debug else "compile-sass"), "--", *(["--dry"] if tasks.environment.dry_run else []), - *(["--skip-default", "--theme-dir", str(theme.parent), "--theme", str(theme.name)] if theme else []), + *( + ["--skip-default", "--theme-dir", str(theme.parent), "--theme", str(theme.name)] + if theme + else [] + ), ("--skip-cms" if system == "lms" else "--skip-lms"), ] ) - depr_warning = ( - "\n" + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" + - "WARNING: 'pavelib/assets.py' is DEPRECATED! It will be removed before Sumac.\n" + - "The function you called is just a temporary wrapper around a new, supported command,\n" + - "which you should use instead:\n" + - "\n" + - f"\t{command}\n" + - "\n" + - "Details: https://github.com/openedx/edx-platform/issues/31895\n" + - "\n" + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" - ) - # Print deprecation warning twice so that it's more likely to be seen in the logs. - print(depr_warning) - sh(command) - print(depr_warning) def process_npm_assets(): """ Process vendor libraries installed via NPM. + + This is a DEPRECATED COMPATIBILITY WRAPPER. It is now handled as part of `npm clean-install`. + If you need to invoke it explicitly, you can run `npm run postinstall`. """ - sh('scripts/copy-node-modules.sh') + run_deprecated_command_wrapper( + old_command="pavelib.assets:process_npm_assets", + ignored_old_flags=[], + new_command=shlex.join(["npm", "run", "postinstall"]), + ) @task @@ -238,9 +238,21 @@ def process_npm_assets(): def process_xmodule_assets(): """ Process XModule static assets. + + This is a DEPRECATED COMPATIBILITY STUB. Refrences to it should be deleted. """ - print("\t\tProcessing xmodule assets is no longer needed. This task is now a no-op.") - print("\t\tWhen paver is removed from edx-platform, this step will not replaced.") + print( + "\n" + + f"{WARNING_SYMBOLS}", + "\n" + + "WARNING: 'paver process_xmodule_assets' is DEPRECATED! It will be removed before Sumac.\n" + + "\n" + + "Starting with Quince, it is no longer necessary to post-process XModule assets, so \n" + + "'paver process_xmodule_assets' is a no-op. Please simply remove it from your build scripts.\n" + + "\n" + + "Details: https://github.com/openedx/edx-platform/issues/31895\n" + + f"{WARNING_SYMBOLS}", + ) def collect_assets(systems, settings, **kwargs): @@ -249,33 +261,29 @@ def collect_assets(systems, settings, **kwargs): `systems` is a list of systems (e.g. 'lms' or 'studio' or both) `settings` is the Django settings module to use. `**kwargs` include arguments for using a log directory for collectstatic output. Defaults to /dev/null. - """ - for sys in systems: - collectstatic_stdout_str = _collect_assets_cmd(sys, **kwargs) - sh(django_cmd(sys, settings, "collectstatic --noinput {logfile_str}".format( - logfile_str=collectstatic_stdout_str - ))) - print(f"\t\tFinished collecting {sys} assets.") - -def _collect_assets_cmd(system, **kwargs): - """ - Returns the collecstatic command to be used for the given system + This is a DEPRECATED COMPATIBILITY WRAPPER - Unless specified, collectstatic (which can be verbose) pipes to /dev/null + It exists to ease the transition for Tutor in Redwood, which directly imported and used this function. """ - try: - if kwargs[COLLECTSTATIC_LOG_DIR_ARG] is None: - collectstatic_stdout_str = "" - else: - collectstatic_stdout_str = "> {output_dir}/{sys}-collectstatic.log".format( - output_dir=kwargs[COLLECTSTATIC_LOG_DIR_ARG], - sys=system - ) - except KeyError: - collectstatic_stdout_str = "> /dev/null" - - return collectstatic_stdout_str + run_deprecated_command_wrapper( + old_command="pavelib.asset:collect_assets", + ignored_old_flags=[], + new_command=" && ".join( + "( " + + shlex.join( + ["./manage.py", SYSTEMS[sys], f"--settings={settings}", "collectstatic", "--noinput"] + ) + ( + "" + if "collect_log_dir" not in kwargs else + " > /dev/null" + if kwargs["collect_log_dir"] is None else + f"> {kwargs['collect_log_dir']}/{SYSTEMS[sys]}-collectstatic.out" + ) + + " )" + for sys in systems + ), + ) def execute_compile_sass(args): @@ -283,6 +291,8 @@ def execute_compile_sass(args): Construct django management command compile_sass (defined in theming app) and execute it. Args: args: command line argument passed via update_assets command + + This is a DEPRECATED COMPATIBILITY WRAPPER. Use `npm run compile-sass` instead. """ for sys in args.system: options = "" @@ -305,12 +315,14 @@ def execute_compile_sass(args): @task @cmdopts([ ('settings=', 's', "Django settings (defaults to devstack)"), - ('watch', 'w', "Watch file system and rebuild on change (defaults to off)"), + ('watch', 'w', "DEPRECATED. This flag never did anything anyway."), ]) @timed def webpack(options): """ Run a Webpack build. + + This is a DEPRECATED COMPATIBILITY WRAPPER. Use `npm run webpack` instead. """ settings = getattr(options, 'settings', Env.DEVSTACK_SETTINGS) result = Env.get_django_settings(['STATIC_ROOT', 'WEBPACK_CONFIG_PATH'], "lms", settings=settings) @@ -318,44 +330,20 @@ def webpack(options): static_root_cms, = Env.get_django_settings(["STATIC_ROOT"], "cms", settings=settings) js_env_extra_config_setting, = Env.get_django_json_settings(["JS_ENV_EXTRA_CONFIG"], "cms", settings=settings) js_env_extra_config = json.dumps(js_env_extra_config_setting or "{}") - environment = ( - "NODE_ENV={node_env} STATIC_ROOT_LMS={static_root_lms} STATIC_ROOT_CMS={static_root_cms} " - "JS_ENV_EXTRA_CONFIG={js_env_extra_config}" - ).format( - node_env="development" if config_path == 'webpack.dev.config.js' else "production", - static_root_lms=static_root_lms, - static_root_cms=static_root_cms, - js_env_extra_config=js_env_extra_config, - ) - sh( - cmd( - '{environment} webpack --config={config_path}'.format( - environment=environment, - config_path=config_path - ) - ) - ) - - -def execute_webpack_watch(settings=None): - """ - Run the Webpack file system watcher. - """ - # We only want Webpack to re-run on changes to its own entry points, - # not all JS files, so we use its own watcher instead of subclassing - # from Watchdog like the other watchers do. - - result = Env.get_django_settings(["STATIC_ROOT", "WEBPACK_CONFIG_PATH"], "lms", settings=settings) - static_root_lms, config_path = result - static_root_cms, = Env.get_django_settings(["STATIC_ROOT"], "cms", settings=settings) - run_background_process( - 'STATIC_ROOT_LMS={static_root_lms} STATIC_ROOT_CMS={static_root_cms} webpack {options}'.format( - options='--watch --config={config_path}'.format( - config_path=config_path - ), - static_root_lms=static_root_lms, - static_root_cms=static_root_cms, - ) + node_env = "development" if config_path == 'webpack.dev.config.js' else "production" + run_deprecated_command_wrapper( + old_command="paver webpack", + ignored_old_flags=(set(["watch"]) & set(options)), + new_command=' '.join([ + f"WEBPACK_CONFIG_PATH={config_path}", + f"NODE_ENV={node_env}", + f"STATIC_ROOT_LMS={static_root_lms}", + f"STATIC_ROOT_CMS={static_root_cms}", + f"JS_ENV_EXTRA_CONFIG={js_env_extra_config}", + "npm", + "run", + "webpack", + ]), ) @@ -411,39 +399,19 @@ def watch_assets(options): return theme_dirs = ':'.join(get_parsed_option(options, 'theme_dirs', [])) - command = shlex.join( - [ + run_deprecated_command_wrapper( + old_command="paver watch_assets", + ignored_old_flags=(set(["debug", "themes", "settings", "background"]) & set(options)), + new_command=shlex.join([ *( - ["env", f"EDX_PLATFORM_THEME_DIRS={theme_dirs}"] if theme_dirs else [] + ["env", f"COMPREHENSIVE_THEME_DIRS={theme_dirs}"] + if theme_dirs else [] ), "npm", "run", "watch", - ] + ]), ) - depr_warning = ( - "\n" + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" + - "WARNING: 'paver watch_assets' is DEPRECATED! It will be removed before Sumac.\n" + - "The command you ran is now just a temporary wrapper around a new,\n" + - "supported command, which you should use instead:\n" + - "\n" + - f"\t{command}\n" + - "\n" + - "Details: https://github.com/openedx/edx-platform/issues/31895\n" + - "\n" + - ("WARNING: ignoring deprecated flag '--debug'\n" if options.get("debug") else "") + - ("WARNING: ignoring deprecated flag '--themes'\n" if options.get("themes") else "") + - ("WARNING: ignoring deprecated flag '--settings'\n" if options.get("settings") else "") + - ("WARNING: ignoring deprecated flag '--background'\n" if options.get("background") else "") + - "⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ \n" + - "\n" - ) - # Print deprecation warning twice so that it's more likely to be seen in the logs. - print(depr_warning) - sh(command) - print(depr_warning) @task @@ -456,10 +424,19 @@ def watch_assets(options): def update_assets(args): """ Compile Sass, then collect static assets. + + This is a DEPRECATED COMPATIBILITY WRAPPER around other DEPRECATED COMPATIBILITY WRAPPERS. + The aggregate affect of this command can be achieved with this sequence of commands instead: + + * pip install -r requirements/edx/assets.txt # replaces install_python_prereqs + * npm clean-install # replaces install_node_prereqs + * npm run build # replaces execute_compile_sass and webpack + * ./manage.py lms collectstatic --noinput # replaces collect_assets (for LMS) + * ./manage.py cms collectstatic --noinput # replaces collect_assets (for CMS) """ parser = argparse.ArgumentParser(prog='paver update_assets') parser.add_argument( - 'system', type=str, nargs='*', default=ALL_SYSTEMS, + 'system', type=str, nargs='*', default=["lms", "studio"], help="lms or studio", ) parser.add_argument( @@ -488,18 +465,17 @@ def update_assets(args): ) parser.add_argument( '--themes', type=str, nargs='+', default=None, - help="list of themes to compile sass for", + help="list of themes to compile sass for. ignored when --watch is used; all themes are watched.", ) parser.add_argument( - '--collect-log', dest=COLLECTSTATIC_LOG_DIR_ARG, default=None, + '--collect-log', dest="collect_log_dir", default=None, help="When running collectstatic, direct output to specified log directory", ) parser.add_argument( '--wait', type=float, default=0.0, - help="How long to pause between filesystem scans" + help="DEPRECATED. Watchdog's default wait time is now used.", ) args = parser.parse_args(args) - collect_log_args = {} # Build Webpack call_task('pavelib.assets.webpack', options={'settings': args.settings}) @@ -508,11 +484,12 @@ def update_assets(args): execute_compile_sass(args) if args.collect: - if args.debug or args.debug_collect: - collect_log_args.update({COLLECTSTATIC_LOG_DIR_ARG: None}) - if args.collect_log_dir: - collect_log_args.update({COLLECTSTATIC_LOG_DIR_ARG: args.collect_log_dir}) + collect_log_args = {"collect_log_dir": args.collect_log_dir} + elif args.debug or args.debug_collect: + collect_log_args = {"collect_log_dir": None} + else: + collect_log_args = {} collect_assets(args.system, args.settings, **collect_log_args) diff --git a/pavelib/paver_tests/test_assets.py b/pavelib/paver_tests/test_assets.py index 029a8db67c4a..3578a5043f7e 100644 --- a/pavelib/paver_tests/test_assets.py +++ b/pavelib/paver_tests/test_assets.py @@ -1,305 +1,130 @@ """Unit tests for the Paver asset tasks.""" - +import json import os +from pathlib import Path from unittest import TestCase from unittest.mock import patch import ddt -import paver.tasks -from paver.easy import call_task, path +import paver.easy +from paver import tasks -from pavelib.assets import COLLECTSTATIC_LOG_DIR_ARG, collect_assets +import pavelib.assets +from pavelib.assets import Env -from ..utils.envs import Env -from .utils import PaverTestCase -ROOT_PATH = path(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))) -TEST_THEME_DIR = ROOT_PATH / "common/test/test-theme" +REPO_ROOT = Path(__file__).parent.parent.parent +LMS_SETTINGS = { + "WEBPACK_CONFIG_PATH": "webpack.fake.config.js", + "STATIC_ROOT": "/fake/lms/staticfiles", -@ddt.ddt -class TestPaverAssetTasks(PaverTestCase): - """ - Test the Paver asset tasks. - """ - @ddt.data( - [""], - ["--force"], - ["--debug"], - ["--system=lms"], - ["--system=lms --force"], - ["--system=studio"], - ["--system=studio --force"], - ["--system=lms,studio"], - ["--system=lms,studio --force"], - ) - @ddt.unpack - def test_compile_sass(self, options): - """ - Test the "compile_sass" task. - """ - parameters = options.split(" ") - system = [] - if '--system=studio' not in parameters: - system += ['lms'] - if '--system=lms' not in parameters: - system += ['studio'] - debug = '--debug' in parameters - force = '--force' in parameters - self.reset_task_messages() - call_task('pavelib.assets.compile_sass', options={'system': system, 'debug': debug, 'force': force}) - expected_messages = [] - if force: - expected_messages.append('rm -rf common/static/css/*.css') - expected_messages.append('libsass common/static/sass') +} +CMS_SETTINGS = { + "WEBPACK_CONFIG_PATH": "webpack.fake.config", + "STATIC_ROOT": "/fake/cms/staticfiles", + "JS_ENV_EXTRA_CONFIG": json.dumps({"key1": [True, False], "key2": {"key2.1": 1369, "key2.2": "1369"}}), +} - if "lms" in system: - if force: - expected_messages.append('rm -rf lms/static/css/*.css') - expected_messages.append('libsass lms/static/sass') - expected_messages.append( - 'rtlcss lms/static/css/bootstrap/lms-main.css lms/static/css/bootstrap/lms-main-rtl.css' - ) - expected_messages.append( - 'rtlcss lms/static/css/discussion/lms-discussion-bootstrap.css' - ' lms/static/css/discussion/lms-discussion-bootstrap-rtl.css' - ) - if force: - expected_messages.append('rm -rf lms/static/certificates/css/*.css') - expected_messages.append('libsass lms/static/certificates/sass') - if "studio" in system: - if force: - expected_messages.append('rm -rf cms/static/css/*.css') - expected_messages.append('libsass cms/static/sass') - expected_messages.append( - 'rtlcss cms/static/css/bootstrap/studio-main.css cms/static/css/bootstrap/studio-main-rtl.css' - ) - assert len(self.task_messages) == len(expected_messages) +def _mock_get_django_settings(django_settings, system, settings=None): # pylint: disable=unused-argument + return [(LMS_SETTINGS if system == "lms" else CMS_SETTINGS)[s] for s in django_settings] @ddt.ddt -class TestPaverThemeAssetTasks(PaverTestCase): +@patch.object(Env, 'get_django_settings', _mock_get_django_settings) +@patch.object(Env, 'get_django_json_settings', _mock_get_django_settings) +class TestDeprecatedPaverAssets(TestCase): """ - Test the Paver asset tasks. + Simple test to ensure that the soon-to-be-removed Paver commands are correctly translated into the new npm-run + commands. """ - @ddt.data( - [""], - ["--force"], - ["--debug"], - ["--system=lms"], - ["--system=lms --force"], - ["--system=studio"], - ["--system=studio --force"], - ["--system=lms,studio"], - ["--system=lms,studio --force"], - ) - @ddt.unpack - def test_compile_theme_sass(self, options): - """ - Test the "compile_sass" task. - """ - parameters = options.split(" ") - system = [] - - if '--system=studio' not in parameters: - system += ['lms'] - if "--system=lms" not in parameters: - system += ['studio'] - debug = '--debug' in parameters - force = '--force' in parameters - - self.reset_task_messages() - call_task( - 'pavelib.assets.compile_sass', - options=dict( - system=system, - debug=debug, - force=force, - theme_dirs=[TEST_THEME_DIR.dirname()], - themes=[TEST_THEME_DIR.basename()] - ), - ) - expected_messages = [] - if force: - expected_messages.append('rm -rf common/static/css/*.css') - expected_messages.append('libsass common/static/sass') - - if 'lms' in system: - expected_messages.append('mkdir_p ' + repr(TEST_THEME_DIR / 'lms/static/css')) - if force: - expected_messages.append( - f'rm -rf {str(TEST_THEME_DIR)}/lms/static/css/*.css' - ) - expected_messages.append("libsass lms/static/sass") - expected_messages.append( - 'rtlcss {test_theme_dir}/lms/static/css/bootstrap/lms-main.css' - ' {test_theme_dir}/lms/static/css/bootstrap/lms-main-rtl.css'.format( - test_theme_dir=str(TEST_THEME_DIR), - ) - ) - expected_messages.append( - 'rtlcss {test_theme_dir}/lms/static/css/discussion/lms-discussion-bootstrap.css' - ' {test_theme_dir}/lms/static/css/discussion/lms-discussion-bootstrap-rtl.css'.format( - test_theme_dir=str(TEST_THEME_DIR), - ) - ) - if force: - expected_messages.append( - f'rm -rf {str(TEST_THEME_DIR)}/lms/static/css/*.css' - ) - expected_messages.append( - f'libsass {str(TEST_THEME_DIR)}/lms/static/sass' - ) - if force: - expected_messages.append('rm -rf lms/static/css/*.css') - expected_messages.append('libsass lms/static/sass') - expected_messages.append( - 'rtlcss lms/static/css/bootstrap/lms-main.css lms/static/css/bootstrap/lms-main-rtl.css' - ) - expected_messages.append( - 'rtlcss lms/static/css/discussion/lms-discussion-bootstrap.css' - ' lms/static/css/discussion/lms-discussion-bootstrap-rtl.css' - ) - if force: - expected_messages.append('rm -rf lms/static/certificates/css/*.css') - expected_messages.append('libsass lms/static/certificates/sass') + def setUp(self): + super().setUp() + self.maxDiff = None + os.environ['NO_PREREQ_INSTALL'] = 'true' + tasks.environment = tasks.Environment() - if "studio" in system: - expected_messages.append('mkdir_p ' + repr(TEST_THEME_DIR / 'cms/static/css')) - if force: - expected_messages.append( - f'rm -rf {str(TEST_THEME_DIR)}/cms/static/css/*.css' - ) - expected_messages.append('libsass cms/static/sass') - expected_messages.append( - 'rtlcss {test_theme_dir}/cms/static/css/bootstrap/studio-main.css' - ' {test_theme_dir}/cms/static/css/bootstrap/studio-main-rtl.css'.format( - test_theme_dir=str(TEST_THEME_DIR), - ) - ) - if force: - expected_messages.append( - f'rm -rf {str(TEST_THEME_DIR)}/cms/static/css/*.css' - ) - expected_messages.append( - f'libsass {str(TEST_THEME_DIR)}/cms/static/sass' - ) - if force: - expected_messages.append('rm -rf cms/static/css/*.css') - expected_messages.append('libsass cms/static/sass') - expected_messages.append( - 'rtlcss cms/static/css/bootstrap/studio-main.css cms/static/css/bootstrap/studio-main-rtl.css' - ) - - assert len(self.task_messages) == len(expected_messages) - - -@ddt.ddt -class TestCollectAssets(PaverTestCase): - """ - Test the collectstatic process call. - - ddt data is organized thusly: - * debug: whether or not collect_assets is called with the debug flag - * specified_log_location: used when collect_assets is called with a specific - log location for collectstatic output - * expected_log_location: the expected string to be used for piping collectstatic logs - """ - - @ddt.data( - [{ - "collect_log_args": {}, # Test for default behavior - "expected_log_location": "> /dev/null" - }], - [{ - "collect_log_args": {COLLECTSTATIC_LOG_DIR_ARG: "/foo/bar"}, - "expected_log_location": "> /foo/bar/lms-collectstatic.log" - }], # can use specified log location - [{ - "systems": ["lms", "cms"], - "collect_log_args": {}, - "expected_log_location": "> /dev/null" - }], # multiple systems can be called - ) - @ddt.unpack - def test_collect_assets(self, options): - """ - Ensure commands sent to the environment for collect_assets are as expected - """ - specified_log_loc = options.get("collect_log_args", {}) - specified_log_dict = specified_log_loc - log_loc = options.get("expected_log_location", "> /dev/null") - systems = options.get("systems", ["lms"]) - if specified_log_loc is None: - collect_assets( - systems, - Env.DEVSTACK_SETTINGS - ) - else: - collect_assets( - systems, - Env.DEVSTACK_SETTINGS, - **specified_log_dict - ) - self._assert_correct_messages(log_location=log_loc, systems=systems) - - def test_collect_assets_debug(self): - """ - When the method is called specifically with None for the collectstatic log dir, then - it should run in debug mode and pipe to console. - """ - expected_log_loc = "" - systems = ["lms"] - kwargs = {COLLECTSTATIC_LOG_DIR_ARG: None} - collect_assets(systems, Env.DEVSTACK_SETTINGS, **kwargs) - self._assert_correct_messages(log_location=expected_log_loc, systems=systems) - - def _assert_correct_messages(self, log_location, systems): - """ - Asserts that the expected commands were run. - - We just extract the pieces we care about here instead of specifying an - exact command, so that small arg changes don't break this test. - """ - for i, sys in enumerate(systems): - msg = self.task_messages[i] - assert msg.startswith(f'python manage.py {sys}') - assert ' collectstatic ' in msg - assert f'--settings={Env.DEVSTACK_SETTINGS}' in msg - assert msg.endswith(f' {log_location}') - - -@ddt.ddt -class TestUpdateAssetsTask(PaverTestCase): - """ - These are nearly end-to-end tests, because they observe output from the commandline request, - but do not actually execute the commandline on the terminal/process - """ + def tearDown(self): + super().tearDown() + del os.environ['NO_PREREQ_INSTALL'] @ddt.data( - [{"expected_substring": "> /dev/null"}], # go to /dev/null by default - [{"cmd_args": ["--debug"], "expected_substring": "collectstatic"}] # TODO: make this regex + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={}, + expected=["npm run compile-sass --"], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"system": "lms,studio"}, + expected=["npm run compile-sass --"], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"debug": True}, + expected=["npm run compile-sass-dev --"], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"system": "lms"}, + expected=["npm run compile-sass -- --skip-cms"], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"system": "studio"}, + expected=["npm run compile-sass -- --skip-lms"], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"system": "cms", "theme_dirs": f"{REPO_ROOT}/common/test,{REPO_ROOT}/themes"}, + expected=[ + "npm run compile-sass -- --skip-lms " + + f"--theme-dir {REPO_ROOT}/common/test --theme-dir {REPO_ROOT}/themes" + ], + ), + dict( + task_name='pavelib.assets.compile_sass', + args=[], + kwargs={"theme_dirs": f"{REPO_ROOT}/common/test,{REPO_ROOT}/themes", "themes": "red-theme,test-theme"}, + expected=[ + "npm run compile-sass -- " + + f"--theme-dir {REPO_ROOT}/common/test --theme-dir {REPO_ROOT}/themes " + + "--theme red-theme --theme test-theme" + ], + ), + dict( + task_name='pavelib.assets.update_assets', + args=["lms", "studio", "--settings=fake.settings"], + kwargs={}, + expected=[ + ( + "WEBPACK_CONFIG_PATH=webpack.fake.config.js " + + "NODE_ENV=production " + + "STATIC_ROOT_LMS=/fake/lms/staticfiles " + + "STATIC_ROOT_CMS=/fake/cms/staticfiles " + + 'JS_ENV_EXTRA_CONFIG=' + + + '"{\\"key1\\": [true, false], \\"key2\\": {\\"key2.1\\": 1369, \\"key2.2\\": \\"1369\\"}}" ' + + "npm run webpack" + ), + "python manage.py lms --settings=fake.settings compile_sass lms ", + "python manage.py cms --settings=fake.settings compile_sass cms ", + ( + "( ./manage.py lms --settings=fake.settings collectstatic --noinput ) && " + + "( ./manage.py cms --settings=fake.settings collectstatic --noinput )" + ), + ], + ), ) @ddt.unpack - def test_update_assets_task_collectstatic_log_arg(self, options): - """ - Scoped test that only looks at what is passed to the collecstatic options - """ - cmd_args = options.get("cmd_args", [""]) - expected_substring = options.get("expected_substring", None) - call_task('pavelib.assets.update_assets', args=cmd_args) - self.assertTrue( - self._is_substring_in_list(self.task_messages, expected_substring), - msg=f"{expected_substring} not found in messages" - ) - - def _is_substring_in_list(self, messages_list, expected_substring): - """ - Return true a given string is somewhere in a list of strings - """ - for message in messages_list: - if expected_substring in message: - return True - return False + @patch.object(pavelib.assets, 'sh') + def test_paver_assets_wrapper_invokes_new_commands(self, mock_sh, task_name, args, kwargs, expected): + paver.easy.call_task(task_name, args=args, options=kwargs) + assert [call_args[0] for (call_args, call_kwargs) in mock_sh.call_args_list] == expected diff --git a/scripts/compile_sass.py b/scripts/compile_sass.py index 41a2d56b3bda..ec1efee24d2b 100755 --- a/scripts/compile_sass.py +++ b/scripts/compile_sass.py @@ -67,14 +67,12 @@ "theme_dirs", metavar="PATH", multiple=True, - envvar="EDX_PLATFORM_THEME_DIRS", - type=click.Path( - exists=True, file_okay=False, readable=True, writable=True, path_type=Path - ), + envvar="COMPREHENSIVE_THEME_DIRS", + type=click.Path(path_type=Path), help=( "Consider sub-dirs of PATH as themes. " "Multiple theme dirs are accepted. " - "If none are provided, we look at colon-separated paths on the EDX_PLATFORM_THEME_DIRS env var." + "If none are provided, we look at colon-separated paths on the COMPREHENSIVE_THEME_DIRS env var." ), ) @click.option( diff --git a/scripts/watch_sass.sh b/scripts/watch_sass.sh index 68d4b1f471a5..e88cb9e3cd6f 100755 --- a/scripts/watch_sass.sh +++ b/scripts/watch_sass.sh @@ -4,11 +4,11 @@ # Invoke from repo root as `npm run watch-sass`. # By default, only watches default Sass. -# To watch themes too, provide colon-separated paths in the EDX_PLATFORM_THEME_DIRS environment variable. +# To watch themes too, provide colon-separated paths in the COMPREHENSIVE_THEME_DIRS environment variable. # Each path will be treated as a "theme dir", which means that every immediate child directory is watchable as a theme. # For example: # -# EDX_PLATFORM_THEME_DIRS=/openedx/themes:./themes npm run watch-sass +# COMPREHENSIVE_THEME_DIRS=/openedx/themes:./themes npm run watch-sass # # would watch default Sass as well as /openedx/themes/indigo, /openedx/themes/mytheme, ./themes/red-theme, etc. From 2a14741bbe58c283776799c39d0f946c8c6d586a Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 6 May 2024 15:26:07 -0400 Subject: [PATCH 2/2] fix: fall back to settings.COMPREHENSIVE_THEME_DIRS correctly There was a logical error in the compile_sass management command: instead of falling back to settings.COMPREHENSIVE_THEME_DIRS when --theme-dirs was *None or missing*, we only fell back to it when --theme-dirs was *missing*. This caused theme compilation to be skipped when COMREHENSIVE_THEME_DIRS *is not set* in the environment, even though settings.COMPREHENSIVE_THEME_DIRS *is set* in Django settings, which is currently the case for edx.org. --- .../djangoapps/theming/management/commands/compile_sass.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/theming/management/commands/compile_sass.py b/openedx/core/djangoapps/theming/management/commands/compile_sass.py index eecbe684b528..fbfdd2f222a4 100644 --- a/openedx/core/djangoapps/theming/management/commands/compile_sass.py +++ b/openedx/core/djangoapps/theming/management/commands/compile_sass.py @@ -73,8 +73,8 @@ def handle(self, *args, **options): {"lms": "lms", "cms": "cms", "studio": "cms"}[sys] for sys in options.get("system", ["lms", "cms"]) ) - theme_dirs = options.get("theme_dirs", settings.COMPREHENSIVE_THEME_DIRS) or [] - themes_option = options.get("themes", []) # '[]' means 'all' + theme_dirs = options.get("theme_dirs") or settings.COMPREHENSIVE_THEME_DIRS or [] + themes_option = options.get("themes") or [] # '[]' means 'all' if not settings.ENABLE_COMPREHENSIVE_THEMING: compile_themes = False themes = []