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

build: restructure "vendor" directories to improve build #32835

Closed

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jul 25, 2023

Context

(Part of: openedx/wg-devops#21)

Our overall goal is to minimize Docker image rebuild time while also maintaining Docker image correctness for edx-platform developers.

As of recently, Tutor lets edx-platform developers use their local edx-platform repository as input into the Docker image build. Before this, developers would either have to:

  • sacrifice speed by pushing their code to a remote git repository+branch and then tell Tutor to build from that repository+branch, or
  • sacrifice correctness by bind-mounting edx-platform into a container running an image build from master (this is also what Devstack does).

Tutor achieves this improvement by bind-mounting several source items (files or directories) from edx-platform into the Docker build context during each build step. When choosing which source items to bind-mount at each build step, we have several constraints:

  1. To achieve correctness, we must bind-mount all items that are significant to the build step.
  2. To achieve correctness, we must not bind-mount items that were previously modified by the build (because bind-mounting would overwrite the modifications).
  3. To maximize speed, we avoid bind-mounting items that are not significant to the build step.
  4. Finally, Docker mandates that source directories cannot be written to while they are bind-mounted.

For example, look at the build step that installs NPM packages:

  1. We bind-mount package.json, package-lock.json, and copy-node-modules.sh as source items, as they all affect npm clean-install.
  2. This step generates node_modules. In future build steps, we cannot bind-mount node_modules (or any parent directory) from edx-platform, because that would clobber the output of npm clean-install.
  3. We do not bind-mount Python requirements, JS sources, etc., as changes to them should not affect npm clean-install.
  4. None of the bind-mounted files are modified by npm clean-install. Note that running npm install would fail because it could modify package-lock.json.

The problem is that the current structure of edx-platform, particularly the "vendor" folders, makes it impossible to follow these constraints when building static assets. Tutor currently works around this by sacrificing some correctness: static assets sources are not bind-mounted, so local changes to static assets will not be reflected in the image build.

This PR fixes the structure of edx-platform vendor folders. This will benefit Tutor users, but it will generally benefit anyone who is trying to write a finely-tuned Dockerfile for edx-platform.

Change Descriptions

Changes Visualized

 BEFORE                                                         >>   AFTER
=======================================================================================================================================
 edx-platform                                                   >>   edx-platform
 ├── lms                                                        >>   ├── lms
 │   └── static                                                 >>   │   └── static
 │       └── css                                                >>   │       └── css
 │           ├── ...                                            >>   │           ├── ...
 │           ├── ...  (Generated:                               >>   │           ├── ...  (Generated:
 │           ├── ...   Compiled LMS CSS)                        >>   │           ├── ...   Compiled LMS CSS)
 │           ├── ...                                            >>   │           └── ...
 │           └── vendor                                         >>   │
 │               ├── ...                                        >>   │
 │               ├── ... (Source:                               >>   │
 │               ├── ...  LMS "vendored-in" CSS libs)           >>   │
 │               └── ...                                        >>   │
 └── common                                                     >>   └── common
     └── static                                                 >>       └── static
         ├── common                                             >>           ├── common
         │   ├── js                                             >>           │   ├── js
         │   │   ├── ...                                        >>           │   │   ├── ...
         │   │   ├── ...(Source:                                >>           │   │   ├── ...(Source:
         │   │   ├── ... LMS+CMS JS to be Webpacked)            >>           │   │   ├── ... LMS+CMS JS to be Webpacked)
         │   │   ├── ...                                        >>           │   │   ├── ...
         │   │   └── vendor                                     >>           │   │   └── vendor ==[symlink]==> ../../node_copies/js
         │   │       ├── ...                                    >>           │   │
         │   │       ├── ... (Generated/Source:                 >>           │   │
         │   │       ├── ...  JS copied from node_modules)      >>           │   │
         │   │       └── ...                                    >>           │   │
         │   └── css                                            >>           │   └── css                      
         │       └── vendor                                     >>           │       └── vendor ==[symlink]==> ../../node_copies/css
         │           ├── ...                                    >>           │
         │           ├── ... (Generated/Source:                 >>           │
         │           ├── ...  CSS copied from node_modules)     >>           │
         │           └── ...                                    >>           │
         └── css                                                >>           ├── css
             ├── ...                                            >>           │   ├── ...
             ├── ... (Source:                                   >>           │   ├── ... (Source:
             ├── ...  Common styles)                            >>           │   ├── ...  Common styles)
             ├── ...                                            >>           │   ├── ...
             └── vendor                                         >>           │   └── vendor
                 ├── ...                                        >>           │       ├── ...
                 ├── ... (Source:                               >>           │       ├── ... (Source:
                 ├── ...  Common "vendored-in" CSS libs         >>           │       ├── ...  Common+LMS "vendored-in" CSS libs)
                 └── ...                                        >>           │       └── ...
                                                                >>           └── node_copies
                                                                >>               ├── css                  
                                                                >>               │   ├── ...       
                                                                >>               │   ├── ... (Generated/Source:
                                                                >>               │   ├── ...  CSS copied from node_modules)
                                                                >>               │   └── ...      
                                                                >>               └── js  
                                                                >>                   ├── ...
                                                                >>                   ├── ... (Generated/Source:
                                                                >>                   ├── ...  JS copied from node_modules)
                                                                >>                   └── ...

Change 1: build: lms/static/css/vendor/* -> common/static/css/vendor

Sass compilation includes the LMS's "vendored-in" CSS libraries at lms/static/css/vendor as a source, and output CSS is written to the parent directory, lms/static/css. This violates the constraint that source directories cannot be written to while they are bind-mounted.

To resolve the issue, we merge the contents of lms/static/css/vendor into common/static/css/vendor. (The directory common/static/css only contains sources: no CSS is outputted there.)

Now, common/static/css only contains sources for the Sass build, and lms/static/css is only used for output.

Change 2: common/static/common/[js|css]/vendor -> common/static/node_copies/[js|css]

To support our old RequireJS-based frontends, a post-install hook on npm clean-install copies certain JS & CSS modules into common/static/common/[js|css]/vendor. However, the copied modules' ancestor directory, common/static/common, is a source for the Webpack build that we need to bind-mount. This violates the constraint that we must not bind-mount items that were previously modified by the build.

To resolve the issue, we relocate common/static/common/[js|css]/vendor to new directories, common/static/node_copies/[js|css]. We provide a symlink from the original location to a new location.

Now, common/static/common and common/static/node_copies can both be used as inputs to the Webpack build, but the latter will not be clobbered when we bind-mount the former. Furthermore, the new directory name ("node_copies") is
more illustrative than the old one ("vendor").

Note: I originally attempted to make this change without the symlink--I updated all references to the old path to the new path. I struggled for several hours to get tests passing for JS using RequireJS, and concluded that I don't understand our RequireJS setup well enough to safely make that change.

Bonus change: log npm run postinstall steps to STDOUT so they are visible in CI

This is a small logging improvement that I ended up needing in order to debug this PR, which was helpful enough that I've decided to keep in the PR.

Testing

Here's how I tested:

Setup

I tested this edx-platform branch three different configurations, using latest Tutor Nightly and Devstack:

tutor dev (with bindmount)

# Nav into edx-platform, switch to this branch, and clean out generated files
cd ~/openedx/edx-platform
sudo chown "$USER" -R .  # fix perms in case you were just using devstack
git switch kdmccormick/asset-folders
git clean . -f
make clean

# Build image without bind-mount, and provision repo
tutor mounts remove .
tutor config save --unset EDX_PLATFORM_VERSION --unset EDX_PLATFORM_REPOSITORY
tutor images build openedx-dev
tutor dev launch --non-interactive

# Build image with this repo bind-mounted and re-provision/re-launch
tutor mounts add .
tutor images build openedx-dev
tutor dev launch --non-interactive

tutor local (no bindmount)

# Un-mount edx-platform
tutor mounts remove .

# Config Tutor to build prod image from this remote branch
tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/kdmccormick/edx-platform
tutor config save --set EDX_PLATFORM_VERSION=kdmccormick/asset-folders
tutor images build openedx

# Provision & launch.
tutor local launch --non-interactive

devstack

# Nav into edx-platform, switch to this branch, and clean out generated files
cd ~/openedx/edx-platform
git switch kdmccormick/asset-folders
git clean . -f
make clean

# Nav to devstack, provision, and start.
cd ~/openedx/devstack
make dev.provision.lms+cms
make dev.start.lms+cms

Test playbook

I ran through this basic flow, keeping an eye out for any visual issues or misbehaving components:

  • Go to Studio home (logged out)
  • Log in as admin
  • Click into a course
  • Add a section, subsection, and unit
  • Click into a unit
  • Add a problem, video, html, and drag&drop
  • Publish
  • Edit each of those
  • Publish
  • Click "View Preview"
  • Navigate backwards, into previous sequence
  • Click "View in Studio"
  • Click "View Live"
  • Navigate forwards, into next sequence
  • Go into the dev tools -> netowrk log, hard refresh, and ensure there is a file loaded named "jquery.treeview.css". This was one of the vendored-in CSS files that was moved.
  • Go to instructor dashboard
  • Click around a bit

@kdmccormick kdmccormick force-pushed the kdmccormick/asset-folders branch 2 times, most recently from 5a94bf3 to 9e4d32b Compare July 27, 2023 19:04
@kdmccormick kdmccormick marked this pull request as ready for review July 27, 2023 19:08
@kdmccormick
Copy link
Member Author

@feanil Looks like I'll need to merge this one last assets PR in order for the optimized Tutor image build to work.

The change is here kinda nuanced, and I'm not sure I did the best job describing it. Review if you can; if not, I can come back and rework the description once I'm back from PTO.

@kdmccormick kdmccormick requested a review from feanil July 27, 2023 19:11
@kdmccormick
Copy link
Member Author

Eh, nevermind, tests are failing. Putting this back in draft state for now.

@kdmccormick kdmccormick marked this pull request as draft July 27, 2023 19:50
@kdmccormick kdmccormick removed the request for review from feanil July 27, 2023 19:50
@kdmccormick kdmccormick changed the title Rearrange static vendor folders for build optimizability build: restructure "vendor" directories for build speed & correctness Aug 16, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/asset-folders branch 6 times, most recently from fd9111f to 389b778 Compare August 22, 2023 20:14
@kdmccormick kdmccormick force-pushed the kdmccormick/asset-folders branch 3 times, most recently from 3ac4c5c to 403913b Compare August 24, 2023 18:16
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 24, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/asset-folders branch 3 times, most recently from 39e9926 to 972f26c Compare August 24, 2023 20:36
@kdmccormick kdmccormick changed the title build: restructure "vendor" directories for build speed & correctness build: restructure "vendor" directories to improve build Aug 25, 2023
@kdmccormick kdmccormick removed the open-source-contribution PR author is not from Axim or 2U label Aug 25, 2023
@openedx openedx deleted a comment from openedx-webhooks Aug 25, 2023
@kdmccormick kdmccormick marked this pull request as ready for review August 25, 2023 15:15
Sass compilation includes the LMS's "vendored-in" CSS libraries at
lms/static/css/vendor as a source, and output CSS is written to the
parent directory, lms/static/css. This violates the constraint that
source directories cannot be written to while they are bind-mounted.

To resolve the issue, we merge the contents of lms/static/css/vendor
into common/static/css/vendor. (The directory common/static/css only
contains sources: no CSS is outputted there.)

Now, common/static/css only contains sources for the Sass build, and
lms/static/css is only used for output.

More details: openedx#32835
Part of: https://github.com/openedx/wg-developer-experience/issues/166
…ies/[js|css]

To support our old RequireJS-based frontends, a post-install hook on npm
clean-install copies certain JS & CSS modules into
common/static/common/[js|css]/vendor. However, the copied modules'
ancestor directory, common/static/common, is a source for the Webpack
build that we need to bind-mount. This violates the constraint that we
must not bind-mount items that were previously modified by the build.

To resolve the issue, we relocate common/static/common/[js|css]/vendor
to new directories, common/static/node_copies/[js|css]. We provide a
symlink from the original location to a new location.

Now, common/static/common and common/static/node_copies can both be used
as inputs to the Webpack build, but the latter will not be clobbered
when we bind-mount the former. Furthermore, the new directory name
("node_copies") is more illustrative than the old one ("vendor").

(Note: I originally attempted to make this change without the symlink--I
updated all references to the old path to the new path. I struggled for
several hours to get tests passing for JS using RequireJS, and concluded
that I don't understand our RequireJS setup well enough to safely make
that change.)

More details: openedx#32835
Part of: https://github.com/openedx/wg-developer-experience/issues/166
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 31, 2023
@openedx openedx deleted a comment from openedx-webhooks Aug 31, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @kdmccormick! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

⚠️ We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. Please see the CONTRIBUTING file for more information. If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by editing the PR title. If the problem persists, you can tag the @openedx/cla-problems team in a comment on your PR for further assistance.

@kdmccormick
Copy link
Member Author

I've realized that I can use COPY --from=(bind-mount) rather than RUN --mount=from=(bind-mount) in Dockerfiles, which gives me much more flexibility to work around the weirdness of these vendor directories as they are today. I'm going to close this in order to avoid sinking more time into it.

Two much smaller followup PRs:
#33142
#33143

@openedx-webhooks
Copy link

@kdmccormick Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@kdmccormick Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@kdmccormick kdmccormick deleted the kdmccormick/asset-folders branch August 31, 2023 12:39
@kdmccormick kdmccormick restored the kdmccormick/asset-folders branch June 3, 2024 12:47
kdmccormick added a commit to kdmccormick/edx-platform that referenced this pull request Jun 3, 2024
Sass compilation includes the LMS's "vendored-in" CSS libraries at
lms/static/css/vendor as a source, and output CSS is written to the
parent directory, lms/static/css. This violates the constraint that
source directories cannot be written to while they are bind-mounted.

To resolve the issue, we merge the contents of lms/static/css/vendor
into common/static/css/vendor. (The directory common/static/css only
contains sources: no CSS is outputted there.)

Now, common/static/css only contains sources for the Sass build, and
lms/static/css is only used for output.

More details: openedx#32835
Part of: https://github.com/openedx/wg-developer-experience/issues/166
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants