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: compile/watch sass with new npm scripts #34318

Merged
merged 3 commits into from
Apr 4, 2024
Merged

build: compile/watch sass with new npm scripts #34318

merged 3 commits into from
Apr 4, 2024
+155 −702

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Mar 1, 2024 •
edited
Loading

Description

paver commands are deprecated for managing static assets. Starting in Sumac, only npm run commands will be supported for managing static assets.

To ease the transition, both paver and npm run commands will work in Redwood. However, we want to stop using the implementations of the paver asset commands right now, as they are blocking the Python 3.11 upgrade. This will also make the removal of paver commands more straightforward come Sumac.

So, this commit turns these commands/functions:

  • paver compile_sass (used by configuration)
  • paver watch_sass (used by configuration and devstack)
  • pavelib/assets.py:_compile_sass (used by Tutor)

into very thin wrappers around the new npm run commands. Each of these paver routines now raise a loud deprecation warning, including a message of the npm run command that the operator can switch to.

We expect no impact to site operators or end users.

Also included in separate commits

  • fix: simplify npm run watch-sass so that it is more reliable
  • docs: npm scripts for static assets are no longer experimental :)

Supporting Information

Testing

With Tutor

  • Check out this branch
  • Install a theme, such as indigo: tutor plugins install indigo
  • Mount your local edx-platform repo: tutor mounts add <path to your edx-platform>
  • tutor dev stop && tutor local stop
  • tutor config save
  • tutor images build openedx openedx-dev
  • Test results of prod Sass build
    • tutor local start -d
    • tutor local do settheme indigo
    • Browse some Django-rendered pages. Good examples are the Studio outline editor, LMS Course Preview (accessible from any edited Studio unit by hitting "Preview"), the Instructor Dashboard, and the LMS XBlock Views within any course.
    • tutor local do settheme default
    • Browse some Django-rendered pages
  • Test results of dev Sass build
    • tutor dev start -d
    • Browse some Django-rendered pages
    • tutor dev do settheme indigo
    • Browse some Django-rendered pages
  • Test hot Sass recompilation
    • tutor dev run lms env EDX_PLATFORM_THEME_DIRS=/openedx/themes npm run watch-sass
    • touch "$(tutor config printroot)/env/build/openedx/themes/indigo/lms/static/sass/_background.scss"
    • Confirm that the theme rebuilds
    • NOTE: The Tutor-recommend command tutor dev run watchthemes seems to lead to an infinite recompilation loop any time any themed Sass file is changed. I have authored this PR to be technically compatible with tutor dev run watchthemes, but I'm not worried about fixing the infinite recompilation bug here.

Without Tutor

To test outside of Tutor, I recommend just building static assets however you normally would. For example, I believe paver update_assets will do the trick for folks using devstack and configuration.

Merge Considerations

I am aiming for this week (Thu March 28 if possible) in order to unblock the Python upgrade, which needs to be complete for the Redwood cut in late April.

Priorities after merging this:

@kdmccormick kdmccormick self-assigned this Mar 1, 2024
@kdmccormick kdmccormick changed the title build: turn more Paver code into a dumb wrapper around NPM build scripts build: switch to new Sass build, with compatibility wrapper for paver Mar 20, 2024
@kdmccormick kdmccormick changed the title build: switch to new Sass build, with compatibility wrapper for paver build: switch to new Sass build, with temporary paver compat wrapper Mar 20, 2024
@kdmccormick kdmccormick changed the title build: switch to new Sass build, with temporary paver compat wrapper build: switch to npm compile-sass, with temporary paver compat wrapper Mar 20, 2024
@kdmccormick kdmccormick changed the title build: switch to npm compile-sass, with temporary paver compat wrapper build: make paver compile_sass wrap npm run compile-sass Mar 20, 2024
@kdmccormick kdmccormick mentioned this pull request Mar 20, 2024
Closed
@kdmccormick kdmccormick force-pushed the kdmccormick/libsass-python branch 2 times, most recently from c9048ea to 936b18a Compare March 21, 2024 17:34
@kdmccormick kdmccormick added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 21, 2024
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick kdmccormick changed the title build: make paver compile_sass wrap npm run compile-sass build: switch to npm run compile-sass Mar 26, 2024
@kdmccormick kdmccormick changed the title build: switch to npm run compile-sass build: compile/watch sass with using new npm scripts Mar 26, 2024
@kdmccormick kdmccormick changed the title build: compile/watch sass with using new npm scripts build: compile/watch sass with new npm scripts Mar 26, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick kdmccormick marked this pull request as ready for review March 26, 2024 21:55
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick kdmccormick mentioned this pull request Mar 28, 2024
Merged
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@kdmccormick
Copy link
Member Author

@feanil friendly reminder about this PR.

The follow-up PR is also ready for review, if you want to review them in one swoop:

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

feanil
feanil approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Looks good, I was able to run the assets commands and see things working locally.

@kdmccormick
fix: simplify npm run watch-sass so that it is more reliable
966f2dc 
The implementation of `npm run watch-sass` was trying really hard
to recompile precisely only the Sass that needed to be recompiled, but in
order to do so, it had to spin up several `watchmedo` processes
per theme. These processes would trigger one another sometimes, leading
to infinite recompilation loops.

Rather than figure out all the dependency directions and messing with
`watchmedo`, I've opted to simplify the script to invoke a single
`watchmedo` process per theme. A single theme recompiles within
seconds, so I think this is a good compromise, one which makes the
script easier to reason about will help me move pass this legacy
assets work.
@kdmccormick
docs: npm scripts for static assets are no longer experimental :)
e33302b
@kdmccormick
build: compile/watch sass with new npm scripts
a228b81 
`paver` commands are deprecated for managing static assets. Starting in
Sumac, only `npm run` commands will be supported for managing static
assets.

To ease the transition, both `paver` and `npm run` commands will work in
Redwood. However, we want to stop using the *implementations* of the
`paver` asset commands right now, as they are blocking the Python 3.11
upgrade. This will also make the removal of `paver` commands more
straightforward come Sumac.

So, this commit turns these commands/functions:
* paver compile_sass (used by configuration)
* paver watch_sass (used by configuration and devstack)
* pavelib/assets.py:_compile_sass (used by Tutor)

into very thin wrappers around the new `npm run` commands. Each of these
paver routines now raise a loud deprecation warning, including a message
of the `npm run` command that the operator can switch to.
We expect no impact to site operators or end users.

openedx#31895
@kdmccormick kdmccormick merged commit 24db4df into openedx:master Apr 4, 2024
67 checks passed
@kdmccormick kdmccormick deleted the kdmccormick/libsass-python branch April 4, 2024 14:31
@kdmccormick kdmccormick mentioned this pull request Apr 4, 2024
Closed
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@kdmccormick kdmccormick mentioned this pull request Apr 18, 2024
Merged
@kdmccormick kdmccormick mentioned this pull request May 1, 2024
Open
@kdmccormick kdmccormick mentioned this pull request Jun 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feanil feanil feanil approved these changes

Assignees

@kdmccormick kdmccormick

Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kdmccormick @open-craft-grove @edx-pipeline-bot @feanil