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

Move away from node-sass #1486

Open
jelly opened this issue Jun 25, 2023 · 8 comments
Open

Move away from node-sass #1486

jelly opened this issue Jun 25, 2023 · 8 comments
Labels
Bug A bug Needed: replication Bug replication is required

Comments

@jelly
Copy link

jelly commented Jun 25, 2023

Problem

This project seems to hard require nodejs 14 as node-sass does not work with a higher nodejs version (does not build with Python 3.10) for some interesting reason.

node-sass is deprecated andprojects should use dart sass. Is a PR welcome to update to dart sass?

Error Logs/Results

npm notice
npm ERR! code 1
npm ERR! path /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/node-sass
npm ERR! command failed
npm ERR! command sh -c -- node scripts/build.js
npm ERR! Building: /usr/bin/node /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/node-gyp/bin/node-gyp.js rebuild --verbose --libsass_ext= --libsass_cflags= --libsass_ldflags= --libsass_library=
npm ERR! gyp info it worked if it ends with ok
npm ERR! gyp verb cli [
npm ERR! gyp verb cli   '/usr/bin/node',
npm ERR! gyp verb cli   '/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/node-gyp/bin/node-gyp.js',
npm ERR! gyp verb cli   'rebuild',
npm ERR! gyp verb cli   '--verbose',
npm ERR! gyp verb cli   '--libsass_ext=',
npm ERR! gyp verb cli   '--libsass_cflags=',
npm ERR! gyp verb cli   '--libsass_ldflags=',
npm ERR! gyp verb cli   '--libsass_library='
npm ERR! gyp verb cli ]
npm ERR! gyp info using node-gyp@3.8.0
npm ERR! gyp info using node@18.15.0 | linux | x64
npm ERR! gyp verb command rebuild []
npm ERR! gyp verb command clean []
npm ERR! gyp verb clean removing "build" directory
npm ERR! gyp verb command configure []
npm ERR! gyp verb check python checking for Python executable "python2" in the PATH
npm ERR! gyp verb `which` failed Error: not found: python2
npm ERR! gyp verb `which` failed     at getNotFoundError (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:13:12)
npm ERR! gyp verb `which` failed     at F (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:68:19)
npm ERR! gyp verb `which` failed     at E (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:80:29)
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:89:16
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/isexe/index.js:42:5
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/isexe/mode.js:8:5
npm ERR! gyp verb `which` failed     at FSReqCallback.oncomplete (node:fs:208:21)
npm ERR! gyp verb `which` failed  python2 Error: not found: python2
npm ERR! gyp verb `which` failed     at getNotFoundError (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:13:12)
npm ERR! gyp verb `which` failed     at F (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:68:19)
npm ERR! gyp verb `which` failed     at E (/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:80:29)
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/which/which.js:89:16
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/isexe/index.js:42:5
npm ERR! gyp verb `which` failed     at /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/isexe/mode.js:8:5
npm ERR! gyp verb `which` failed     at FSReqCallback.oncomplete (node:fs:208:21) {
npm ERR! gyp verb `which` failed   code: 'ENOENT'
npm ERR! gyp verb `which` failed }
npm ERR! gyp verb check python checking for Python executable "python" in the PATH
npm ERR! gyp verb `which` succeeded python /usr/bin/python
npm ERR! gyp ERR! configure error
npm ERR! gyp ERR! stack Error: Command failed: /usr/bin/python -c import sys; print "%s.%s.%s" % sys.version_info[:3];
npm ERR! gyp ERR! stack   File "<string>", line 1
npm ERR! gyp ERR! stack     import sys; print "%s.%s.%s" % sys.version_info[:3];
npm ERR! gyp ERR! stack                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
npm ERR! gyp ERR! stack SyntaxError: Missing parentheses in call to 'print'. Did you mean print(...)?
npm ERR! gyp ERR! stack
npm ERR! gyp ERR! stack     at ChildProcess.exithandler (node:child_process:419:12)
npm ERR! gyp ERR! stack     at ChildProcess.emit (node:events:513:28)
npm ERR! gyp ERR! stack     at maybeClose (node:internal/child_process:1091:16)
npm ERR! gyp ERR! stack     at ChildProcess._handle.onexit (node:internal/child_process:302:5)
npm ERR! gyp ERR! System Linux 6.3.9-arch1-1
npm ERR! gyp ERR! command "/usr/bin/node" "/build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/node-gyp/bin/node-gyp.js" "rebuild" "--verbose" "--libsass_ext=" "--libsass_cflags=" "--libsass_ldflags=" "--libsass_library="
npm ERR! gyp ERR! cwd /build/python-sphinx_rtd_theme/src/sphinx_rtd_theme-1.2.2/node_modules/node-sass
npm ERR! gyp ERR! node -v v18.15.0
npm ERR! gyp ERR! node-gyp -v v3.8.0
npm ERR! gyp ERR! not ok
npm ERR! Build failed with error code: 1
@jelly jelly added Bug A bug Needed: replication Bug replication is required labels Jun 25, 2023
@benjaoming
Copy link
Contributor

Hi @jelly

Everything in the build setup is pretty outdated. I've isolated the environment in Docker to perhaps make it easier to build things:

You can see the commands here: https://github.com/readthedocs/sphinx_rtd_theme/blob/master/Makefile

docker-build-all will build new CSS files if you have modified the SASS code.

(there's a bug that means you sometimes have to run make docker-images an extra time, haven't had time to look at it)

@jelly
Copy link
Author

jelly commented Jun 26, 2023

That does not help us as in Arch Linux we build the package currently with nodejs 14, which we intend to drop soon. So I am wondering if you are open in a PR which moves away from node-sass to dart-sass?

@benjaoming
Copy link
Contributor

benjaoming commented Jun 26, 2023

Arch Linux is something that @humitos and @agjohnson like a lot ❤️ I think they can make a call here about how much can be invested in changing the current setup, if any 😐

If node-sass and dart-sass compile the same CSS code, then the change would be fairly simple. I'd worry that the Node upgrade isn't possible because of the super-deprecated state of the stack, though ⚠️

@jelly
Copy link
Author

jelly commented Jun 26, 2023

I'll see if I can make a patch and then we can discuss what the possibilities are 👍

@jelly
Copy link
Author

jelly commented Jun 26, 2023

So dropping node-sass or replacing it, isn't easy due to the requirement of it by bourboun-neat.

sphinx_rtd_theme@1.2.2 /home/jelle/projects/sphinx_rtd_theme
├─┬ bourbon-neat@1.9.1
│ └── node-sass@4.14.1 deduped
└── node-sass@4.14.1

Now the question is, can we update this as newer versions dropped the node-sass requirement, it seems however that we can't as wyrm depends on bourbon-neat==1.9 but this project is rather dead. So my conclusion is, this project should switch to a different sass framework, which seems to be quite some work:

src/sass/_theme_rst.sass://    1. Lots of this @extends from wyrm_core/_type.sass (http://www.github.com/snide/wyrm/.
src/sass/_theme_rst.sass://        * That said, know that I'm very unlikely to accept PRs from wyrm just to change style here.
src/sass/_theme_rst.sass://    3. Try to use variables from wyrm_core/wy_variables.sass. Notable are...
src/sass/_theme_variables.sass:// that are set in wyrm_core/wy_variables.sass. You'll find wyrm in bower_components if you're looking
src/sass/badge_only.sass:@import wyrm_core/wy_variables
src/sass/badge_only.sass:@import wyrm_core/mixin
src/sass/badge_only.sass:@import wyrm_core/grid_settings
src/sass/theme.sass:@import wyrm_core/wy_variables
src/sass/theme.sass:@import wyrm_core/grid_settings
src/sass/theme.sass:@import wyrm_core/neat_extra
src/sass/theme.sass:@import wyrm_core/reset
src/sass/theme.sass:@import wyrm_core/mixin
src/sass/theme.sass:// Font Awesome with wyrm extras
src/sass/theme.sass:@import wyrm_core/font_icon_defaults
src/sass/theme.sass:@import wyrm_core/alert
src/sass/theme.sass:@import wyrm_core/button
src/sass/theme.sass:@import wyrm_core/dropdown
src/sass/theme.sass:@import wyrm_core/form
src/sass/theme.sass:@import wyrm_core/generic
src/sass/theme.sass:@import wyrm_core/table
src/sass/theme.sass:@import wyrm_core/type

Maybe tailwind or bulma or bootstrap? would be an interesting sass framework to switch too

@agjohnson
Copy link
Collaborator

I believe there are a few issues going over the blocks to upgrading if you're interested, but you've found the major issues we've touched on. The dependency on a very specific version of Wyrm and Bourbon/Neat is the largest block to ugrading to Dart. To solve this, a fork of Wyrm is needed, but I don't think we have the appetite to maintain that in addition. Wyrm was written by the same original author as this theme, and wasn't used much outside this theme.

There is already a branch/PR porting to Bootstrap, but we have to take this upgrade incredibly slow and deliberately because of how much of a backwards incompatible change it will be. We incur all of the support around the theme and projects breaking, which in the case of a big backwards incompatible visual change like this, would mostly be silent breakage with visual elements and style overrides.

Our current release plan is to continue deprecating our support for older versions of Sphinx and docutils until we have a reasonable dependency and testing matrix. After that, we'll consider either a backwards incompatible major release using Bootstrap or some pattern to ease into the migration.

Right now the technical work behind a Bootstrap port is not holding this back much. The blocker is the amount of work in getting users to rewrite their own projects to match the backwards incompatible change and the amount of support we'll incur with a big backwards incompatible change.

@jelly
Copy link
Author

jelly commented Jun 29, 2023

For the Arch package we now just use the prebuild assets so it's less of an issue.

@agjohnson
Copy link
Collaborator

That would be my suggestion for the time being as well. It is possible to build the assets, but the dependency chain is extremely fragile right now and held together with duct tape and magic.

For some background, here is a similar issue and some of the background around the versions used here:

In particular, if installed at the system level, node-gyp will be used in place of the dependency from node-sass, regardless of Python version. Very much not a good fix, but I agree that overall switching away from node-sass, node-gyp, Bourbon, Neat, and Wyrm is the right call.

@humitos humitos mentioned this issue Jan 23, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug Needed: replication Bug replication is required
Projects
None yet
Development

No branches or pull requests

3 participants