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
This reverts commit 52adce48d1 because we were getting this in the build pipeline:
```
WARNING: lxml 5.2.1 does not provide the extra 'html-clean'
ERROR: Exception:
Traceback (most recent call last):
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/cli/base_command.py", line 173, in _main
status = self.run(options, args)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/cli/req_command.py", line 203, in wrapper
return func(self, options, args)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/commands/install.py", line 315, in run
requirement_set = resolver.resolve(
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 94, in resolve
result = self._result = resolver.resolve(
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_vendor/resolvelib/resolvers.py", line 472, in resolve
state = resolution.resolve(requirements, max_rounds=max_rounds)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_vendor/resolvelib/resolvers.py", line 366, in resolve
failure_causes = self._attempt_to_pin_criterion(name)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_vendor/resolvelib/resolvers.py", line 221, in _attempt_to_pin_criterion
satisfied = all(
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_vendor/resolvelib/resolvers.py", line 222, in <genexpr>
self._p.is_satisfied_by(requirement=r, candidate=candidate)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/resolution/resolvelib/provider.py", line 178, in is_satisfied_by
return requirement.is_satisfied_by(candidate)
File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/pip/_internal/resolution/resolvelib/requirements.py", line 84, in is_satisfied_by
assert candidate.name == self.name, (
AssertionError: Internal issue: Candidate is not for this requirement lxml[html-clean,html-clean] vs lxml[html-clean]
WARNING: You are using pip version 21.2.1; however, version 24.0 is available.
You should consider upgrading via the '/edx/app/edxapp/venvs/edxapp/bin/python -m pip install --upgrade pip' command.
```
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.
Instead of using libsass-python's `sass` module, which is incompatible
with Python 3.11, we now directly use libsass-python's `_sass` module,
which directly binds to the underlying C++ libsass library.
This ensures that the Sass build will not a blocker for the edx-platform
Python 3.11 upgrade, which needs to land in Redwood.
For more details, see the docstring at scripts/compile_sass.py#L167
The implementation of `npm run watch-sass` was trying really hard
to recompile precisely only the Sass that needed to be recompiled, but in
order to do so, it had to spin up several `watchmedo` processes
per theme. These processes would trigger one another sometimes, leading
to infinite recompilation loops.
Rather than figure out all the dependency directions and messing with
`watchmedo`, I've opted to simplify the script to invoke a single
`watchmedo` process per theme. A single theme recompiles within
seconds, so I think this is a good compromise, one which makes the
script easier to reason about will help me move pass this legacy
assets work.
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
Without this change, `npm run postinstall` (aka
scripts/copy-node-modules.sh) uses `set -x`, which echo steps to STDERR.
By default, it seems that GitHub Actions doesn't show STDERR.
Having the steps visible in CI was very useful while debugging some
Tutor build improvements, so I figured it would be good to upstream the
change.
This should probably be turned into an ADR at some point, but for
now I've just recreated the page on the public wiki. (The space it
was in had been deleted during the 2U/Axim split.)
Implements the `npm run watch` section of the assets ADR [1], plus some
modifications since I decided to switch from pywatchman to watchdog (see
ADR changes for justification). This will replace `paver watch_assets`
(edx-platform) and `openedx-assets watch-themes` (Tutor).
Specifically, this PR adds three experimental commands:
* `npm run watch-sass` : Watch for Sass changes with watchdog.
* `npm run watch-webpack` : Invoke Webpack-watch for JS changes.
* `npm run watch` : Invoke both `watch-sass` and `watch-webpack` simultaneously.
These commands are only intended to work in development mode. They have
been tested both on bare-metal edx-platform and through `tutor dev run`
on on Linux.
Before removing the "experimental" label, we need to:
* Test through Devstack on Linux.
* Test through Devstack and `tutor dev run` on macOS.
* Test on bare-metal macOS. Might not work, which is OK, but we should
document that.
* Document the commands in edx-platform's README.
* Confirm that this not only works through `tutor dev run`, but also as
a suitable replacement in the `watchthemes` service that Tutor runs
automatically as part of `tutor dev start`. Tweak if necessary.
References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
Part of: https://github.com/openedx/edx-platform/issues/31612
This PR implements much of the static assets rework ADR [1], including:
* `npm run build[-dev]`, and its subcommands,
* `npm run webpack[-dev]` and
* `npm run compile-sass[-dev]`.
This is backwards-compatible. `paver update_assets` should not be affected.
The new command warns that it is "experimental" for a few reasons:
* `npm run build` will fail in the webpack phase unless you first
run `xmodule_assets`. This will be changed soon [2].
* We have tested the new build, but not quite so thoroughly that we'd
recommend it as the production default yet. Once the xmodule_assets
work lands, we'll share this on the forums so early adopters can try it
out.
* The commands lack some top-level documentation. Once they stabilize more,
we'll add a section to the README that explains how and when to use `npm run
build` and its subcommands and its env vars.
* `npm run watch` is not yet implemented.
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/32685
Part of: https://github.com/openedx/edx-platform/issues/31604
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
