From 53ffd5cb6f73c3e7d99efcdd93393808a50a51f7 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Wed, 10 Apr 2024 08:47:14 -0400 Subject: [PATCH 1/5] revert: revert: build: remove dependency on Python `sass` module This reverts commit a27cda2fd96935843e2e81e3b302b9b207cd9e95. --- scripts/compile_sass.py | 124 +++++++++++++++++++++++++++++++--------- 1 file changed, 96 insertions(+), 28 deletions(-) diff --git a/scripts/compile_sass.py b/scripts/compile_sass.py index 5698b2eaa6cb..97f800a34fe7 100755 --- a/scripts/compile_sass.py +++ b/scripts/compile_sass.py @@ -42,7 +42,9 @@ from __future__ import annotations import glob +import os import subprocess +import sys from pathlib import Path import click @@ -153,42 +155,108 @@ def main( def compile_sass_dir( message: str, - source: Path, - dest: Path, + source_root: Path, + target_root: Path, includes: list[Path], tolerate_missing: bool = False, ) -> None: """ Compile a directory of Sass into a target CSS directory, and generate any missing RTL CSS. - Structure of source dir is mirrored in target dir. + + IMPLEMENTATION NOTES: + ===================== + + libsass is a C++ library for compiling Sass (ref: https://github.com/sass/libsass). + + libsass-python is a small PyPI package wrapping libsass, including: + * The `_sass` module, which provides direct Python bindings for the C++ library. + (ref: https://github.com/sass/libsass-python/blob/0.10.0/pysass.cpp) + * The `sass` module, which adds some friendly Pythonic wrapper functions around `_sass`, + notably `sass.compile_dirname(...)`. + (ref: https://github.com/sass/libsass-python/blob/0.10.0/sass.py#L198-L201) + + Our legacy Sass code only works with a super old version of libsass (3.3.2,) which is provided to us by a super + old version of libsass-python (0.10.0). In this super old libsass-python version: + * the `sass` module DOESN'T support Python 3.11+, but + * the `_sass` module DOES support Python 3.11+. + + Upgrading our Sass to work with newer a libsass version would be arduous and would potentially break + comprehensive themes, so we don't want to do that. Forking libsass-python at v0.10.0 and adding Python 3.11+ + support would mean adding another repo to the openedx org. Rather than do either of those, we've decided to + hack around the problem by just reimplementing what we need of `sass.compile_dirname` here, directly on top + of the `_sass` C++ binding module. + + Eventually, we may eschew libsass-python altogether by switching to SassC@3.3.2, a direct CLI for libsass@3.3.2. + (ref: https://github.com/sass/sassc). This would be nice because it would allow us to remove Python from the + Sass build pipeline entirely. However, it would mean explicitly compiling & installing both libsass and SassC + within the edx-platform Dockerfile, which has its own drawbacks. """ - use_dev_settings = NORMALIZED_ENVS[env] == "development" + # Constants from libsass-python + SASS_STYLE_NESTED = 0 + _SASS_STYLE_EXPANDED = 1 + _SASS_STYLE_COMPACT = 2 + SASS_STYLE_COMPRESSED = 3 + SASS_COMMENTS_NONE = 0 + SASS_COMMENTS_LINE_NUMBERS = 1 + + # Defaults from libass-python + precision = 5 + source_map_filename = None + custom_functions = [] + importers = None + + use_dev_settings: bool = NORMALIZED_ENVS[env] == "development" + fs_encoding: str = sys.getfilesystemencoding() or sys.getdefaultencoding() + output_style: int = SASS_STYLE_NESTED if use_dev_settings else SASS_STYLE_COMPRESSED + source_comments: int = SASS_COMMENTS_LINE_NUMBERS if use_dev_settings else SASS_COMMENTS_NONE + include_paths: bytes = os.pathsep.join(str(include) for include in includes).encode(fs_encoding) + click.secho(f" {message}...", fg="cyan") - click.secho(f" Source: {source}") - click.secho(f" Target: {dest}") - if not source.is_dir(): + click.secho(f" Source: {source_root}") + click.secho(f" Target: {target_root}") + if not source_root.is_dir(): if tolerate_missing: - click.secho(f" Skipped because source directory does not exist.", fg="yellow") + click.secho(f" Skipped because source directory does not exist.", fg="yellow") return else: - raise FileNotFoundError(f"missing Sass source dir: {source}") - click.echo(f" Include paths:") + raise FileNotFoundError(f"missing Sass source dir: {source_root}") + click.echo(f" Include paths:") for include in includes: - click.echo(f" {include}") - if not dry: - # Import sass late so that this script can be dry-run without installing - # libsass, which takes a while as it must be compiled from its C source. - import sass - - dest.mkdir(parents=True, exist_ok=True) - sass.compile( - dirname=(str(source), str(dest)), - include_paths=[str(include_path) for include_path in includes], - source_comments=use_dev_settings, - output_style=("nested" if use_dev_settings else "compressed"), - ) - click.secho(f" Compiled.", fg="green") + click.echo(f" {include}") + + click.echo(f" Files:") + for dirpath, _, filenames in os.walk(str(source_root)): + for filename in filenames: + if filename.startswith('_'): + continue + if not filename.endswith(('.scss', '.sass')): + continue + source = Path(dirpath) / filename + target = (target_root / source.relative_to(source_root)).with_suffix('.css') + click.echo(f" {source} -> {target}") + if not dry: + # Import _sass late so that this script can be dry-run without installing + # libsass, which takes a while as it must be compiled from its C source. + from _sass import compile_filename # pylint: disable=protected-access + success, output, _ = compile_filename( + str(source).encode(fs_encoding), + output_style, + source_comments, + include_paths, + precision, + source_map_filename, + custom_functions, + importers, + ) + output_text = output.decode('utf-8') + if not success: + raise Exception(f"Failed to compile {source}: {output_text}") + target.parent.mkdir(parents=True, exist_ok=True) + with open(target, 'w', encoding="utf-8") as target_file: + target_file.write(output_text) + + click.secho(f" Done.", fg="green") # For Sass files without explicit RTL versions, generate # an RTL version of the CSS using the rtlcss library. for sass_path in glob.glob(str(source) + "/**/*.scss"): @@ -201,16 +269,16 @@ def compile_sass_dir( if Path(sass_path.replace(".scss", "-rtl.scss")).exists(): # Don't generate RTL CSS if there is an explicit Sass version for RTL continue - click.echo(" Generating missing right-to-left CSS:") + click.echo(" Generating missing right-to-left CSS:") source_css_file = sass_path.replace(str(source), str(dest)).replace( ".scss", ".css" ) target_css_file = source_css_file.replace(".css", "-rtl.css") - click.echo(f" Source: {source_css_file}") - click.echo(f" Target: {target_css_file}") + click.echo(f" Source: {source_css_file}") + click.echo(f" Target: {target_css_file}") if not dry: subprocess.run(["rtlcss", source_css_file, target_css_file]) - click.secho(" Generated.", fg="green") + click.secho(" Generated.", fg="green") # Information click.secho(f"USING ENV: {NORMALIZED_ENVS[env]}", fg="blue") From eab3819a95decb3baa0d90e2704b0c4de01834ec Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Wed, 10 Apr 2024 08:52:27 -0400 Subject: [PATCH 2/5] fix: NameErrors in compile_sass.py Fixes three NameErrors which were introduced by the previous commit, leading to it being reverted. These NameErrors slipped through because the script was not tested on any themes that had zero SCSS overrides. --- scripts/compile_sass.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/compile_sass.py b/scripts/compile_sass.py index 97f800a34fe7..41a2d56b3bda 100755 --- a/scripts/compile_sass.py +++ b/scripts/compile_sass.py @@ -259,7 +259,7 @@ def compile_sass_dir( click.secho(f" Done.", fg="green") # For Sass files without explicit RTL versions, generate # an RTL version of the CSS using the rtlcss library. - for sass_path in glob.glob(str(source) + "/**/*.scss"): + for sass_path in glob.glob(str(source_root) + "/**/*.scss"): if Path(sass_path).name.startswith("_"): # Don't generate RTL CSS for partials continue @@ -270,7 +270,7 @@ def compile_sass_dir( # Don't generate RTL CSS if there is an explicit Sass version for RTL continue click.echo(" Generating missing right-to-left CSS:") - source_css_file = sass_path.replace(str(source), str(dest)).replace( + source_css_file = sass_path.replace(str(source_root), str(target_root)).replace( ".scss", ".css" ) target_css_file = source_css_file.replace(".css", "-rtl.css") From 8f438165498e204839fa1457945562f5fde38278 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Wed, 10 Apr 2024 13:25:57 -0400 Subject: [PATCH 3/5] test: add a dummy empty theme This will be used to protect against bugs like the on fixed in the previous commit. --- themes/empty-theme/README.rst | 7 +++++++ themes/empty-theme/cms/static/sass/.gitignore | 2 ++ themes/empty-theme/cms/templates/.gitignore | 2 ++ themes/empty-theme/lms/static/sass/.gitignore | 2 ++ themes/empty-theme/lms/templates/.gitignore | 2 ++ 5 files changed, 15 insertions(+) create mode 100644 themes/empty-theme/README.rst create mode 100644 themes/empty-theme/cms/static/sass/.gitignore create mode 100644 themes/empty-theme/cms/templates/.gitignore create mode 100644 themes/empty-theme/lms/static/sass/.gitignore create mode 100644 themes/empty-theme/lms/templates/.gitignore diff --git a/themes/empty-theme/README.rst b/themes/empty-theme/README.rst new file mode 100644 index 000000000000..541e9ac28457 --- /dev/null +++ b/themes/empty-theme/README.rst @@ -0,0 +1,7 @@ +Empty Theme +########### + +This is a dummy theme to ensure that the build succeeds on a folder that is +*structured* like a theme, but contains no actual Sass modules nor HTML +templates. Compiling and enabling this theme should be functionally and +visually equivalent to the default theme. diff --git a/themes/empty-theme/cms/static/sass/.gitignore b/themes/empty-theme/cms/static/sass/.gitignore new file mode 100644 index 000000000000..7f1d3dd29328 --- /dev/null +++ b/themes/empty-theme/cms/static/sass/.gitignore @@ -0,0 +1,2 @@ +# This empty gitignore file ensures that this otherwise-empty +# directory is included in version control. diff --git a/themes/empty-theme/cms/templates/.gitignore b/themes/empty-theme/cms/templates/.gitignore new file mode 100644 index 000000000000..7f1d3dd29328 --- /dev/null +++ b/themes/empty-theme/cms/templates/.gitignore @@ -0,0 +1,2 @@ +# This empty gitignore file ensures that this otherwise-empty +# directory is included in version control. diff --git a/themes/empty-theme/lms/static/sass/.gitignore b/themes/empty-theme/lms/static/sass/.gitignore new file mode 100644 index 000000000000..7f1d3dd29328 --- /dev/null +++ b/themes/empty-theme/lms/static/sass/.gitignore @@ -0,0 +1,2 @@ +# This empty gitignore file ensures that this otherwise-empty +# directory is included in version control. diff --git a/themes/empty-theme/lms/templates/.gitignore b/themes/empty-theme/lms/templates/.gitignore new file mode 100644 index 000000000000..7f1d3dd29328 --- /dev/null +++ b/themes/empty-theme/lms/templates/.gitignore @@ -0,0 +1,2 @@ +# This empty gitignore file ensures that this otherwise-empty +# directory is included in version control. From 94d7e144039836c277344326631e91682e6b0eb0 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Wed, 10 Apr 2024 13:28:40 -0400 Subject: [PATCH 4/5] test: compile built-in themes as part of static assets check This will help safe-guard against bugs in the Sass and Webpack build which only arise from certain uses of the legacy Comprehensive Theming system. --- .github/workflows/static-assets-check.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/static-assets-check.yml b/.github/workflows/static-assets-check.yml index 5aedfdf52f73..369151170a44 100644 --- a/.github/workflows/static-assets-check.yml +++ b/.github/workflows/static-assets-check.yml @@ -82,5 +82,5 @@ jobs: CMS_CFG: lms/envs/minimal.yml run: | - paver update_assets lms - paver update_assets cms + paver update_assets lms --theme-dirs ./themes + paver update_assets cms --theme-dirs ./themes From 101aeadc7cb350ba98fa12bcb50e859934bd3731 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 11 Apr 2024 12:04:35 -0400 Subject: [PATCH 5/5] fix: `--theme-dirs` argument to `compile_sass` management command This fixes the ability to pass custom theme directories to the management command which compiles site themes, a la: ./manage.py lms compile_sass --theme-dirs /my/custom/themes/dir The exception, which was due to a incompatible use of @lru_cache, was: File "openedx/core/djangoapps/theming/management/commands/compile_sass.py", line 93, in parse_arguments: available_themes.update({t.theme_dir_name: t for t in get_themes([theme_dir])}) TypeError: unhashable type: 'list' This has been broken since the @lru_cache decorator was added, but it wasn't noticed because: * We weren't compiling any comprehensive themes in CI. * Tutor supports compehensive theming, but not *site theming*, so it doesn't use this management command at all (site themeing == comp theming * site configuration). * Although edx.org executes this management command, it does not provide use the `--theme-dirs` argument, so the bug was not hit. --- openedx/core/djangoapps/theming/helpers.py | 8 +++++--- .../theming/management/commands/compile_sass.py | 2 +- openedx/core/djangoapps/theming/tests/test_helpers.py | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/theming/helpers.py b/openedx/core/djangoapps/theming/helpers.py index 4d1df6418082..b88bbdca4382 100644 --- a/openedx/core/djangoapps/theming/helpers.py +++ b/openedx/core/djangoapps/theming/helpers.py @@ -268,9 +268,11 @@ def get_themes(themes_dir=None): """ if not is_comprehensive_theming_enabled(): return [] - if themes_dir is None: - themes_dir = get_theme_base_dirs_unchecked() - return get_themes_unchecked(themes_dir, settings.PROJECT_ROOT) + if themes_dir: + themes_dirs = [themes_dir] + else: + themes_dirs = get_theme_base_dirs_unchecked() + return get_themes_unchecked(themes_dirs, settings.PROJECT_ROOT) def get_theme_base_dirs_unchecked(): diff --git a/openedx/core/djangoapps/theming/management/commands/compile_sass.py b/openedx/core/djangoapps/theming/management/commands/compile_sass.py index 0e193fb10174..765ef98aeac3 100644 --- a/openedx/core/djangoapps/theming/management/commands/compile_sass.py +++ b/openedx/core/djangoapps/theming/management/commands/compile_sass.py @@ -90,7 +90,7 @@ def parse_arguments(*args, **options): # pylint: disable=unused-argument 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])}) + 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()} diff --git a/openedx/core/djangoapps/theming/tests/test_helpers.py b/openedx/core/djangoapps/theming/tests/test_helpers.py index 955e723df145..95829a16b416 100644 --- a/openedx/core/djangoapps/theming/tests/test_helpers.py +++ b/openedx/core/djangoapps/theming/tests/test_helpers.py @@ -44,6 +44,7 @@ def test_get_themes(self): Theme('red-theme', 'red-theme', get_theme_base_dir('red-theme'), settings.PROJECT_ROOT), Theme('stanford-style', 'stanford-style', get_theme_base_dir('stanford-style'), settings.PROJECT_ROOT), Theme('test-theme', 'test-theme', get_theme_base_dir('test-theme'), settings.PROJECT_ROOT), + Theme('empty-theme', 'empty-theme', get_theme_base_dir('empty-theme'), settings.PROJECT_ROOT), ] actual_themes = get_themes() self.assertCountEqual(expected_themes, actual_themes)