Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert: revert: build: remove dependency on Python sass module #34502

Merged
merged 5 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/static-assets-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
8 changes: 5 additions & 3 deletions openedx/core/djangoapps/theming/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()}
Expand Down
1 change: 1 addition & 0 deletions openedx/core/djangoapps/theming/tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
128 changes: 98 additions & 30 deletions scripts/compile_sass.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@
from __future__ import annotations

import glob
import os
import subprocess
import sys
from pathlib import Path

import click
Expand Down Expand Up @@ -153,45 +155,111 @@ 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 [email protected], a direct CLI for [email protected].
(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"):
for sass_path in glob.glob(str(source_root) + "/**/*.scss"):
if Path(sass_path).name.startswith("_"):
# Don't generate RTL CSS for partials
continue
Expand All @@ -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:")
source_css_file = sass_path.replace(str(source), str(dest)).replace(
click.echo(" Generating missing right-to-left CSS:")
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")
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")
Expand Down
7 changes: 7 additions & 0 deletions themes/empty-theme/README.rst
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 2 additions & 0 deletions themes/empty-theme/cms/static/sass/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# This empty gitignore file ensures that this otherwise-empty
# directory is included in version control.
2 changes: 2 additions & 0 deletions themes/empty-theme/cms/templates/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# This empty gitignore file ensures that this otherwise-empty
# directory is included in version control.
2 changes: 2 additions & 0 deletions themes/empty-theme/lms/static/sass/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# This empty gitignore file ensures that this otherwise-empty
# directory is included in version control.
2 changes: 2 additions & 0 deletions themes/empty-theme/lms/templates/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# This empty gitignore file ensures that this otherwise-empty
# directory is included in version control.
Loading