Revert "build: finish replacing paver assets (#34554)" (#34700)

Reverts #34554, which causes compilation of edX.org's legacy comprehensive theme to be skipped in their deployment pipeline. We have not determined the precise cause yet, but it seems like the compile_sass management command is not correctly getting the list of comprehensive theme directories from Django settings.
openedx · May 6, 2024 · 4c0284b · 4c0284b
1 parent 08323cc
commit 4c0284b
Show file tree

Hide file tree

Showing 9 changed files with 633 additions and 375 deletions.
diff --git a/cms/envs/common.py b/cms/envs/common.py
@@ -39,7 +39,6 @@
 # pylint: disable=unused-import, useless-suppression, wrong-import-order, wrong-import-position
 
 import importlib.util
-import json
 import os
 import sys
 
@@ -1276,11 +1275,14 @@
 
 # Static content
 STATIC_URL = '/static/studio/'
-STATIC_ROOT = os.environ.get('STATIC_ROOT_CMS', ENV_ROOT / 'staticfiles' / 'studio')
+STATIC_ROOT = 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
@@ -1320,16 +1322,11 @@
 EMBARGO_SITE_REDIRECT_URL = None
 
 ##### custom vendor plugin variables #####
-
-# .. 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', '{}'))
+# 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 = {}
 
 ############################### PIPELINE #######################################
 
@@ -1506,14 +1503,7 @@
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-worker-stats.json')
     }
 }
-
-# .. 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')
+WEBPACK_CONFIG_PATH = 'webpack.prod.config.js'
 
 
 ############################ SERVICE_VARIANT ##################################
@@ -2190,11 +2180,8 @@
 
 # .. setting_name: COMPREHENSIVE_THEME_DIRS
 # .. setting_default: []
-# .. 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_description: See LMS annotation.
+COMPREHENSIVE_THEME_DIRS = []
 
 # .. 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
@@ -11,14 +11,16 @@ Overview
 Status
 ******
 
-**Accepted**
+**Provisional**
 
-This was `originally authored <https://github.com/openedx/edx-platform/pull/31790>`_ in March 2023. We `modified it in July 2023 <https://github.com/openedx/edx-platform/pull/32804>`_ based on learnings from the implementation process, and then `modified and it again in May 2024 <https://github.com/openedx/edx-platform/pull/34554>`_ to make the migration easier for operators to understand.
+This was `originally authored <https://github.com/openedx/edx-platform/pull/31790>`_ in March 2023. We `modified it in July 2023 <https://github.com/openedx/edx-platform/pull/32804>`_ based on learnings from the implementation process.
 
-Related deprecation tickets:
+The status will be moved to *Accepted* upon completion of reimplementation. Related work:
 
 * `[DEPR]: Asset processing in Paver <https://github.com/openedx/edx-platform/issues/31895>`_
-* `[DEPR]: Paver <https://github.com/openedx/edx-platform/issues/34467>`_
+* `Process edx-platform assets without Paver <https://github.com/openedx/edx-platform/issues/31798>`_
+* `Process edx-platform assets without Python <https://github.com/openedx/edx-platform/issues/31800>`_
+
 
 Context
 *******
@@ -90,6 +92,7 @@ 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
 ********
 
@@ -111,9 +114,6 @@ 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,9 +226,6 @@ 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.
@@ -269,7 +266,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``
-     - ``COMPREHENSIVE_THEME_DIRS``
+     - ``EDX_PLATFORM_THEME_DIRS``
 
 Migration
 =========
@@ -288,16 +285,37 @@ 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.
 
-**May 2024 update:** The ``openedx-assets`` script will be removed from Tutor,
-with migration instructions documented in
-`Tutor's changelog <https://github.com/overhangio/tutor/blob/master/CHANGELOG.md>`_.
+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``.
 
 non-Tutor migration guide
 -------------------------
 
-A migration guide for site operators who are directly referencing Paver will be
-included in the
-`Paver deprecation ticket <https://github.com/openedx/edx-platform/issues/34467>`_.
+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.
+
 
 See also
 ********

diff --git 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 = os.environ.get('STATIC_ROOT_LMS', ENV_ROOT / "staticfiles")
+STATIC_ROOT = ENV_ROOT / "staticfiles"
 STATIC_URL_BASE = '/static/'
 
 STATICFILES_DIRS = [
@@ -2822,14 +2822,7 @@ def _make_locale_paths(settings):  # pylint: disable=missing-function-docstring
         'STATS_FILE': os.path.join(STATIC_ROOT, 'webpack-worker-stats.json')
     }
 }
-
-# .. 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')
+WEBPACK_CONFIG_PATH = 'webpack.prod.config.js'
 
 ########################## DJANGO DEBUG TOOLBAR ###############################
 
@@ -4556,11 +4549,9 @@ def _make_locale_paths(settings):  # pylint: disable=missing-function-docstring
 
 # .. setting_name: COMPREHENSIVE_THEME_DIRS
 # .. setting_default: []
-# .. 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_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_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
@@ -1,22 +1,21 @@
 """
 Management command for compiling sass.
-
-DEPRECATED in favor of `npm run compile-sass`.
 """
-import shlex
 
-from django.core.management import BaseCommand
-from django.conf import settings
 
-from pavelib.assets import run_deprecated_command_wrapper
+from django.core.management import BaseCommand, CommandError
+from paver.easy import call_task
+
+from openedx.core.djangoapps.theming.helpers import get_theme_base_dirs, get_themes, is_comprehensive_theming_enabled
+from pavelib.assets import ALL_SYSTEMS
 
 
 class Command(BaseCommand):
     """
     Compile theme sass and collect theme assets.
     """
 
-    help = "DEPRECATED. Use 'npm run compile-sass' instead."
+    help = 'Compile and collect themed assets...'
 
     # NOTE (CCB): This allows us to compile static assets in Docker containers without database access.
     requires_system_checks = []
@@ -29,7 +28,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=["lms", "cms"],
+            'system', type=str, nargs='*', default=ALL_SYSTEMS,
             help="lms or studio",
         )
 
@@ -56,7 +55,7 @@ def add_arguments(self, parser):
             '--force',
             action='store_true',
             default=False,
-            help="DEPRECATED. Full recompilation is now always forced.",
+            help="Force full compilation",
         )
         parser.add_argument(
             '--debug',
@@ -65,48 +64,77 @@ def add_arguments(self, parser):
             help="Disable Sass compression",
         )
 
-    def handle(self, *args, **options):
+    @staticmethod
+    def parse_arguments(*args, **options):  # pylint: disable=unused-argument
         """
-        Handle compile_sass command.
+        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
         """
-        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 = []
+        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:
-            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)]
+            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,
                 ),
-                *(
-                    arg
-                    for theme in themes
-                    for arg in ["--theme", theme]
+            )
+
+        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"
                 ),
-            ]),
+            )
+
+        call_task(
+            'pavelib.assets.compile_sass',
+            options={'system': system, 'theme_dirs': theme_dirs, 'themes': themes, 'force': force, 'debug': debug},
         )