Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: remove dependency on Paver scripts #1042

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions changelog.d/20240318_154804_kyle_assets_build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
- 💥[Feature] The `openedx-assets` command is replaced with `npm run` subcommands.
This will slightly reduce the build time for edx-platform assets and comprehensive themes.
It will also open up the door for more significant build time reductions in the future.
Here is a migration guide, where each command is to be run in the `lms` or `cms` container.

**Before** | **After**
-----------------------------------------|-------------------------------------------------------------------------------------
`openedx-assets build --env=prod ARGS` | `npm run build -- ARGS`
`openedx-assets build --env=dev ARGS` | `npm run build-dev -- ARGS`
`openedx-assets common --env=prod ARGS` | `npm run compile-sass -- --skip-themes ARGS`
`openedx-assets common --env=dev ARGS` | `npm run compile-sass-dev -- --skip-themes ARGS`
`openedx-assets webpack --env=prod ARGS` | `npm run webpack -- ARGS`
`openedx-assets webpack --env=dev ARGS` | `npm run webpack-dev -- ARGS`
`openedx-assets npm` | `npm run postinstall` (`npm clean-install` runs this automatically)
`openedx-assets xmodule` | (no longer necessary)
`openedx-assets collect ARGS` | `./manage.py lms collectstatic --noinput ARGS && ./manage.py cms collectstatic ARGS`
`openedx-assets watch-themes ARGS` | `npm run watch-themes -- ARGS`

