-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: remove dependency on Python sass
module
#34439
build: remove dependency on Python sass
module
#34439
Conversation
6f7e7a1
to
b73bbe4
Compare
ff01a70
to
f390c02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I was able to build the assets and load the site on Node 16.10
f390c02
to
73c22b6
Compare
Sandbox deployment successful 🚀 |
Instead of using libsass-python's `sass` module, which is incompatible with Python 3.11, we now directly use libsass-python's `_sass` module, which directly binds to the underlying C++ libsass library. For more details, see the docstring at scripts/compile_sass.py#L167
73c22b6
to
3c829fa
Compare
Sandbox deployment successful 🚀 |
Stuck in a meeting so can't verify that this is the source, but it seems likely, but we're seeing this error in our builds:
|
@dianakhuang Oof, seems likely, feel free to merge #34476 |
with open(target, 'w', encoding="utf-8") as target_file: | ||
target_file.write(output_text) | ||
|
||
click.secho(f" Done.", fg="green") | ||
# For Sass files without explicit RTL versions, generate | ||
# an RTL version of the CSS using the rtlcss library. | ||
for sass_path in glob.glob(str(source) + "/**/*.scss"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the issue is on this line, I should have changed source
to source_root
. This made it through my local testing because source
is a local variable in the new loop above this one, which was erroneously being used here, leading a silent failure.
I imagine that 2U must have a theme without any matched scss files, meaning that the local source
variable is never assigned, leading a to a loud failure instead in their pipelines instead of a quiet one.
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Instead of using libsass-python's `sass` module, which is incompatible with Python 3.11, we now directly use libsass-python's `_sass` module, which directly binds to the underlying C++ libsass library. This ensures that the Sass build will not a blocker for the edx-platform Python 3.11 upgrade, which needs to land in Redwood. For more details, see the docstring at scripts/compile_sass.py#L167
)" (openedx#34476) This reverts commit a08a10c. compile_sass.py had a bug which was caught by 2U's pipeline, but not by local testing.
Supporting information
Depends on:
Part of:
Description
Instead of using libsass-python's
sass
module, which is incompatiblewith Python 3.11, we now skip through to libsass-python's
_sass
module,which directly binds to the underlying C++ libsass library.
This ensures that the Sass build will not a blocker for the edx-platform
Python 3.11 upgrade, which needs to land in Redwood.
For more details, see the docstring at scripts/compile_sass.py#L167
Testing
Same as #34318 (comment)
Merge considerations
None