From c1959a158512cf00c3a5a115bbcb47fd8e7a423a Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 11 Jul 2023 11:17:33 -0400 Subject: [PATCH] build: copy from node_modules using NPM postinstall hook, not Paver TODO will fill in details from PR description Part of: https://github.com/openedx/edx-platform/issues/31604 --- .../0017-reimplement-asset-processing.rst | 27 +++--- package.json | 3 + pavelib/assets.py | 90 +------------------ pavelib/utils/test/suites/js_suite.py | 2 - scripts/copy-node-modules.sh | 90 +++++++++++++++++++ 5 files changed, 105 insertions(+), 107 deletions(-) create mode 100755 scripts/copy-node-modules.sh diff --git a/docs/decisions/0017-reimplement-asset-processing.rst b/docs/decisions/0017-reimplement-asset-processing.rst index 7f16dcc249ff..7ffb7ab5e829 100644 --- a/docs/decisions/0017-reimplement-asset-processing.rst +++ b/docs/decisions/0017-reimplement-asset-processing.rst @@ -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 `_. - + * - + **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 `_. + `Remove the need for this step entirely `_. - + * - + **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 `_. * - + **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 `_ 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 `_ 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 `_, 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 ====================== diff --git a/package.json b/package.json index fb855d32a3e9..ebdcf240e153 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/pavelib/assets.py b/pavelib/assets.py index 3bf8a9ce88ae..8b1b4e706546 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -46,39 +46,6 @@ 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}) diff --git a/pavelib/utils/test/suites/js_suite.py b/pavelib/utils/test/suites/js_suite.py index a6896e285854..65c5feaf843b 100644 --- a/pavelib/utils/test/suites/js_suite.py +++ b/pavelib/utils/test/suites/js_suite.py @@ -39,8 +39,6 @@ def __enter__(self): 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): """ diff --git a/scripts/copy-node-modules.sh b/scripts/copy-node-modules.sh new file mode 100755 index 000000000000..db997da95785 --- /dev/null +++ b/scripts/copy-node-modules.sh @@ -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 "=====================================================================================" +