docs: update static assets ADR with the new plan #32804

kdmccormick · 2023-07-20T20:38:37Z

We update the static assets plan based on three major things we've learned:

You can stick any old script under npm run via package.json, and apparently that's the standard thing to do in frontend packages. So, forget scripts/build-assets.sh, let's do npm run build!
Removing the special XModule asset processing was necessary in order to achieve most of the optimization goals of this ADR, so we went ahead and did it. We can remove the xmodule_assets step entirely now.
Upgrading from libsass-python to node-sass/dart-sass is hard any nobody has time to do it right now. Even sidegrading to a another old non-Python version of libsass proved to be difficult when I tried. So, we are stuck with libsass-python for now, which means we are stuck with writing the compile-sass script in Python. This is fine, as we can still get most of the benefits we need as long as we segment out the requirements for compile-sass from the rest of edx-platform.

Links:

kdmccormick

Extra notes for reviewers.

kdmccormick · 2023-07-20T20:47:06Z

docs/decisions/0017-reimplement-asset-processing.rst

+State of edx-platform frontends (early 2023)
+============================================


Timestamping this since the line between XModule and XBlock has blurred since writing. Still, the table makes sense from the perspective of this ADR.

kdmccormick · 2023-07-20T20:48:02Z

docs/decisions/0017-reimplement-asset-processing.rst

+     - ``npm clean-install && npm run build``
+
+       Simple NPM wrappers around the build stages. The wrappers will be written in Bash and tested on both GNU+Linux and macOS.

-       A Bash script that contains all build stages, with subcommands available for running each stage separately. Its command-line interface inspired by Tutor's ``openedx-assets`` script. The script will be runnable on any POSIX system, including macOS and Ubuntu and it will linted for common shell scripting mistakes using `shellcheck <https://www.shellcheck.net>`_.
+       These commands are a "one stop shop" for building assets, but more efficiency-oriented users may choose to run build stages individually.


As I moved towards npm run, I moved away from the structure of openedx-assets.

kdmccormick · 2023-07-20T20:49:31Z

docs/decisions/0017-reimplement-asset-processing.rst


       Bash wrappers around invocation(s) of `watchman <https://facebook.github.io/watchman/>`_, a popular file-watching library maintained by Meta. Watchman is already installed into edx-platform (and other services) via the pywatchman pip wrapper package.

-       Note: This adds a Python dependency to build-assets.sh. However, we could be clear that watchman is an *optional* dependency of build-assets.sh which enables the optional ``--watch`` feature. This would keep the *build* action Python-free. Alternatively, watchman is also available Python-free via apt and homebrew.


This note seemed irrelevant now that npm run watch is an entirely separate command.

kdmccormick · 2023-07-20T20:54:44Z

docs/decisions/0017-reimplement-asset-processing.rst

-``scripts/build-assets.sh`` will accept various command-line options to configure the build. It will also accept the same options in the form of the ``OPENEDX_BUILD_ASSETS_OPTS`` enviroment variable. Options from the environment variable will be processed first, and then overridden by options provided on the command line. The environment variable allows distributions like Tutor to seed the build script with "defaults" in the event that the upstream defaults are not sufficient, while still allowing individual operators to run the script with whichever options they like.
+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::

-As a concrete example, the default value of ``--theme-dirs`` will be ``''`` (that is: no themes) and the default value of ``--static-root`` will be ``./test_root/static``. Neither of those are suitable for Tutor. Instead, Tutor will set the environment variable in its Dockerfile::
+  MY_ENV_VAR="my value" npm run build    # Set for the whole build.
+  MY_ENV_VAR="my value" npm run webpack  # Set for just a single step, like webpack.

-  ...
-  ENV OPENEDX_BUILD_ASSETS_OPTS '--theme-dirs /openedx/themes --static-root /openedx/staticfiles'
-  ...
+For Docker-based distributions like Tutor, these environment variables can instead be set in the Dockerfile.

-Later, in the container, a user might run::


This whole OPENEDX_BUILDS_ASSETS_OPTS idea doesn't play well with the simpler npm run command structure, so I removed it in favor of simple environment variables (below).

kdmccormick · 2023-07-20T20:55:56Z

docs/decisions/0017-reimplement-asset-processing.rst

-Rewrite asset processing in Python
-==================================
-
-Some of the benefits of dropping Paver could still be achieved even if we re-wrote the asset processing system using, for example, Python and Click. However, entirely dropping Python from the asset build in favor of Bash has promising benefits:


We aren't entirely removing Python any more, so I removed this section, but I folded its arguments into the rejection of "Improve existing system" since they are largely the same.

feanil

