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.
Together, these changes make it so that all features of the Paver-based
asset compilation system are supported with drop-in Paver-free
replacements. The remaining Paver asset functions are trivial wrappers,
which can be comfortably deleted before Sumac.
* Turn `./manage.py ... compile_sass` into a simple wrapper around `npm
run compile-sass`
* Turn `paver webpack` into a simple wrapper around `npm run webpack`
* Turn `pavelib.assets:collect_assets` into a simple wrapper around
`./manage.py ... collectstatic`
* Add/improve deprecation warnings for all Paver asset commands.
* Load defaults for asset-related Django settings from environment
variables. This allows the build to work without Python. For the
settings which will be removed in Sumac, I've added deprecation
warnings.
* Change EDX_PLATFORM_THEME_DIRS env var to COMPREHENSIVE_THEME_DIRS.
This simplifies the migration instructions, because all the new env
vars now match their corresponding Django settings. This amends an
ADR, but it should not be a breaking change because the env var was
recently added (since Quince) and nobody should be using it yet.
* Future-proof the static assets ADR with links. The linked pages will
be kept up-to-date even if the ADR isn't.
Part of: https://github.com/openedx/edx-platform/issues/34467
`paver` commands are deprecated for managing static assets. Starting in
Sumac, only `npm run` commands will be supported for managing static
assets.
To ease the transition, both `paver` and `npm run` commands will work in
Redwood. However, we want to stop using the *implementations* of the
`paver` asset commands right now, as they are blocking the Python 3.11
upgrade. This will also make the removal of `paver` commands more
straightforward come Sumac.
So, this commit turns these commands/functions:
* paver compile_sass (used by configuration)
* paver watch_sass (used by configuration and devstack)
* pavelib/assets.py:_compile_sass (used by Tutor)
into very thin wrappers around the new `npm run` commands. Each of these
paver routines now raise a loud deprecation warning, including a message
of the `npm run` command that the operator can switch to.
We expect no impact to site operators or end users.
https://github.com/openedx/edx-platform/issues/31895
All CI used to go through scripts/generic-ci-tests.sh, which is a
wrapper around various `paver` test/linting/check invocations.
These days, most edx-platform CI checks just invoke their tools (pylint,
pycodestyle, pytest, etc.) directly.
In anticipation of the proposed Paver deprecation [1], let's remove
the parts of this script that aren't used any more, including several
`paver` command invocations. This should have no impact on CI.
Furthermore, we are able to remove the SHARD environment variable,
which was formely used to split unit and quality checks up into
smaller pieces. Unit tests and pylint checks now have their own
separate sharding logic, so there is only one "quality" shard remaining
(SHARD=4, ie generic quality checks), thus we don't need a SHARD
variable at all.
[1] https://github.com/openedx/edx-platform/issues/34467
This is the only call to the removed function and the removed function
printed a dead link about an issue that may no longer be relevant so
we're just removing this call rather than putting the message back.
The Webpack configuration file for built-in XBlock JS used to be
generated at build time and git-ignored. It lived at
common/static/xmodule/webpack.xmodule.config.js. It was generated
because the JS that it referred to was also generated at build-time, and
the filenames of those JS modules were not static.
Now that its contents have been made entirely static [1], there is no
reason we need to continue generating this Webpack configuration file.
So, we check it into edx-platform under the name
./webpack.builtinblocks.config.js. We choose to put it in the repo's
root directory because the paths contained in the config file are
relative to the repo's root.
This allows us to behead both the xmodule/static_content.py
(`xmodule_assets`) script andthe `process_xmodule_assets` paver task, a
major step in removing the need for Python in the edx-platform asset
build [2]. It also allows us to delete the `HTMLSnippet` class and all
associated attributes, which were exclusively used by
xmodule/static_content.py..
We leave `xmodule_assets` and `process_xmodule_assets` in as stubs for
now in order to avoid breaking external code (like Tutor) which calls
Paver; the entire pavelib/assets.py function will be eventually removed
soon anyway [3]. Further, to avoid extraneous refactoring, we keep one
method of `HTMLSnippet` around on a few of its former subclasses:
`get_html`. This method was originally part of the XModule framework;
now, it is left over on a few classes as a simple internal helper
method.
References:
1. https://github.com/openedx/edx-platform/pull/32480
2. https://github.com/openedx/edx-platform/issues/31800
3. https://github.com/openedx/edx-platform/issues/31895
Part of: https://github.com/openedx/edx-platform/issues/32481
Two fixes:
* In the (in-repo, non-Tutor) Dockerfile, add copy-node-modules.sh
before `npm install`, since it is needed by the new postinstall hook.
* In paver/assets.py, run copy-node-modules.sh for backwards com-
patibility, just for cases where `SKIP_NPM_INSTALL` is enabled
(which would prevent our new postinstall hook from running
automatically!). We will deprecate the paver asset commands all at
once once the new non-paver stuff is 100% working.
During the review of ADR 17 [1], Régis pointed out [2] that the shell script
which replaces Paver's `process_npm_assets` could be automatically invoked as
an NPM post-install hook, ensuring that the step is seamlessly executed whenever
`npm install` is run. I had avoided using that suggestion, as I worried that it
would make it harder to move node_modules out of the edx-platform directory in
Tutor's openedx image.
Since then, two things have changed. Firstly, Tutor v16's new persistent mounts
interface [3] has lessened the importance of moving node_modules. Secondly, I
have realized that using a post-install hook would not preclude us from
modifying the underlying script (scripts/copy-node-modules.sh) to look in an
alternative location for node_modules, should that end up being something we
want to do.
This commit modifies the ADR based on those findings, stubs out Paver's
`process_npm_assets`, and adds the suggested post-install hook and replacement
Bash script.
References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. https://github.com/openedx/edx-platform/pull/31790#discussion_r1122802492
3. https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14
Part of: https://github.com/openedx/edx-platform/issues/31604
In ~Palm and earlier, all built-in XBlock Sass was included into LMS and CMS
styles before being compiled. The generated CSS was coupled together with
broader LMS/CMS CSS. This means that comprehensive themes have been able to
modify built-in XBlock appearance by setting certain Sass variables. We say that
built-in XBlock Sass was, and is expected to be, "theme-aware".
Shortly after Palm, we decoupled XBlock Sass from LMS and CMS Sass [1]. Each
built-in block's Sass is now compiled into two separate CSS targets, one for
block editing and one for block display. The CSS, now located at
`common/static/css/xmodule`, is injected into the running Webpack context with
the new `XModuleWebpackLoader`. Built-in XBlocks already used
`add_webpack_to_fragment` in order to add JS Webpack bundles to their view
fragments, so when CSS was added to Webpack, it Just Worked.
This unlocked a slieu of simplifications for static asset processing [2];
however, it accidentally made XBlock Sass theme-*unaware*, or perhaps
theme-confused, since the CSS was targeted at `common/static/css/xmodule`
regardless of the theme. The result of this is that **built-in XBlock views will
use CSS based on the Sass variables _last theme to be compiled._** Sass
variables are only used in a handful of places in XBlocks, so the bug is subtle,
but it is there for those running off of master. For example, using edX.org's
theme on master, we can see that there is a default blue underline in the Studio
sequence nav [3]. With this bugfix, it becomes the standard edX.org
greenish-black [4].
This commit makes several changes, firstly to fix the bug, and secondly to leave
ourselves with a more comprehensible asset setup in the `xmodule/` directory.
* We remove the `XModuleWebpackLoader`, thus taking built-in XBlock Sass back
out of Webpack.
* We compile XBlock Sass not to `common/static/css/xmodule`, but to:
* `[lms|cms]/static/css` for the default theme, and
* `<THEME_ROOT>/[lms|cms]/static/css`, for any custom theme.
This is where the comprehensive theming system expects to find themable
assets. Unfortunately, this does mean that the Sass is compiled twice, both
for LMS and CMS. We would have liked to compile it once to somewhere in the
`common/`, but comprehensive theming does not consider `common/` assets to be
themable.
* We split `add_webpack_to_fragment` into two more specialized functions:
* `add_webpack_js_to_fragment` , for adding *just* JS from a Webpack bundle,
and
* `add_sass_to_fragment`, for adding static links to CSS compiled themable
Sass (not Webpack). Both these functions are moved to a new module
`xmodule/util/builtin_assets.py`, since the original module
(`xmodule/util/xmodule_django.py`) didn't make a ton of sense.
* In an orthogonal bugfix, we merge Sass `CourseInfoBlock`, `StaticTabBlock`,
`AboutBlock` into the `HtmlBlock` Sass files. The first three were never used,
as their styling was handled by `HtmlBlock` (their shared parent class).
* As a refactoring, we change Webpack bundle names and Sass module names to be
less misleading:
* student_view, public_view, and author_view: was `<Name>BlockPreview`, is now
`<Name>BlockDisplay`.
* studio_view: was `<Name>BlockStudio`, is now `<Name>BlockEditor`.
* As a refactoring, we move the contents of `xmodule/static` into the existing
`xmodule/assets` directory, and adopt its simper structure. We now have:
* `xmodule/assets/*.scss`: Top-level compiled Sass modules. These could be
collapsed away in a future refactoring.
* `xmodule/assets/<blocktype>/*`: Resources for each block, including both JS
modules and Sass includes (underscore-prefixed so that they aren't
compiled). This structure maps closely with what externally-defined XBlocks
do.
* `xmodule/js` still exists, but it will soon be folded into the
`xmodule/assets`.
* We add a new README [4] to explain the new structure, and also update a
docstring in `openedx/lib/xblock/utils` which had fallen out of date with
reality.
* Side note: We avoid the term "XModule" in all of this, because that's
(thankfully) become a much less useful/accurate way to describe these blocks.
Instead, we say "built-in XBlocks".
Refs:
1. https://github.com/openedx/edx-platform/pull/32018
2. https://github.com/openedx/edx-platform/issues/32292
3. https://github.com/openedx/edx-platform/assets/3628148/8b44545d-0f71-4357-9385-69d6e1cca86f
4. https://github.com/openedx/edx-platform/assets/3628148/d0b7b309-b8a4-4697-920a-8a520e903e06
5. https://github.com/openedx/edx-platform/tree/master/xmodule/assets#readme
Part of: https://github.com/openedx/edx-platform/issues/32292
Otherwise we're not really respecting the package-lock file and won't get
repeatable results.
Also:
- Clean up old error handling for npm<3. Were on npm 8 now. Probably
can get rid of this.
- Use the shorthand `npm ci` rather than `npm clean-install` just for
consistency with code elsewhere.
- Update comments in tests to be explicit about use of ci rather than
install
Now that all XModule SCSS is located in xmodule/static/sass,
it would make sense to co-locate the CSS there as well.
We also add a README to explain the purpose of this new folder.
In the future, we will move xmodule/js and xmodule/assets
into xmodule/static as well.
Part of: https://github.com/openedx/edx-platform/issues/32292
