From 237daceaf753947288aa6a99b091bae4ba3998b3 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 4 Apr 2024 10:31:20 -0400 Subject: [PATCH 1/3] Revert "build: compile/watch sass with new npm scripts" This reverts commit 24db4dfb531d240c1f5e14c3f4cbf609d3415229. --- .../core/djangoapps/theming/paver_helpers.py | 75 +++ pavelib/assets.py | 593 ++++++++++++++---- pavelib/paver_tests/test_assets.py | 81 +++ 3 files changed, 627 insertions(+), 122 deletions(-) create mode 100644 openedx/core/djangoapps/theming/paver_helpers.py diff --git a/openedx/core/djangoapps/theming/paver_helpers.py b/openedx/core/djangoapps/theming/paver_helpers.py new file mode 100644 index 000000000000..b481d0f1b1fd --- /dev/null +++ b/openedx/core/djangoapps/theming/paver_helpers.py @@ -0,0 +1,75 @@ +""" +This file contains helpers for paver commands, Django is not initialized in paver commands. +So, django settings, models etc. can not be used here. +""" + + +import os + +from path import Path + + +def get_theme_paths(themes, theme_dirs): + """ + get absolute path for all the given themes, if a theme is no found + at multiple places than all paths for the theme will be included. + If a theme is not found anywhere then theme will be skipped with + an error message printed on the console. + + If themes is 'None' then all themes in given dirs are returned. + + Args: + themes (list): list of all theme names + theme_dirs (list): list of base dirs that contain themes + Returns: + list of absolute paths to themes. + """ + theme_paths = [] + + for theme in themes: + theme_base_dirs = get_theme_base_dirs(theme, theme_dirs) + if not theme_base_dirs: + print(( + "\033[91m\nSkipping '{theme}': \n" + "Theme ({theme}) not found in any of the theme dirs ({theme_dirs}). \033[00m".format( + theme=theme, + theme_dirs=", ".join(theme_dirs) + ), + )) + theme_paths.extend(theme_base_dirs) + + return theme_paths + + +def get_theme_base_dirs(theme, theme_dirs): + """ + Get all base dirs where the given theme can be found. + + Args: + theme (str): name of the theme to find + theme_dirs (list): list of all base dirs where the given theme could be found + + Returns: + list of all the dirs for the goven theme + """ + theme_paths = [] + for _dir in theme_dirs: + for dir_name in {theme}.intersection(os.listdir(_dir)): + if is_theme_dir(Path(_dir) / dir_name): + theme_paths.append(Path(_dir) / dir_name) + return theme_paths + + +def is_theme_dir(_dir): + """ + Returns true if given dir contains theme overrides. + A theme dir must have subdirectory 'lms' or 'cms' or both. + + Args: + _dir: directory path to check for a theme + + Returns: + Returns true if given dir is a theme directory. + """ + theme_sub_directories = {'lms', 'cms'} + return bool(os.path.isdir(_dir) and theme_sub_directories.intersection(os.listdir(_dir))) diff --git a/pavelib/assets.py b/pavelib/assets.py index fd420e42a8c7..8f950fe3f647 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -2,18 +2,23 @@ Asset compilation and collection. """ + import argparse import glob import json -import shlex +import os import traceback +from datetime import datetime from functools import wraps from threading import Timer from paver import tasks -from paver.easy import call_task, cmdopts, consume_args, needs, no_help, sh, task +from paver.easy import call_task, cmdopts, consume_args, needs, no_help, path, 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 +from watchdog.observers.api import DEFAULT_OBSERVER_TIMEOUT + +from openedx.core.djangoapps.theming.paver_helpers import get_theme_paths from .utils.cmd import cmd, django_cmd from .utils.envs import Env @@ -33,6 +38,19 @@ 'studio': CMS } +# Common lookup paths that are added to the lookup paths for all sass compilations +COMMON_LOOKUP_PATHS = [ + path("common/static"), + path("common/static/sass"), + path('node_modules/@edx'), + path('node_modules'), +] + +# system specific lookup path additions, add sass dirs if one system depends on the sass files for other systems +SASS_LOOKUP_DEPENDENCIES = { + 'cms': [path('lms') / 'static' / 'sass' / 'partials', ], +} + # Collectstatic log directory setting COLLECTSTATIC_LOG_DIR_ARG = 'collect_log_dir' @@ -40,6 +58,242 @@ WEBPACK_COMMAND = 'STATIC_ROOT_LMS={static_root_lms} STATIC_ROOT_CMS={static_root_cms} $(npm bin)/webpack {options}' +def get_sass_directories(system, theme_dir=None): + """ + Determine the set of SASS directories to be compiled for the specified list of system and theme + and return a list of those directories. + + Each item in the list is dict object containing the following key-value pairs. + { + "sass_source_dir": "", # directory where source sass files are present + "css_destination_dir": "", # destination where css files would be placed + "lookup_paths": [], # list of directories to be passed as lookup paths for @import resolution. + } + + if theme_dir is empty or None then return sass directories for the given system only. (i.e. lms or cms) + + :param system: name if the system for which to compile sass e.g. 'lms', 'cms' + :param theme_dir: absolute path of theme for which to compile sass files. + """ + if system not in SYSTEMS: + raise ValueError("'system' must be one of ({allowed_values})".format( + allowed_values=', '.join(list(SYSTEMS.keys()))) + ) + system = SYSTEMS[system] + + applicable_directories = [] + + if theme_dir: + # Add theme sass directories + applicable_directories.extend( + get_theme_sass_dirs(system, theme_dir) + ) + else: + # add system sass directories + applicable_directories.extend( + get_system_sass_dirs(system) + ) + + return applicable_directories + + +def get_common_sass_directories(): + """ + Determine the set of common SASS directories to be compiled for all the systems and themes. + + Each item in the returned list is dict object containing the following key-value pairs. + { + "sass_source_dir": "", # directory where source sass files are present + "css_destination_dir": "", # destination where css files would be placed + "lookup_paths": [], # list of directories to be passed as lookup paths for @import resolution. + } + """ + applicable_directories = [] + + # add common sass directories + applicable_directories.append({ + "sass_source_dir": path("common/static/sass"), + "css_destination_dir": path("common/static/css"), + "lookup_paths": COMMON_LOOKUP_PATHS, + }) + + return applicable_directories + + +def get_theme_sass_dirs(system, theme_dir): + """ + Return list of sass dirs that need to be compiled for the given theme. + + :param system: name if the system for which to compile sass e.g. 'lms', 'cms' + :param theme_dir: absolute path of theme for which to compile sass files. + """ + if system not in ('lms', 'cms'): + raise ValueError('"system" must either be "lms" or "cms"') + + dirs = [] + + system_sass_dir = path(system) / "static" / "sass" + sass_dir = theme_dir / system / "static" / "sass" + css_dir = theme_dir / system / "static" / "css" + certs_sass_dir = theme_dir / system / "static" / "certificates" / "sass" + certs_css_dir = theme_dir / system / "static" / "certificates" / "css" + builtin_xblock_sass = path("xmodule") / "assets" + + dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) + if sass_dir.isdir(): + css_dir.mkdir_p() + + # first compile lms sass files and place css in theme dir + dirs.append({ + "sass_source_dir": system_sass_dir, + "css_destination_dir": css_dir, + "lookup_paths": dependencies + [ + sass_dir / "partials", + system_sass_dir / "partials", + system_sass_dir, + ], + }) + + # now compile theme sass files and override css files generated from lms + dirs.append({ + "sass_source_dir": sass_dir, + "css_destination_dir": css_dir, + "lookup_paths": dependencies + [ + sass_dir / "partials", + system_sass_dir / "partials", + system_sass_dir, + ], + }) + + # now compile theme sass files for certificate + if system == 'lms': + dirs.append({ + "sass_source_dir": certs_sass_dir, + "css_destination_dir": certs_css_dir, + "lookup_paths": [ + sass_dir / "partials", + sass_dir + ], + }) + + # Now, finally, compile builtin XBlocks' Sass. Themes cannot override these + # Sass files directly, but they *can* modify Sass variables which will affect + # the output here. We compile all builtin XBlocks' Sass both for LMS and CMS, + # not because we expect the output to be different between LMS and CMS, but + # because only LMS/CMS-compiled Sass can be themed; common sass is not themed. + dirs.append({ + "sass_source_dir": builtin_xblock_sass, + "css_destination_dir": css_dir, + "lookup_paths": [ + # XBlock editor views may need both LMS and CMS partials. + # XBlock display views should only need LMS patials. + # In order to keep this build script simpler, though, we just + # include everything and compile everything at once. + theme_dir / "lms" / "static" / "sass" / "partials", + theme_dir / "cms" / "static" / "sass" / "partials", + path("lms") / "static" / "sass" / "partials", + path("cms") / "static" / "sass" / "partials", + path("lms") / "static" / "sass", + path("cms") / "static" / "sass", + ], + }) + + return dirs + + +def get_system_sass_dirs(system): + """ + Return list of sass dirs that need to be compiled for the given system. + + :param system: name if the system for which to compile sass e.g. 'lms', 'cms' + """ + if system not in ('lms', 'cms'): + raise ValueError('"system" must either be "lms" or "cms"') + + dirs = [] + sass_dir = path(system) / "static" / "sass" + css_dir = path(system) / "static" / "css" + builtin_xblock_sass = path("xmodule") / "assets" + + dependencies = SASS_LOOKUP_DEPENDENCIES.get(system, []) + dirs.append({ + "sass_source_dir": sass_dir, + "css_destination_dir": css_dir, + "lookup_paths": dependencies + [ + sass_dir / "partials", + sass_dir, + ], + }) + + if system == 'lms': + dirs.append({ + "sass_source_dir": path(system) / "static" / "certificates" / "sass", + "css_destination_dir": path(system) / "static" / "certificates" / "css", + "lookup_paths": [ + sass_dir / "partials", + sass_dir + ], + }) + + # See builtin_xblock_sass compilation in get_theme_sass_dirs for details. + dirs.append({ + "sass_source_dir": builtin_xblock_sass, + "css_destination_dir": css_dir, + "lookup_paths": dependencies + [ + path("lms") / "static" / "sass" / "partials", + path("cms") / "static" / "sass" / "partials", + path("lms") / "static" / "sass", + path("cms") / "static" / "sass", + ], + }) + + return dirs + + +def get_watcher_dirs(theme_dirs=None, themes=None): + """ + Return sass directories that need to be added to sass watcher. + + Example: + >> get_watcher_dirs('/edx/app/edx-platform/themes', ['red-theme']) + [ + 'common/static', + 'common/static/sass', + 'lms/static/sass', + 'lms/static/sass/partials', + '/edx/app/edxapp/edx-platform/themes/red-theme/lms/static/sass', + '/edx/app/edxapp/edx-platform/themes/red-theme/lms/static/sass/partials', + 'cms/static/sass', + 'cms/static/sass/partials', + '/edx/app/edxapp/edx-platform/themes/red-theme/cms/static/sass/partials', + ] + + Parameters: + theme_dirs (list): list of theme base directories. + themes (list): list containing names of themes + Returns: + (list): dirs that need to be added to sass watchers. + """ + dirs = [] + dirs.extend(COMMON_LOOKUP_PATHS) + if theme_dirs and themes: + # Register sass watchers for all the given themes + themes = get_theme_paths(themes=themes, theme_dirs=theme_dirs) + for theme in themes: + for _dir in get_sass_directories('lms', theme) + get_sass_directories('cms', theme): + dirs.append(_dir['sass_source_dir']) + dirs.extend(_dir['lookup_paths']) + + # Register sass watchers for lms and cms + for _dir in get_sass_directories('lms') + get_sass_directories('cms') + get_common_sass_directories(): + dirs.append(_dir['sass_source_dir']) + dirs.extend(_dir['lookup_paths']) + + # remove duplicates + dirs = list(set(dirs)) + return dirs + + def debounce(seconds=1): """ Prevents the decorated function from being called more than every `seconds` @@ -73,6 +327,7 @@ class SassWatcher(PatternMatchingEventHandler): def register(self, observer, directories): """ register files with observer + Arguments: observer (watchdog.observers.Observer): sass file observer directories (list): list of directories to be register for sass watcher. @@ -102,8 +357,8 @@ 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.'), - ('force', '', 'DEPRECATED. Full recompilation is now always forced.'), + ('debug', 'd', 'Debug mode'), + ('force', '', 'Force full compilation'), ]) @timed def compile_sass(options): @@ -141,89 +396,173 @@ def compile_sass(options): compile sass files for cms only for 'red-theme', 'stanford-style' and 'test-theme' present in '/edx/app/edxapp/edx-platform/themes' and '/edx/app/edxapp/edx-platform/common/test/'. - This is a DEPRECATED COMPATIBILITY WRAPPER. Use `npm run compile-sass` instead. """ - systems = set(get_parsed_option(options, 'system', ALL_SYSTEMS)) - command = shlex.join( - [ - "npm", - "run", - "compile-sass", - "--", - *(["--dry"] if tasks.environment.dry_run else []), - *(["--skip-lms"] if not systems & {"lms"} else []), - *(["--skip-cms"] if not systems & {"cms", "studio"} else []), - *( - arg - for theme_dir in get_parsed_option(options, 'theme_dirs', []) - for arg in ["--theme-dir", str(theme_dir)] - ), - *( - arg - for theme in get_parsed_option(options, "theme", []) - 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) + debug = options.get('debug') + force = options.get('force') + systems = get_parsed_option(options, 'system', ALL_SYSTEMS) + themes = get_parsed_option(options, 'themes', []) + theme_dirs = get_parsed_option(options, 'theme_dirs', []) + + if not theme_dirs and themes: + # We can not compile a theme sass without knowing the directory that contains the theme. + raise ValueError('theme-dirs must be provided for compiling theme sass.') + + if themes and theme_dirs: + themes = get_theme_paths(themes=themes, theme_dirs=theme_dirs) + + # Compile sass for OpenEdx theme after comprehensive themes + if None not in themes: + themes.append(None) + + timing_info = [] + dry_run = tasks.environment.dry_run + compilation_results = {'success': [], 'failure': []} + + print("\t\tStarted compiling Sass:") + + # compile common sass files + is_successful = _compile_sass('common', None, debug, force, timing_info) + if is_successful: + print("Finished compiling 'common' sass.") + compilation_results['success' if is_successful else 'failure'].append('"common" sass files.') + + for system in systems: + for theme in themes: + print("Started compiling '{system}' Sass for '{theme}'.".format(system=system, theme=theme or 'system')) + + # Compile sass files + is_successful = _compile_sass( + system=system, + theme=path(theme) if theme else None, + debug=debug, + force=force, + timing_info=timing_info + ) + + if is_successful: + print("Finished compiling '{system}' Sass for '{theme}'.".format( + system=system, theme=theme or 'system' + )) + + compilation_results['success' if is_successful else 'failure'].append('{system} sass for {theme}.'.format( + system=system, theme=theme or 'system', + )) + + print("\t\tFinished compiling Sass:") + if not dry_run: + for sass_dir, css_dir, duration in timing_info: + print(f">> {sass_dir} -> {css_dir} in {duration}s") + if compilation_results['success']: + print("\033[92m\nSuccessful compilations:\n--- " + "\n--- ".join(compilation_results['success']) + "\n\033[00m") + if compilation_results['failure']: + print("\033[91m\nFailed compilations:\n--- " + "\n--- ".join(compilation_results['failure']) + "\n\033[00m") -def _compile_sass(system, theme, _debug, _force, _timing_info): + +def _compile_sass(system, theme, debug, force, timing_info): """ - This is a DEPRECATED COMPATIBILITY WRAPPER + Compile sass files for the given system and theme. - It exists to ease the transition for Tutor in Redwood, which directly imported and used this function. + :param system: system to compile sass for e.g. 'lms', 'cms', 'common' + :param theme: absolute path of the theme to compile sass for. + :param debug: boolean showing whether to display source comments in resulted css + :param force: boolean showing whether to remove existing css files before generating new files + :param timing_info: list variable to keep track of timing for sass compilation """ - command = shlex.join( - [ - "npm", - "run", - "compile-sass", - "--", - *(["--dry"] if tasks.environment.dry_run 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) + + # Note: import sass only when it is needed and not at the top of the file. + # This allows other paver commands to operate even without libsass being + # installed. In particular, this allows the install_prereqs command to be + # used to install the dependency. + import sass + if system == "common": + sass_dirs = get_common_sass_directories() + else: + sass_dirs = get_sass_directories(system, theme) + + dry_run = tasks.environment.dry_run + + # determine css out put style and source comments enabling + if debug: + source_comments = True + output_style = 'nested' + else: + source_comments = False + output_style = 'compressed' + + for dirs in sass_dirs: + start = datetime.now() + css_dir = dirs['css_destination_dir'] + sass_source_dir = dirs['sass_source_dir'] + lookup_paths = dirs['lookup_paths'] + + if not sass_source_dir.isdir(): + print("\033[91m Sass dir '{dir}' does not exists, skipping sass compilation for '{theme}' \033[00m".format( + dir=sass_source_dir, theme=theme or system, + )) + # theme doesn't override sass directory, so skip it + continue + + if force: + if dry_run: + tasks.environment.info("rm -rf {css_dir}/*.css".format( + css_dir=css_dir, + )) + else: + sh(f"rm -rf {css_dir}/*.css") + + all_lookup_paths = COMMON_LOOKUP_PATHS + lookup_paths + print(f"Compiling Sass: {sass_source_dir} -> {css_dir}") + for lookup_path in all_lookup_paths: + print(f" with Sass lookup path: {lookup_path}") + if dry_run: + tasks.environment.info("libsass {sass_dir}".format( + sass_dir=sass_source_dir, + )) + else: + sass.compile( + dirname=(sass_source_dir, css_dir), + include_paths=all_lookup_paths, + source_comments=source_comments, + output_style=output_style, + ) + + # For Sass files without explicit RTL versions, generate + # an RTL version of the CSS using the rtlcss library. + for sass_file in glob.glob(sass_source_dir + '/**/*.scss'): + if should_generate_rtl_css_file(sass_file): + source_css_file = sass_file.replace(sass_source_dir, css_dir).replace('.scss', '.css') + target_css_file = source_css_file.replace('.css', '-rtl.css') + sh("rtlcss {source_file} {target_file}".format( + source_file=source_css_file, + target_file=target_css_file, + )) + + # Capture the time taken + if not dry_run: + duration = datetime.now() - start + timing_info.append((sass_source_dir, css_dir, duration)) + return True + + +def should_generate_rtl_css_file(sass_file): + """ + Returns true if a Sass file should have an RTL version generated. + """ + # Don't generate RTL CSS for partials + if path(sass_file).name.startswith('_'): + return False + + # Don't generate RTL CSS if the file is itself an RTL version + if sass_file.endswith('-rtl.scss'): + return False + + # Don't generate RTL CSS if there is an explicit Sass version for RTL + rtl_sass_file = path(sass_file.replace('.scss', '-rtl.scss')) + if rtl_sass_file.exists(): + return False + + return True def process_npm_assets(): @@ -243,6 +582,18 @@ def process_xmodule_assets(): print("\t\tWhen paver is removed from edx-platform, this step will not replaced.") +def restart_django_servers(): + """ + Restart the django server. + + `$ touch` makes the Django file watcher thinks that something has changed, therefore + it restarts the server. + """ + sh(cmd( + "touch", 'lms/urls.py', 'cms/urls.py', + )) + + def collect_assets(systems, settings, **kwargs): """ Collect static assets, including Django pipeline processing. @@ -393,57 +744,55 @@ def listfy(data): @task @cmdopts([ - ('background', 'b', 'DEPRECATED. Use shell tools like & to run in background if needed.'), - ('settings=', 's', "DEPRECATED. Django is not longer invoked to compile JS/Sass."), + ('background', 'b', 'Background mode'), + ('settings=', 's', "Django settings (defaults to devstack)"), ('theme-dirs=', '-td', 'The themes dir containing all themes (defaults to None)'), - ('themes=', '-t', 'DEPRECATED. All themes in --theme-dirs are now watched.'), - ('wait=', '-w', 'DEPRECATED. Watchdog\'s default wait time is now used.'), + ('themes=', '-t', 'The themes to add sass watchers for (defaults to None)'), + ('wait=', '-w', 'How long to pause between filesystem scans.') ]) @timed def watch_assets(options): """ Watch for changes to asset files, and regenerate js/css - - This is a DEPRECATED COMPATIBILITY WRAPPER. Use `npm run watch` instead. """ # Don't watch assets when performing a dry run if tasks.environment.dry_run: return - theme_dirs = ':'.join(get_parsed_option(options, 'theme_dirs', [])) - command = shlex.join( - [ - *( - ["env", f"EDX_PLATFORM_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) + settings = getattr(options, 'settings', Env.DEVSTACK_SETTINGS) + + themes = get_parsed_option(options, 'themes') + theme_dirs = get_parsed_option(options, 'theme_dirs', []) + + default_wait = [str(DEFAULT_OBSERVER_TIMEOUT)] + wait = float(get_parsed_option(options, 'wait', default_wait)[0]) + + if not theme_dirs and themes: # lint-amnesty, pylint: disable=no-else-raise + # We can not add theme sass watchers without knowing the directory that contains the themes. + raise ValueError('theme-dirs must be provided for watching theme sass.') + else: + theme_dirs = [path(_dir) for _dir in theme_dirs] + + sass_directories = get_watcher_dirs(theme_dirs, themes) + observer = Observer(timeout=wait) + + SassWatcher().register(observer, sass_directories) + + print("Starting asset watcher...") + observer.start() + + # Run the Webpack file system watcher too + execute_webpack_watch(settings=settings) + + if not getattr(options, 'background', False): + # when running as a separate process, the main thread needs to loop + # in order to allow for shutdown by control-c + try: + while True: + observer.join(2) + except KeyboardInterrupt: + observer.stop() + print("\nStopped asset watcher.") @task diff --git a/pavelib/paver_tests/test_assets.py b/pavelib/paver_tests/test_assets.py index 029a8db67c4a..5722b2062478 100644 --- a/pavelib/paver_tests/test_assets.py +++ b/pavelib/paver_tests/test_assets.py @@ -8,6 +8,7 @@ import ddt import paver.tasks from paver.easy import call_task, path +from watchdog.observers import Observer from pavelib.assets import COLLECTSTATIC_LOG_DIR_ARG, collect_assets @@ -196,6 +197,86 @@ def test_compile_theme_sass(self, options): assert len(self.task_messages) == len(expected_messages) +class TestPaverWatchAssetTasks(TestCase): + """ + Test the Paver watch asset tasks. + """ + + def setUp(self): + self.expected_sass_directories = [ + path('common/static/sass'), + path('common/static'), + path('node_modules/@edx'), + path('node_modules'), + path('lms/static/sass/partials'), + path('lms/static/sass'), + path('lms/static/certificates/sass'), + path('cms/static/sass'), + path('cms/static/sass/partials'), + ] + + # Reset the options that paver stores in a global variable (thus polluting tests) + if 'pavelib.assets.watch_assets' in paver.tasks.environment.options: + del paver.tasks.environment.options['pavelib.assets.watch_assets'] + + super().setUp() + + def tearDown(self): + self.expected_sass_directories = [] + super().tearDown() + + def test_watch_assets(self): + """ + Test the "compile_sass" task. + """ + with patch('pavelib.assets.SassWatcher.register') as mock_register: + with patch('pavelib.assets.Observer.start'): + with patch('pavelib.assets.execute_webpack_watch') as mock_webpack: + call_task( + 'pavelib.assets.watch_assets', + options={"background": True}, + ) + assert mock_register.call_count == 2 + assert mock_webpack.call_count == 1 + + sass_watcher_args = mock_register.call_args_list[0][0] + + assert isinstance(sass_watcher_args[0], Observer) + assert isinstance(sass_watcher_args[1], list) + assert len(sass_watcher_args[1]) == len(self.expected_sass_directories) + + def test_watch_theme_assets(self): + """ + Test the Paver watch asset tasks with theming enabled. + """ + self.expected_sass_directories.extend([ + path(TEST_THEME_DIR) / 'lms/static/sass', + path(TEST_THEME_DIR) / 'lms/static/sass/partials', + path(TEST_THEME_DIR) / 'lms/static/certificates/sass', + path(TEST_THEME_DIR) / 'cms/static/sass', + path(TEST_THEME_DIR) / 'cms/static/sass/partials', + ]) + + with patch('pavelib.assets.SassWatcher.register') as mock_register: + with patch('pavelib.assets.Observer.start'): + with patch('pavelib.assets.execute_webpack_watch') as mock_webpack: + call_task( + 'pavelib.assets.watch_assets', + options={ + "background": True, + "theme_dirs": [TEST_THEME_DIR.dirname()], + "themes": [TEST_THEME_DIR.basename()] + }, + ) + assert mock_register.call_count == 2 + assert mock_webpack.call_count == 1 + + sass_watcher_args = mock_register.call_args_list[0][0] + assert isinstance(sass_watcher_args[0], Observer) + assert isinstance(sass_watcher_args[1], list) + assert len(sass_watcher_args[1]) == len(self.expected_sass_directories) + + @ddt.ddt class TestCollectAssets(PaverTestCase): """ From 725d19e29121638db3ea8bff1b416ba48ca41688 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 4 Apr 2024 10:31:20 -0400 Subject: [PATCH 2/3] Revert "docs: npm scripts for static assets are no longer experimental :)" This reverts commit bd82b1d75a1ed48d94e98ef236d62383e42df75b. --- package.json | 6 +++--- scripts/compile_sass.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 5b4f06527685..341673136281 100644 --- a/package.json +++ b/package.json @@ -4,13 +4,13 @@ "repository": "https://github.com/openedx/edx-platform", "scripts": { "postinstall": "scripts/copy-node-modules.sh", - "build": "npm run webpack && npm run compile-sass", - "build-dev": "npm run webpack-dev && npm run compile-sass-dev", + "build": "echo 'WARNING: `npm run build` in edx-platform is experimental. Use at your own risk.' && npm run webpack && npm run compile-sass", + "build-dev": "echo 'WARNING: `npm run build-dev` in edx-platform is experimental. Use at your own risk.' && npm run webpack-dev && npm run compile-sass-dev", "webpack": "NODE_ENV=${NODE_ENV:-production} \"$(npm bin)/webpack\" --config=${WEBPACK_CONFIG_PATH:-webpack.prod.config.js}", "webpack-dev": "NODE_ENV=development \"$(npm bin)/webpack\" --config=webpack.dev.config.js", "compile-sass": "scripts/compile_sass.py --env=${NODE_ENV:-production}", "compile-sass-dev": "scripts/compile_sass.py --env=development", - "watch": "{ npm run watch-webpack& npm run watch-sass& } && sleep infinity", + "watch": "echo 'WARNING: `npm run watch` in edx-platform is experimental. Use at your own risk.' && { npm run watch-webpack& npm run watch-sass& } && sleep infinity", "watch-webpack": "npm run webpack-dev -- --watch", "watch-sass": "scripts/watch_sass.sh" }, diff --git a/scripts/compile_sass.py b/scripts/compile_sass.py index 5698b2eaa6cb..8a21f718fefe 100755 --- a/scripts/compile_sass.py +++ b/scripts/compile_sass.py @@ -219,6 +219,7 @@ def compile_sass_dir( click.echo() # Warnings + click.secho("WARNING: `npm run compile-sass` is experimental. Use at your own risk.", fg="yellow", bold=True) if show_help: click.echo(context.get_help()) return From f392ae1d702fd91d840b300cd388bfed748cb176 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 4 Apr 2024 10:31:20 -0400 Subject: [PATCH 3/3] Revert "fix: simplify `npm run watch-sass` so that it is more reliable" This reverts commit 3b8d018e4b6524a7cb1d6439557fd5c33a2ec67a. --- scripts/watch_sass.sh | 101 +++++++++++++++++++++++++++++------------- 1 file changed, 71 insertions(+), 30 deletions(-) diff --git a/scripts/watch_sass.sh b/scripts/watch_sass.sh index 68d4b1f471a5..dee0d88f6215 100755 --- a/scripts/watch_sass.sh +++ b/scripts/watch_sass.sh @@ -2,6 +2,7 @@ # Wait for changes to Sass, and recompile. # Invoke from repo root as `npm run watch-sass`. +# This script tries to recompile the minimal set of Sass for any given change. # By default, only watches default Sass. # To watch themes too, provide colon-separated paths in the EDX_PLATFORM_THEME_DIRS environment variable. @@ -45,24 +46,34 @@ echo_quoted_cmd() { start_sass_watch() { # Start a watch for .scss files in a particular dir. Run in the background. - # Usage: start_sass_watch NAME_FOR_LOGGING HANDLER_COMMAND WATCH_ROOT_PATHS... + # start_sass_watch NAME_FOR_LOGGING WATCH_ROOT_PATH HANDLER_COMMAND local name="$1" - local handler="$2" - shift 2 - local paths=("$@") - section "Starting Sass watchers for $name:" + local path="$2" + local handler="$3" + log "Starting watcher for $name:" # Note: --drop means that we should ignore any change events that happen during recompilation. # This is good because it means that if you edit 3 files, we won't fire off three simultaneous compiles. # It's not perfect, though, because if you change 3 files, only the first one will trigger a recompile, # so depending on the timing, your latest changes may or may not be picked up. We accept this as a reasonable # tradeoff. Some watcher libraries are smarter than watchdog, in that they wait until the filesystem "settles" # before firing off a the recompilation. This sounds nice but did not seem worth the effort for legacy assets. - local watch_cmd=(watchmedo shell-command -v --patterns '*.scss' --recursive --drop --command "$handler" "${paths[@]}") + local watch_cmd=(watchmedo shell-command -v --patterns '*.scss' --recursive --drop --command "$handler" "$path") echo_quoted_cmd "${watch_cmd[@]}" "${watch_cmd[@]}" & } -# Verify execution environment. +clean_up() { + # Kill all background processes we started. + # Since they're all 'watchmedo' instances, we can just use killall. + log "Stopping all watchers:" + local stop_cmd=(killall watchmedo) + echo_quoted_cmd "${stop_cmd[@]}" + "${stop_cmd[@]}" || true + log "Watchers stopped." +} + +warning "'npm run watch-sass' in edx-platform is experimental. Use at your own risk." + if [[ ! -d common/static/sass ]] ; then error 'This command must be run from the root of edx-platform!' exit 1 @@ -74,51 +85,81 @@ if ! type watchmedo 1>/dev/null 2>&1 ; then exit 1 fi -# Reliably kill all child processes when script is interrupted (Ctrl+C) or otherwise terminated. -trap "exit" INT TERM -trap "kill 0" EXIT +trap clean_up EXIT -# Watch default Sass. -# If it changes, then recompile the default theme *and* all custom themes, -# as custom themes' Sass is based on the default Sass. -start_sass_watch "default theme" \ - 'npm run compile-sass-dev' \ +# Start by compiling all watched Sass right away, mirroring the behavior of webpack --watch. +section "COMPILING SASS:" +npm run compile-sass +echo +echo + +section "STARTING DEFAULT SASS WATCHERS:" + +# Changes to LMS Sass require a full recompilation, since LMS Sass can be used in CMS and in themes. +start_sass_watch "default LMS Sass" \ lms/static/sass \ + 'npm run compile-sass-dev' + +# Changes to default cert Sass only require recompilation of default cert Sass, since cert Sass +# cannot be included into LMS, CMS, or themes. +start_sass_watch "default certificate Sass" \ lms/static/certificates/sass \ + 'npm run compile-sass-dev -- --skip-cms --skip-themes' + +# Changes to CMS Sass require recompilation of default & themed CMS Sass, but not LMS Sass. +start_sass_watch "default CMS Sass" \ cms/static/sass \ + 'npm run compile-sass-dev -- --skip-lms' + +# Sass changes in common, node_modules, and xmodule all require full recompilations. +start_sass_watch "default common Sass" \ common/static \ + 'npm run compile-sass-dev' +start_sass_watch "node_modules Sass" \ node_modules \ - xmodule/assets + 'npm run compile-sass-dev' +start_sass_watch "builtin XBlock Sass" \ + xmodule/assets \ + 'npm run compile-sass-dev' -# Watch each theme's Sass. -# If it changes, only recompile that theme. -export IFS=":" +export IFS=";" for theme_dir in ${EDX_PLATFORM_THEME_DIRS:-} ; do for theme_path in "$theme_dir"/* ; do + theme_name="${theme_path#"$theme_dir/"}" lms_sass="$theme_path/lms/static/sass" cert_sass="$theme_path/lms/static/certificates/sass" cms_sass="$theme_path/cms/static/sass" - theme_watch_dirs=() + + if [[ -d "$lms_sass" ]] || [[ -d "$cert_sass" ]] || [[ -d "$cms_sass" ]] ; then + section "STARTING WATCHERS FOR THEME '$theme_name':" + fi + + # Changes to a theme's LMS Sass require that the full theme is recompiled, since LMS + # Sass is used in certs and CMS. if [[ -d "$lms_sass" ]] ; then - theme_watch_dirs+=("$lms_sass") + start_sass_watch "$theme_name LMS Sass" \ + "$lms_sass" \ + "npm run compile-sass-dev -- -T $theme_dir -t $theme_name --skip-default" fi + + # Changes to a theme's certs Sass only requires that its certs Sass be recompiled. if [[ -d "$cert_sass" ]] ; then - theme_watch_dirs+=("$cert_sass") + start_sass_watch "$theme_name certificate Sass" \ + "$cert_sass" \ + "npm run compile-sass-dev -- -T $theme_dir -t $theme_name --skip-default --skip-cms" fi + + # Changes to a theme's CMS Sass only requires that its CMS Sass be recompiled. if [[ -d "$cms_sass" ]] ; then - theme_watch_dirs+=("$cms_sass") - fi - # A directory is a theme if it as LMS Sass *and/or* CMS Sass *and/or* certificate Sass. - if [[ -n "${theme_watch_dirs[*]}" ]] ; then - start_sass_watch "theme '$theme_name'" \ - "npm run compile-sass-dev -- -T $theme_dir -t $theme_name --skip-default" \ - "${theme_watch_dirs[@]}" + start_sass_watch "$theme_name CMS Sass" \ + "$cms_sass" \ + "npm run compile-sass-dev -- -T $theme_dir -t $theme_name --skip-default --skip-lms" fi + done done -# Wait until interrupted/terminated. sleep infinity & echo echo "Watching Open edX LMS & CMS Sass for changes."