For more details, see the [deprecation notice for paver](https://github.com/openedx/edx-platform/issues/34467)
and the [static assets reference](https://github.com/openedx/edx-platform/tree/open-release/redwood.master/docs/references/static-assets.rst)
in edx-platform.
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved
Empty file added docs/__init__.py
Empty file.
4 changes: 2 additions & 2 deletions docs/dev.rst
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ To open a python shell in the LMS or CMS, run::

You can then import edx-platform and django modules and execute python code.

To rebuild assets, you can use the ``openedx-assets`` command that ships with Tutor::
To rebuild assets, you can run the ``build-dev`` NPM script that comes with edx-plaform::

tutor dev run lms openedx-assets build --env=dev
tutor dev run lms npm run build-dev


.. _specialized for developer usage:
Expand Down
2 changes: 0 additions & 2 deletions tutor/templates/apps/openedx/config/cms.env.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ CELERY_BROKER_USER: "{{ REDIS_USERNAME }}"
CELERY_BROKER_PASSWORD: "{{ REDIS_PASSWORD }}"
ALTERNATE_WORKER_QUEUES: "lms"
ENABLE_COMPREHENSIVE_THEMING: true
COMPREHENSIVE_THEME_DIRS: ["/openedx/themes"]
STATIC_ROOT_BASE: "/openedx/staticfiles"
Comment on lines -30 to -31
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: Since this edx-platform change, we can specify these as environment variables (in the Dockerfile) and they will be automatically loaded into Django settings.

EMAIL_BACKEND: "django.core.mail.backends.smtp.EmailBackend"
EMAIL_HOST: "{{ SMTP_HOST }}"
EMAIL_PORT: {{ SMTP_PORT }}
Expand Down
2 changes: 0 additions & 2 deletions tutor/templates/apps/openedx/config/lms.env.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ CELERY_BROKER_USER: "{{ REDIS_USERNAME }}"
CELERY_BROKER_PASSWORD: "{{ REDIS_PASSWORD }}"
ALTERNATE_WORKER_QUEUES: "cms"
ENABLE_COMPREHENSIVE_THEMING: true
COMPREHENSIVE_THEME_DIRS: ["/openedx/themes"]
STATIC_ROOT_BASE: "/openedx/staticfiles"
EMAIL_BACKEND: "django.core.mail.backends.smtp.EmailBackend"
EMAIL_HOST: "{{ SMTP_HOST }}"
EMAIL_PORT: {{ SMTP_PORT }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
FRONTEND_REGISTER_URL = LMS_ROOT_URL + '/register'

# Create folders if necessary
for folder in [LOG_DIR, MEDIA_ROOT, STATIC_ROOT_BASE, ORA2_FILEUPLOAD_ROOT]:
for folder in [LOG_DIR, MEDIA_ROOT, STATIC_ROOT, ORA2_FILEUPLOAD_ROOT]:
if not os.path.exists(folder):
os.makedirs(folder, exist_ok=True)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
}

# Create folders if necessary
for folder in [DATA_DIR, LOG_DIR, MEDIA_ROOT, STATIC_ROOT_BASE, ORA2_FILEUPLOAD_ROOT]:
for folder in [DATA_DIR, LOG_DIR, MEDIA_ROOT, STATIC_ROOT, ORA2_FILEUPLOAD_ROOT]:
if not os.path.exists(folder):
os.makedirs(folder, exist_ok=True)

Expand Down
41 changes: 20 additions & 21 deletions tutor/templates/build/openedx/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ RUN ln -s /openedx/node_modules /openedx/edx-platform/node_modules

ENV PATH /openedx/venv/bin:./node_modules/.bin:/openedx/nodeenv/bin:${PATH}
ENV VIRTUAL_ENV /openedx/venv/
ENV COMPREHENSIVE_THEME_DIRS /openedx/themes
ENV STATIC_ROOT_LMS /openedx/staticfiles
ENV STATIC_ROOT_CMS /openedx/staticfiles/studio

WORKDIR /openedx/edx-platform

{# Install auto-mounted directories as Python packages. #}
Expand Down Expand Up @@ -204,27 +208,22 @@ ENV PATH /openedx/bin:${PATH}

{{ patch("openedx-dockerfile-pre-assets") }}

# Collect production assets. By default, only assets from the default theme
# Build & collect production assets. By default, only assets from the default theme
# will be processed. This makes the docker image lighter and faster to build.
# Only the custom themes added to /openedx/themes will be compiled.
# Here, we don't run "paver update_assets" which is slow, compiles all themes
# and requires a complex settings file. Instead, we decompose the commands
# and run each one individually to collect the production static assets to
# /openedx/staticfiles.
ENV NO_PYTHON_UNINSTALL 1
ENV NO_PREREQ_INSTALL 1
# We need to rely on a separate openedx-assets command to accelerate asset processing.
# For instance, we don't want to run all steps of asset collection every time the theme
# is modified.
RUN openedx-assets xmodule \
&& openedx-assets npm \
&& openedx-assets webpack --env=prod \
&& openedx-assets common
COPY --chown=app:app ./themes/ /openedx/themes/
RUN openedx-assets themes \
&& openedx-assets collect --settings=tutor.assets \
# De-duplicate static assets with symlinks
&& rdfind -makesymlinks true -followsymlinks true /openedx/staticfiles/
RUN npm run postinstall # Postinstall artifacts are stuck in nodejs-requirements layer. Create them here too.
RUN npm run compile-sass -- --skip-themes
RUN npm run webpack

# Now that the default theme is built, build any custom themes
COPY --chown=app:app ./themes/ /openedx/themes
RUN npm run compile-sass -- --skip-default

# and finally, collect assets for the production image,
# de-duping assets with symlinks.
RUN ./manage.py lms collectstatic --noinput --settings=tutor.assets && \
./manage.py cms collectstatic --noinput --settings=tutor.assets && \
# De-duplicate static assets with symlinks \
rdfind -makesymlinks true -followsymlinks true /openedx/staticfiles/

# Create a data directory, which might be used (or not)
RUN mkdir /openedx/data
Expand Down Expand Up @@ -277,7 +276,7 @@ ENV PYTHONBREAKPOINT=ipdb.set_trace
# static assets, then production assets will be served instead.
RUN rm -r /openedx/staticfiles && \
mkdir /openedx/staticfiles && \
openedx-assets webpack --env=dev
npm run build-dev

{{ patch("openedx-dev-dockerfile-post-python-requirements") }}

Expand Down
218 changes: 0 additions & 218 deletions tutor/templates/build/openedx/bin/openedx-assets

This file was deleted.

3 changes: 1 addition & 2 deletions tutor/templates/build/openedx/settings/cms/assets.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% include "build/openedx/settings/partials/assets.py" %}

STATIC_ROOT = path(STATIC_ROOT_BASE) / 'studio'
WEBPACK_LOADER['DEFAULT']['STATS_FILE'] = STATIC_ROOT / "webpack-stats.json"
WEBPACK_LOADER['DEFAULT']['STATS_FILE'] = path(STATIC_ROOT) / "webpack-stats.json"

derive_settings(__name__)
3 changes: 1 addition & 2 deletions tutor/templates/build/openedx/settings/lms/assets.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{% include "build/openedx/settings/partials/assets.py" %}

STATIC_ROOT = path(STATIC_ROOT_BASE)
WEBPACK_LOADER['DEFAULT']['STATS_FILE'] = STATIC_ROOT / "webpack-stats.json"
WEBPACK_LOADER['DEFAULT']['STATS_FILE'] = path(STATIC_ROOT) / "webpack-stats.json"

derive_settings(__name__)
3 changes: 0 additions & 3 deletions tutor/templates/build/openedx/settings/partials/assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
from openedx.core.lib.derived import derive_settings

ENABLE_COMPREHENSIVE_THEMING = True
COMPREHENSIVE_THEME_DIRS.append('/openedx/themes')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my system, ENABLE_COMPREHENSIVE_THEMING is already enabled and have confirmed this using shell of lms container. and in ENV variables, COMPREHENSIVE_THEME_DIRS is added. the problem is LMS doesn't pick COMPREHENSIVE_THEME_DIRS from Env variables. It picks its value using themes_dirs = get_theme_base_dirs_from_settings(settings.COMPREHENSIVE_THEME_DIRS).

There are two options here:

  1. move COMPREHENSIVE_THEME_DIRS back to settings variable
  2. Make changes in edx-platform to pick COMPREHENSIVE_THEME_DIRS value using get_env_setting

Any of these solution is not being done in https://github.com/openedx/edx-platform/pull/34318/files.

image

If there seems other alternative solution, feel free to propose here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for testing @hinakhadim . Are you sure you are on the latest master for edx-platform? It's possible I missed something, but my intention was to load settings.COMPREHENSIVE_THEME_DIRS from the environment variable of the same name: https://github.com/openedx/edx-platform/blob/0d4adaa5d70364fe9ff0857b15cbc58cf952f584/cms/envs/common.py#L2205

COMPREHENSIVE_THEME_DIRS = os.environ.get("COMPREHENSIVE_THEME_DIRS", "").split(":")

This behavior was added in this commit openedx/edx-platform@21a1235 (original PR)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for information, @kdmccormick. I have tested with latest master for edx-platform and it is now loading the indigo theme and picking value of COMPREHENSIVE_THEME_DIRS from env. So, everything is fine from my side.


STATIC_ROOT_BASE = '/openedx/staticfiles'

SECRET_KEY = 'secret'
XQUEUE_INTERFACE = {
Expand Down
2 changes: 0 additions & 2 deletions tutor/templates/build/openedx/settings/partials/i18n.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
from ..common import *
from openedx.core.lib.derived import derive_settings

STATIC_ROOT_BASE = '/openedx/staticfiles'

SECRET_KEY = 'secret'
XQUEUE_INTERFACE = {
'django_auth': None,
Expand Down
2 changes: 1 addition & 1 deletion tutor/templates/dev/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ services:
# Additional service for watching theme changes
watchthemes:
<<: *openedx-service
command: openedx-assets watch-themes --env dev
command: npm run watch-sass
restart: unless-stopped

{% if RUN_ELASTICSEARCH and is_docker_rootless() %}
Expand Down
Loading
Loading