From 95e0cdef281db8010607c27345758b90ebe6ad72 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 17 Jul 2023 14:06:05 -0400 Subject: [PATCH] Revert "build: copy from node_modules using NPM postinstall hook, not Paver (#32717)" This reverts commit 4b64d8342d69bb92afe56a49c61bc7e5663aceff. --- .../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, 107 insertions(+), 105 deletions(-) delete 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 7ffb7ab5e829..7f16dcc249ff 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. - - ``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. + - ``scripts/build-assets.sh npm`` + 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/copy-node-modules.sh # (automatically invoked by 'npm install'!) + - ``scripts/build-assets.sh npm`` * - ``openedx-assets xmodule`` - ``scripts/build-assets.sh xmodule`` * - ``openedx-assets common`` @@ -328,6 +328,13 @@ 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 ebdcf240e153..fb855d32a3e9 100644 --- a/package.json +++ b/package.json @@ -2,9 +2,6 @@ "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 8b1b4e706546..3bf8a9ce88ae 100644 --- a/pavelib/assets.py +++ b/pavelib/assets.py @@ -46,6 +46,39 @@ 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', ], @@ -611,8 +644,60 @@ def process_npm_assets(): """ Process vendor libraries installed via NPM. """ - print("\t\tProcessing NPM assets is now done automatically in an npm post-install hook.") - print("\t\tThis function is now a no-op.") + 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) @task @@ -898,6 +983,7 @@ 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 65c5feaf843b..a6896e285854 100644 --- a/pavelib/utils/test/suites/js_suite.py +++ b/pavelib/utils/test/suites/js_suite.py @@ -39,6 +39,8 @@ 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 deleted file mode 100755 index db997da95785..000000000000 --- a/scripts/copy-node-modules.sh +++ /dev/null @@ -1,90 +0,0 @@ -#!/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 "=====================================================================================" -