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.

Sign up for GitHub

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

Jump to bottom

Build edx-platform assets without Paver #31604

Closed
5 of 6 tasks
Tracked by #31800
kdmccormick opened this issue Jan 19, 2023 · 2 comments
Closed
5 of 6 tasks
Tracked by #31800

Build edx-platform assets without Paver #31604

kdmccormick opened this issue Jan 19, 2023 · 2 comments
Assignees
@kdmccormick

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Jan 19, 2023 •
edited
Loading

Background

This issue is at the overlap of two epics:

ADR:

Tasks

  • Rewrite build script, including:
    • node_modules copying
    • xmodule_assets invocation
    • Webpack invocation
    • default SCSS compilation
    • theme SCSS compilation
  • Run script in CI
  • Add & document management command for fetching settings & theme

PRs
Beta Give feedback
  1. build: copy from node_modules using NPM postinstall hook, not Paver #32717
  2. build: npm run webpack (WIP) #32721
  3. build: npm run compile-sass (WIP) #32722
  4. build: turn paver/assets.py into a dumb wrapper around new asset build (WIP) #32723
  5. build: npm run build (experimental) #32823

Notes

Blocks:
@kdmccormick kdmccormick self-assigned this Jan 19, 2023
This was referenced Jan 19, 2023
Closed
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Feb 7, 2023
Closed
Closed
@kdmccormick kdmccormick moved this from 📥 New to 🛠️ In Progress in Developer Experience Working Group Feb 14, 2023
@kdmccormick kdmccormick changed the title Build LMS & CMS assets without Python Build edx-platform assets without Python Feb 21, 2023
This was referenced Feb 21, 2023
Closed
Closed
@kdmccormick kdmccormick changed the title Build edx-platform assets without Python Build edx-platform assets without Paver Feb 21, 2023
@kdmccormick kdmccormick mentioned this issue Feb 21, 2023
Open
@kdmccormick kdmccormick linked a pull request Feb 21, 2023 that will close this issue
[old] build: rewrite static asset build #31791
Closed
@kdmccormick
Copy link
Member Author

Implemented by #31791

@kdmccormick kdmccormick mentioned this issue Feb 21, 2023
Merged
@kdmccormick kdmccormick added the open-source-contribution PR author is not from Axim or 2U label Mar 6, 2023
kdmccormick added a commit that referenced this issue Jul 17, 2023
@kdmccormick
build: copy from node_modules using NPM postinstall hook, not Paver (#…
4b64d83 
…32717)

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. #31790 (comment)
3. https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14

Part of: #31604
kdmccormick added a commit to kdmccormick/tutor that referenced this issue Jul 17, 2023
@kdmccormick
build: copy in copy-node-modules.sh for npm post-install hook
4f1e001 
This post-install hook replaces `openedx-assets npm`.

Part of: openedx/edx-platform#31604
kdmccormick added a commit to kdmccormick/tutor that referenced this issue Jul 17, 2023
@kdmccormick
build: copy in copy-node-modules.sh for npm post-install hook
bdce02a 
This post-install hook replaces `openedx-assets npm`.

Part of: openedx/edx-platform#31604
@kdmccormick kdmccormick mentioned this issue Jul 17, 2023
Merged
kdmccormick added a commit to kdmccormick/tutor that referenced this issue Jul 18, 2023
@kdmccormick
build: copy in copy-node-modules.sh for npm post-install hook
64ac783 
This post-install hook replaces `openedx-assets npm`.

Part of: openedx/edx-platform#31604
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 20, 2023
@kdmccormick
build: npm run webpack
2fb96b2 
TODO: will copy in commit message from PR when squash-merging

Part of: openedx#31604
regisb pushed a commit to overhangio/tutor that referenced this issue Jul 21, 2023
@kdmccormick @regisb
build: copy in copy-node-modules.sh for npm post-install hook
e4ddee2 
This post-install hook replaces `openedx-assets npm`.

Part of: openedx/edx-platform#31604
@kdmccormick kdmccormick mentioned this issue Jul 24, 2023
Merged
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 26, 2023
@kdmccormick
build: npm run webpack
46e5624 
See https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: openedx#31604
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 26, 2023
@kdmccormick
build: npm run compile-sass
468bfae 
See https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: openedx#31604
kdmccormick added a commit to kdmccormick/edx-platform that referenced this issue Jul 26, 2023
@kdmccormick
build: npm run build
e1a88d9 
See https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Part of: openedx#31604
kdmccormick added a commit that referenced this issue Jul 26, 2023
@kdmccormick
build: npm run build (experimental) (#32823)
3e67719 
This PR implements much of the static assets rework ADR [1], including:

* `npm run build[-dev]`, and its subcommands,
* `npm run webpack[-dev]` and
* `npm run compile-sass[-dev]`.

This is backwards-compatible. `paver update_assets` should not be affected.
The new command warns that it is "experimental" for a few reasons:

* `npm run build` will fail in the webpack phase unless you first
run  `xmodule_assets`. This will be changed soon [2].

* We have tested the new build, but not quite so thoroughly that we'd
recommend it as the production default yet. Once the xmodule_assets
work lands, we'll share this on the forums so early adopters can try it
out.

* The commands lack some top-level documentation. Once they stabilize more,
we'll add a section to the README that explains how and when to use `npm run
build` and its subcommands and its env vars.

* `npm run watch` is not yet implemented.

References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. #32685

Part of: #31604
@kdmccormick
Copy link
Member Author

Implemented by #32823

@github-project-automation github-project-automation bot moved this from In Progress to Closed in Developer Experience Working Group Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kdmccormick kdmccormick

Labels
None yet
Projects
No open projects
Developer Experience Working Group
Status: Closed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[old] build: rewrite static asset build
1 participant
@kdmccormick