From 2a2ee8840013e59111a084e64b2994f850174fe3 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 6 Jan 2023 15:31:22 -0500 Subject: [PATCH 1/3] refactor: build: reimplement `process_npm_assets` as shell script Reimplement `pavelib/assets.py:process_npm_assets` equivalently as `scripts/assets/copy-node-modules.sh`. For backwards compatibility, `pavelib/assets.py:process_npm_assets` will still exist, just as a thin wrapper around the new shell script. Rationale: * Other parts of pavelib have already been reimplemented, like Python unit tests. We're following that trend. * The Python logic in pavelib is harder to understand than simple shell scripts. * pavelib has dependencies (Python, paver, edx-platform, other libs) which means that any pavelib scripts must be executed later in the edx-platform build process than we might want them to. For example, in a Dockerfile, it might be more performant to process npm assets *before* installing Python, but as long as we are still using pavelib, that is not an option. * The benefits of paver have been eclipsed by other tools, like Docker (for requisite management) and Click (for CLI building). * In the next couple commits, we make improvements to process-npm-assets.sh. These improvements would have been possible in the pavelib implementation, but would have been more complicated. Part of https://github.com/openedx/wg-developer-experience/issues/150 Closes https://github.com/openedx/edx-platform/issues/31606 --- pavelib/assets.py | 84 +------------------- scripts/assets/copy-node-modules.sh | 119 ++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 83 deletions(-) create mode 100755 scripts/assets/copy-node-modules.sh diff --git a/pavelib/assets.py b/pavelib/assets.py index ad933de19596..01308ce93a86 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', ], @@ -604,60 +571,11 @@ 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) + sh(cmd('/bin/sh', 'scripts/assets/copy-node-modules.sh')) @task diff --git a/scripts/assets/copy-node-modules.sh b/scripts/assets/copy-node-modules.sh new file mode 100755 index 000000000000..29d18fa38d92 --- /dev/null +++ b/scripts/assets/copy-node-modules.sh @@ -0,0 +1,119 @@ +#!/bin/sh +# +# Copy certain node_modules to other places in edx-platform. +# +# Run this from the root of edx-platform, after node_modules are installed, +# but before trying to compile static assets. +# +# Background: +# +# Earlier in edx-platform's development, JS and CSS dependencies were +# committed directly (a.k.a. "vendored in") to the platform. +# Later on, as NPM became popular, new edx-platform frontends began +# using package.json to specify dependencies, and of course those new +# dependencies were installed into node_modules. +# +# Unfortunately, not all old pages were updated to use package.json. +# However, rather then continuing to vendor-in the dependencies for +# the old pages, it was decided to copy the required files from +# node_modules to the old "vendor" directories. That way, the old +# pages' dependencies would remain synced with the newer pages' +# dependencies. That is what this script does. +# +# This was formerly implemented in pavelib/assets.py. As we are moving +# away from paver, it was reimplemented in this shell script. +# +# NOTE: This uses plain POSIX `sh` instead of `bash` for the purpose of +# maximum portability. + +USAGE="Usage: $0" + +NODE_MODULES_PATH="./node_modules" + +# Vendor destination paths for assets. +# These are not configurable yet, but that could be changed if necessary. +JS_VENDOR_PATH="./common/static/common/js/vendor" +CSS_VENDOR_PATH="./common/static/common/css/vendor" + +# Enable stricter sh behavior. +set -eu + +# Parse options. +while [ $# -gt 0 ]; do + case $1 in + --help) + echo "$USAGE" + exit 0 + ;; + *) + echo "Error: Unrecognized option: $1" + echo "$USAGE" + exit 1 + ;; + esac +done + +echo "-------------------------------------------------------------------------" +echo "Copying shared node_modules to 'vendor' directories..." +echo " Working directory == '$(pwd)'" +echo " NODE_MODULES_PATH == '$NODE_MODULES_PATH'" +echo " CSS_VENDOR_PATH == '$CSS_VENDOR_PATH'" +echo " JS_VENDOR_PATH == '$JS_VENDOR_PATH'" +echo "-------------------------------------------------------------------------" + +# Input validation +if ! [ -d "$NODE_MODULES_PATH" ]; then + echo "Error: not a directory: $NODE_MODULES_PATH" + exit 1 +fi +if ! [ -d ./common ]; then + echo "Error: not a directory: ./common" + echo "Hint: $0 must be run from the root of the edx-platform directory!" + exit 1 +fi + +# Echo lines back to user. +set -x + +# Create vendor directories. +mkdir -p common/static/common/js/vendor +mkdir -p common/static/common/css/vendor + +# Copy studio-frontend assets into into vendor directory. +find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \ + -type f \! -name \*.css \! -name \*.css.map -print0 | \ + xargs --null cp --target-directory="$JS_VENDOR_PATH" +find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \ + -type f \( -name \*.css -o -name \*.css.map \) -print0 | \ + xargs --null cp --target-directory="$CSS_VENDOR_PATH" + +# Copy certain node_modules scripts into "vendor" directory. +cp --force \ + "$NODE_MODULES_PATH/backbone.paginator/lib/backbone.paginator.js" \ + "$NODE_MODULES_PATH/backbone/backbone.js" \ + "$NODE_MODULES_PATH/bootstrap/dist/js/bootstrap.bundle.js" \ + "$NODE_MODULES_PATH/hls.js/dist/hls.js" \ + "$NODE_MODULES_PATH/jquery-migrate/dist/jquery-migrate.js" \ + "$NODE_MODULES_PATH/jquery.scrollto/jquery.scrollTo.js" \ + "$NODE_MODULES_PATH/jquery/dist/jquery.js" \ + "$NODE_MODULES_PATH/moment-timezone/builds/moment-timezone-with-data.js" \ + "$NODE_MODULES_PATH/moment/min/moment-with-locales.js" \ + "$NODE_MODULES_PATH/picturefill/dist/picturefill.js" \ + "$NODE_MODULES_PATH/requirejs/require.js" \ + "$NODE_MODULES_PATH/underscore.string/dist/underscore.string.js" \ + "$NODE_MODULES_PATH/underscore/underscore.js" \ + "$NODE_MODULES_PATH/which-country/index.js" \ + "$JS_VENDOR_PATH" + +# Copy certain node_modules developer scripts into "vendor" directory. +# Since they're just developer libraries, they might not exist in a production build pipeline. +# So, let the error pass silently (`... || true`) if the copy fails. +cp --force "$NODE_MODULES_PATH/sinon/pkg/sinon.js" "$JS_VENDOR_PATH" || true +cp --force "$NODE_MODULES_PATH/squirejs/src/Squire.js" "$JS_VENDOR_PATH" || true + +# Stop echoing. +set +x + +echo "-------------------------------------------------------------------------" +echo "Done copying shared node_modules." +echo "-------------------------------------------------------------------------" From 1c65ca13df0ad4866ee42173dfe7cec9ef6c9fb7 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Tue, 17 Jan 2023 11:08:57 -0500 Subject: [PATCH 2/3] build: make node_modules path configurable in copy-node-modules.sh The problem: In Tutor, NPM packages are installet d into the openedx image at /openedx/edx-platform/node_modules. So, when you bind-mount your own edx-platform, you override that node_modules folder. Then, you must re-install node_modules yourself (`tutor dev run --mount=edx-platform lms npm install`), which takes a long time and is completely redundant with the node_modules that you had to download when you downloaded the openedx image. If you forget to do this, then your edx-platform frontend will be broken. The solution: In Tutor: Move node_modules somewhere else in the openedx image, such as /openedx/node_modules. What this commit changes: Add a `--node-modules` parameter to copy-node-modules.sh, thus helping remove the assumption that node_modules must be a child directory of edx-platform. Part of https://github.com/openedx/wg-developer-experience/issues/150 --- scripts/assets/copy-node-modules.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/scripts/assets/copy-node-modules.sh b/scripts/assets/copy-node-modules.sh index 29d18fa38d92..cad7ef908e46 100755 --- a/scripts/assets/copy-node-modules.sh +++ b/scripts/assets/copy-node-modules.sh @@ -26,8 +26,11 @@ # NOTE: This uses plain POSIX `sh` instead of `bash` for the purpose of # maximum portability. -USAGE="Usage: $0" +USAGE="Usage: $0 [ (-n|--node-modules) NODE_MODULES_PATH ]" +# By default, we look for node_modules in the current directory. +# Some Open edX distributions may want node_modules to be located somewhere +# else, so we let this be configured with -n|--node-modules. NODE_MODULES_PATH="./node_modules" # Vendor destination paths for assets. @@ -41,6 +44,16 @@ set -eu # Parse options. while [ $# -gt 0 ]; do case $1 in + -n|--node-modules) + shift + if [ $# -eq 0 ]; then + echo "Error: Missing value for -n/--node-modules" + echo "$USAGE" + exit 1 + fi + NODE_MODULES_PATH="$1" + shift + ;; --help) echo "$USAGE" exit 0 From 1cc3fe3ba3cea35d8846f9d891d392099d9e9877 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Mon, 9 Jan 2023 09:55:14 -0500 Subject: [PATCH 3/3] build: copy edx-ui-toolkit instead of symlinking to it The problem: (See previous commit) The solution: (See previous commit) What this commit changes: In order to allow certain especially old parts of edx-platform to use edx-ui-toolkit without involving webpack, the platform has a symlink: $EDX_PLATFORM/common/static/edx-ui-toolkit/js -> $EDX_PLATFORM/node_modules Unfortunately, this symlink is based on the assumption that node_modules will always be a child directory of edx-platform. In order to eliminate this assumption, we delete the symlink, and then have the existing `scripts/assets/copy-node-modules.sh` script copy edx-ui-toolkit/js from node_modules over to where the symlink used to be. Part of https://github.com/openedx/wg-developer-experience/issues/150 --- common/static/.gitignore | 1 + common/static/edx-ui-toolkit/js | 1 - scripts/assets/copy-node-modules.sh | 17 +++++++++++++++-- 3 files changed, 16 insertions(+), 3 deletions(-) delete mode 120000 common/static/edx-ui-toolkit/js diff --git a/common/static/.gitignore b/common/static/.gitignore index f2c422d5b03f..a5b23eeaeadc 100644 --- a/common/static/.gitignore +++ b/common/static/.gitignore @@ -1 +1,2 @@ xmodule +edx-ui-toolkit diff --git a/common/static/edx-ui-toolkit/js b/common/static/edx-ui-toolkit/js deleted file mode 120000 index 0e607da1cd97..000000000000 --- a/common/static/edx-ui-toolkit/js +++ /dev/null @@ -1 +0,0 @@ -../../../node_modules/edx-ui-toolkit/src/js \ No newline at end of file diff --git a/scripts/assets/copy-node-modules.sh b/scripts/assets/copy-node-modules.sh index cad7ef908e46..6bceffa122b0 100755 --- a/scripts/assets/copy-node-modules.sh +++ b/scripts/assets/copy-node-modules.sh @@ -37,6 +37,7 @@ NODE_MODULES_PATH="./node_modules" # These are not configurable yet, but that could be changed if necessary. JS_VENDOR_PATH="./common/static/common/js/vendor" CSS_VENDOR_PATH="./common/static/common/css/vendor" +EDX_UI_TOOLKIT_VENDOR_PATH="./common/static/edx-ui-toolkit" # Enable stricter sh behavior. set -eu @@ -72,6 +73,7 @@ echo " Working directory == '$(pwd)'" echo " NODE_MODULES_PATH == '$NODE_MODULES_PATH'" echo " CSS_VENDOR_PATH == '$CSS_VENDOR_PATH'" echo " JS_VENDOR_PATH == '$JS_VENDOR_PATH'" +echo " EDX_UI_TOOLKIT_VENDOR_PATH == '$EDX_UI_TOOLKIT_VENDOR_PATH'" echo "-------------------------------------------------------------------------" # Input validation @@ -89,8 +91,9 @@ fi set -x # Create vendor directories. -mkdir -p common/static/common/js/vendor -mkdir -p common/static/common/css/vendor +mkdir -p "$JS_VENDOR_PATH" +mkdir -p "$CSS_VENDOR_PATH" +mkdir -p "$EDX_UI_TOOLKIT_VENDOR_PATH" # Copy studio-frontend assets into into vendor directory. find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \ @@ -100,6 +103,16 @@ find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \ -type f \( -name \*.css -o -name \*.css.map \) -print0 | \ xargs --null cp --target-directory="$CSS_VENDOR_PATH" +# Copy edx-ui-toolkit's JS to into its own special directory. +# Note: $EDX_UI_TOOLKIT_VENDOR_PATH/js used to be a git-managed symlink. +# To avoid confusing behavior for folks who still have the js/ symlink +# hanging around in their repo, the safest thing to do is to remove +# the target js/ before copying in js/ fresh from the source. +rm -rf "$EDX_UI_TOOLKIT_VENDOR_PATH/js" +cp --recursive \ + "$NODE_MODULES_PATH/edx-ui-toolkit/src/js" \ + "$EDX_UI_TOOLKIT_VENDOR_PATH" + # Copy certain node_modules scripts into "vendor" directory. cp --force \ "$NODE_MODULES_PATH/backbone.paginator/lib/backbone.paginator.js" \