One non-blocking comment but generally looks like a good improvement which leads us closer to npm and node for front-end things and python/django for backend/historic things.

I like that we don't have to have people know about a new interface for invoking commands.

feanil · 2023-07-20T21:11:24Z

docs/decisions/0017-reimplement-asset-processing.rst


-       Bash wrapper around a call to webpack. The script will accept parameters rather than looking up Django settings itself.
+       Simple shell script defined in package.json to invoke Webpack in prod or dev mode. The script will look for several environment variables, with a default defined for each one. See **Build Configuration** for details. The script will NOT invoke ``print_setting``; we leave to distributions the tasking of setting environment variables appropriately.


Love this, though I know it may feel a bit duplicative to have this info in both an environment variable and in the LMS settings. In the future we could allow the user to override the setting based on a value in the environment as well so I'm not too worried about it for now.

There is an implied "We expect Django and Node to put assets in the same place for things to render properly." Is this written down somewhere or should we document it?

(Non-blocking thought.)

Agreed all around. The specifics of things like the exact env var semantics are still in flux (hence this PR :) so I'm hoping that this ADR will tide over anyone who wants to experiment with the new assets system. Once I'm closer to removing Paver, though, I'll make sure that these sort of settings are either de-duped or documented.

edx-pipeline-bot · 2023-07-21T12:52:28Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-07-21T14:37:22Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-07-21T14:37:22Z

2U Release Notice: This PR has been deployed to the edX production environment.

kdmccormick force-pushed the kdmccormick/new-assets-adr branch 3 times, most recently from e647836 to 9561a2f Compare July 20, 2023 20:53

kdmccormick commented Jul 20, 2023

View reviewed changes

kdmccormick marked this pull request as ready for review July 20, 2023 20:57

kdmccormick requested a review from feanil July 20, 2023 20:57

kdmccormick force-pushed the kdmccormick/new-assets-adr branch from 9561a2f to 81c7c10 Compare July 20, 2023 20:59

feanil approved these changes Jul 20, 2023

View reviewed changes

kdmccormick mentioned this pull request Jul 20, 2023

Ensure that asset-related env vars and Django settings line up #32805

Closed

docs: update static assets ADR with the new plan

7f354ca

kdmccormick force-pushed the kdmccormick/new-assets-adr branch from 81c7c10 to 7f354ca Compare July 21, 2023 02:38

kdmccormick merged commit 4f393a1 into openedx:master Jul 21, 2023

kdmccormick deleted the kdmccormick/new-assets-adr branch July 27, 2023 14:48

kdmccormick mentioned this pull request May 1, 2024

[DEPR]: Paver #34467

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: update static assets ADR with the new plan #32804

docs: update static assets ADR with the new plan #32804

kdmccormick commented Jul 20, 2023

kdmccormick left a comment

kdmccormick Jul 20, 2023 •

edited

Loading

kdmccormick Jul 20, 2023

kdmccormick Jul 20, 2023

kdmccormick Jul 20, 2023

kdmccormick Jul 20, 2023

feanil left a comment

feanil Jul 20, 2023

kdmccormick Jul 20, 2023

edx-pipeline-bot commented Jul 21, 2023

edx-pipeline-bot commented Jul 21, 2023

edx-pipeline-bot commented Jul 21, 2023

		State of edx-platform frontends (early 2023)
		============================================


		Bash wrappers around invocation(s) of `watchman <https://facebook.github.io/watchman/>`_, a popular file-watching library maintained by Meta. Watchman is already installed into edx-platform (and other services) via the pywatchman pip wrapper package.

		Note: This adds a Python dependency to build-assets.sh. However, we could be clear that watchman is an optional dependency of build-assets.sh which enables the optional ``--watch`` feature. This would keep the build action Python-free. Alternatively, watchman is also available Python-free via apt and homebrew.


		Bash wrapper around a call to webpack. The script will accept parameters rather than looking up Django settings itself.
		Simple shell script defined in package.json to invoke Webpack in prod or dev mode. The script will look for several environment variables, with a default defined for each one. See Build Configuration for details. The script will NOT invoke ``print_setting``; we leave to distributions the tasking of setting environment variables appropriately.

docs: update static assets ADR with the new plan #32804

docs: update static assets ADR with the new plan #32804

Conversation

kdmccormick commented Jul 20, 2023

kdmccormick left a comment

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

feanil Jul 20, 2023

Choose a reason for hiding this comment

kdmccormick Jul 20, 2023

Choose a reason for hiding this comment

edx-pipeline-bot commented Jul 21, 2023

edx-pipeline-bot commented Jul 21, 2023

edx-pipeline-bot commented Jul 21, 2023

kdmccormick Jul 20, 2023 •

edited

Loading