Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

build: copy from node_modules using NPM postinstall hook, not Paver (#32717)

Browse Source 
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
This commit is contained in:
Kyle McCormick
2023-07-17 13:26:58 -04:00
committed by GitHub
parent 53718094a5
commit 4b64d8342d
5 changed files with 105 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
docs/decisions/0017-reimplement-asset-processing.rst
View File

@@ -131,17 +131,17 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
- ``scripts/build-assets.sh``
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>`_.
* - + **Build stage 1: Copy npm-installed assets** from node_modules to other folders in edx-platform. They are used by certain especially-old legacy LMS & CMS frontends that are not set up to work with npm directly.
- ``paver update_assets --skip-collect``
Implemented in Python within update_assets. There is no standalone command for it.
- ``scripts/build-assets.sh npm``
- ``npm install``
An NPM post-install hook will automatically call scripts/copy-node-modules.sh, a pure Bash reimplementation of the node_modules asset copying, whenever ``npm install`` is invoked.
Pure Bash reimplementation. See *Rejected Alternatives* for a note about this.
* - + **Build stage 2: Copy XModule fragments** from the xmodule source tree over to input directories for Webpack and SCSS compilation. This is required for a hard-coded list of old XModule-style XBlocks. This is not required for new pure XBlocks, which include (or pip-install) their assets into edx-platform as ready-to-serve JS/CSS/etc fragments.
- ``paver process_xmodule_assets``, or
@@ -156,7 +156,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
+ `Reimplement this step in Bash <https://github.com/openedx/edx-platform/issues/31611>`_.
+ `Remove the need for this step entirely <https://github.com/openedx/edx-platform/issues/31624>`_.
* - + **Build stage 3: Run Webpack** in order to to shim, minify, otherwise process, and bundle JS modules. This requires a call to the npm-installed ``webpack`` binary.
- ``paver webpack``
@@ -168,7 +168,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
Bash wrapper around a call to webpack. The script will accept parameters rather than looking up Django settings itself.
The print_setting command will still be available for distributions to use to extract ``STATIC_ROOT`` from Django settings, but it will only need to be run once. As described in **Build Configuration** below, unnecessary Django settings will be removed. Some distributions may not even need to look up ``STATIC_ROOT``; Tutor, for example, will probably render ``STATIC_ROOT`` directly into the environment variable ``OPENEDX_BUILD_ASSETS_OPTS`` variable, described in the **Build Configuration**.
* - + **Build stage 4: Compile default SCSS** into CSS for legacy LMS/CMS frontends.
- ``paver compile_sass``
@@ -180,7 +180,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
- ``scripts/build-assets.sh css``
Bash reimplementation, calling ``node-sass`` and ``rtlcss``.
The initial implementation of build-assets.sh may use ``sassc``, a CLI provided by libsass, instead of node-sass. Then, ``sassc`` can be replaced by ``node-sass`` as part of a subsequent `edx-platform frontend framework upgrade effort <https://github.com/openedx/edx-platform/issues/31616>`_.
* - + **Build stage 5: Compile themes' SCSS** into CSS for legacy LMS/CMS frontends. The default SCSS is used as a base, and theme-provided SCSS files are used as overrides. Themes are searched for from some number of operator-specified theme directories.
@@ -198,7 +198,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
The management command will remain available, but it will need to be updated to point at the Bash script, which will replace the paver task (see build stage 4 for details).
The overall asset *build* action will use the Bash script; this means that list of theme directories will need to be provided as arguments, but it ensures that the build can remain Python-free.
* - **Collect** the built static assets from edx-platform to another location (the ``STATIC_ROOT``) so that they can be efficiently served *without* Django's webserver. This step, by nature, requires Python and Django in order to find and organize the assets, which may come from edx-platform itself or from its many installed Python and NPM packages. This is only needed for **production** environments, where it is usually desirable to serve assets with something efficient like NGINX.
- ``paver update_assets``
@@ -210,7 +210,7 @@ The three top-level edx-platform asset processing actions are *build*, *collect*
- ``./manage.py lms collectstatic --noinput && ./manage.py cms collectstatic --noinput``
The standard Django interface will be used without a wrapper. The ignore patterns will be added to edx-platform's `staticfiles app configuration <https://docs.djangoproject.com/en/4.1/ref/contrib/staticfiles/#customizing-the-ignored-pattern-list>`_ so that they do not need to be supplied as part of the command.
* - **Watch** static assets for changes in the background. When a change occurs, rebuild them automatically, so that the Django webserver picks up the changes. This is only necessary in **development** environments. A few different sets of assets may be watched: XModule fragments, Webpack assets, default SCSS, and theme SCSS.
- ``paver watch_assets``
@@ -300,7 +300,7 @@ Either way, the migration path is straightforward:
* - ``openedx-assets build``
- ``scripts/build-assets.sh``
* - ``openedx-assets npm``
- ``scripts/build-assets.sh npm``
- ``scripts/copy-node-modules.sh # (automatically invoked by 'npm install'!)
* - ``openedx-assets xmodule``
- ``scripts/build-assets.sh xmodule``
* - ``openedx-assets common``
@@ -328,13 +328,6 @@ OpenCraft has also performed a discovery on a `modernized system for static asse
Rejected Alternatives
*********************
Copy node_modules via npm post-install
======================================
It was noted that `npm supports lifecycle scripts <https://docs.npmjs.com/cli/v6/using-npm/scripts#pre--post-scripts>`_ in package.json, including ``postinstall``. We could use a post-install script to copy assets out of node_modules; this would occurr automatically after ``npm install``. Arguably, this would be more idiomatic than this ADR's proposal of ``scripts/build-assets.sh npm``.
For now, we decided against this. While it seems like a good potential future improvement, we are currently unsure how it would interact with `moving node_modules out of edx-platform in Tutor <https://github.com/openedx/wg-developer-experience/issues/150>`_, which is a motivation behind this ADR. For example, if node_modules could be located anywhere on the image, then we are not sure how the post-install script could know its target directory without us hard-coding Tutor's directory structure into the script.
Live with the problem
======================

3
package.json
View File

@@ -2,6 +2,9 @@
"name": "edx",
"version": "0.1.0",
"repository": "https://github.com/openedx/edx-platform",
"scripts": {
"postinstall": "scripts/copy-node-modules.sh"
},
"dependencies": {
"@babel/core": "7.19.0",
"@babel/plugin-proposal-object-rest-spread": "^7.18.9",

90
pavelib/assets.py
View File

@@ -46,39 +46,6 @@ COMMON_LOOKUP_PATHS = [
path('node_modules'),
]
# A list of NPM installed libraries that should be copied into the common
# static directory.
# If string ends with '/' then all file in the directory will be copied.
NPM_INSTALLED_LIBRARIES = [
'backbone.paginator/lib/backbone.paginator.js',
'backbone/backbone.js',
'bootstrap/dist/js/bootstrap.bundle.js',
'hls.js/dist/hls.js',
'jquery-migrate/dist/jquery-migrate.js',
'jquery.scrollto/jquery.scrollTo.js',
'jquery/dist/jquery.js',
'moment-timezone/builds/moment-timezone-with-data.js',
'moment/min/moment-with-locales.js',
'picturefill/dist/picturefill.js',
'requirejs/require.js',
'underscore.string/dist/underscore.string.js',
'underscore/underscore.js',
'@edx/studio-frontend/dist/',
'which-country/index.js'
]
# A list of NPM installed developer libraries that should be copied into the common
# static directory only in development mode.
NPM_INSTALLED_DEVELOPER_LIBRARIES = [
'sinon/pkg/sinon.js',
'squirejs/src/Squire.js',
]
# Directory to install static vendor files
NPM_JS_VENDOR_DIRECTORY = path('common/static/common/js/vendor')
NPM_CSS_VENDOR_DIRECTORY = path("common/static/common/css/vendor")
NPM_CSS_DIRECTORY = path("common/static/common/css")
# system specific lookup path additions, add sass dirs if one system depends on the sass files for other systems
SASS_LOOKUP_DEPENDENCIES = {
'cms': [path('lms') / 'static' / 'sass' / 'partials', ],
@@ -644,60 +611,8 @@ def process_npm_assets():
"""
Process vendor libraries installed via NPM.
"""
def copy_vendor_library(library, skip_if_missing=False):
"""
Copies a vendor library to the shared vendor directory.
"""
if library.startswith('node_modules/'):
library_path = library
else:
library_path = f'node_modules/{library}'
if library.endswith('.css') or library.endswith('.css.map'):
vendor_dir = NPM_CSS_VENDOR_DIRECTORY
else:
vendor_dir = NPM_JS_VENDOR_DIRECTORY
if os.path.exists(library_path):
sh('/bin/cp -rf {library_path} {vendor_dir}'.format(
library_path=library_path,
vendor_dir=vendor_dir,
))
elif not skip_if_missing:
raise Exception(f'Missing vendor file {library_path}')
def copy_vendor_library_dir(library_dir, skip_if_missing=False):
"""
Copies all vendor libraries in directory to the shared vendor directory.
"""
library_dir_path = f'node_modules/{library_dir}'
print(f'Copying vendor library dir: {library_dir_path}')
if os.path.exists(library_dir_path):
for dirpath, _, filenames in os.walk(library_dir_path):
for filename in filenames:
copy_vendor_library(os.path.join(dirpath, filename), skip_if_missing=skip_if_missing)
# Skip processing of the libraries if this is just a dry run
if tasks.environment.dry_run:
tasks.environment.info("install npm_assets")
return
# Ensure that the vendor directory exists
NPM_JS_VENDOR_DIRECTORY.mkdir_p()
NPM_CSS_DIRECTORY.mkdir_p()
NPM_CSS_VENDOR_DIRECTORY.mkdir_p()
# Copy each file to the vendor directory, overwriting any existing file.
print("Copying vendor files into static directory")
for library in NPM_INSTALLED_LIBRARIES:
if library.endswith('/'):
copy_vendor_library_dir(library)
else:
copy_vendor_library(library)
# Copy over each developer library too if they have been installed
print("Copying developer vendor files into static directory")
for library in NPM_INSTALLED_DEVELOPER_LIBRARIES:
copy_vendor_library(library, skip_if_missing=True)
print("\t\tProcessing NPM assets is now done automatically in an npm post-install hook.")
print("\t\tThis function is now a no-op.")
@task
@@ -983,7 +898,6 @@ def update_assets(args):
collect_log_args = {}
process_xmodule_assets()
process_npm_assets()
# Build Webpack
call_task('pavelib.assets.webpack', options={'settings': args.settings})

2
pavelib/utils/test/suites/js_suite.py
View File

@@ -39,8 +39,6 @@ class JsTestSuite(TestSuite):
if self.mode == 'run' and not self.run_under_coverage:
test_utils.clean_dir(self.report_dir)
assets.process_npm_assets()
@property
def _default_subsuites(self):
"""

90
scripts/copy-node-modules.sh Executable file
View File

@@ -0,0 +1,90 @@
#!/usr/bin/env bash
# Copy certain npm-installed assets from node_modules to other folders in
# edx-platform. These assets are used by certain especially-old legacy LMS & CMS
# frontends that are not set up to import from node_modules directly.
# Many of the destination folders are named "vendor", because they originally
# held vendored-in (directly-committed) libraries; once we moved most frontends
# to use NPM, we decided to keep library versions in-sync by copying to the
# former "vendor" directories.
# Enable stricter error handling.
set -euo pipefail
COL_LOG="\e[36m" # Log/step/section color (cyan)
COL_OFF="\e[0m" # Normal color
# Keep these as variables in case we ever want to parameterize this script's
# input or output dirs, as proposed in:
# https://github.com/openedx/wg-developer-experience/issues/150
# https://github.com/openedx/wg-developer-experience/issues/151
node_modules="node_modules"
vendor_js="common/static/common/js/vendor"
vendor_css="common/static/common/css/vendor"
# Stylized logging.
log ( ) {
echo -e "${COL_LOG}$* $COL_OFF"
}
log "====================================================================================="
log "Copying required assets from node_modules..."
log "-------------------------------------------------------------------------------"
# Start echoing all commands back to user for ease of debugging.
set -x
log "Ensuring vendor directories exist..."
mkdir -p "$vendor_js"
mkdir -p "$vendor_css"
log "Copying studio-frontend JS & CSS from node_modules into vendor directores..."
while read -r -d $'\0' src_file ; do
if [[ "$src_file" = *.css ]] || [[ "$src_file" = *.css.map ]] ; then
cp --force "$src_file" "$vendor_css"
else
cp --force "$src_file" "$vendor_js"
fi
done < <(find "$node_modules/@edx/studio-frontend/dist" -type f -print0)
log "Copying certain JS modules from node_modules into vendor directory..."
cp --force \
"$node_modules/backbone.paginator/lib/backbone.paginator.js" \
"$node_modules/backbone/backbone.js" \
"$node_modules/bootstrap/dist/js/bootstrap.bundle.js" \
"$node_modules/hls.js/dist/hls.js" \
"$node_modules/jquery-migrate/dist/jquery-migrate.js" \
"$node_modules/jquery.scrollto/jquery.scrollTo.js" \
"$node_modules/jquery/dist/jquery.js" \
"$node_modules/moment-timezone/builds/moment-timezone-with-data.js" \
"$node_modules/moment/min/moment-with-locales.js" \
"$node_modules/picturefill/dist/picturefill.js" \
"$node_modules/requirejs/require.js" \
"$node_modules/underscore.string/dist/underscore.string.js" \
"$node_modules/underscore/underscore.js" \
"$node_modules/which-country/index.js" \
"$vendor_js"
log "Copying certain JS developer modules into vendor directory..."
if [[ "${NODE_ENV:-production}" = development ]] ; then
cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js"
cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js"
else
# TODO: https://github.com/openedx/edx-platform/issues/31768
# In the old implementation of this scipt (pavelib/assets.py), these two
# developer libraries were copied into the JS vendor directory whether not
# the build was for prod or dev. In order to exactly match the output of
# the old script, this script will also copy them in for prod builds.
# However, in the future, it would be good to only copy them for dev
# builds. Furthermore, these libraries should not be `npm install`ed
# into prod builds in the first place.
cp --force "$node_modules/sinon/pkg/sinon.js" "$vendor_js" || true # "|| true" means "tolerate errors"; in this case,
cp --force "$node_modules/squirejs/src/Squire.js" "$vendor_js" || true # that's "tolerate if these files don't exist."
fi
# Done echoing.
set +x
log "-------------------------------------------------------------------------------"
log " Done copying required assets from node_modules."
log "====================================================================================="