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

Python 3 support for node-js build stack #1311

Closed
benjaoming opened this issue Aug 17, 2022 · 14 comments
Closed

Python 3 support for node-js build stack #1311

benjaoming opened this issue Aug 17, 2022 · 14 comments
Assignees

Comments

@benjaoming
Copy link
Contributor

node-sass 4.3.0 is super-old and uses a stack for Python 2. Since then, a bunch of newer support has landed for both Node and Python versions.

I suggest updating to 5.0.0, since prior releases just seem to be about very small bug fixes and support of many newer Node versions: https://github.com/sass/node-sass/releases

PR incoming...

@benjaoming benjaoming self-assigned this Aug 17, 2022
@benjaoming
Copy link
Contributor Author

benjaoming commented Aug 17, 2022

Same issue with bourbon-neat.

@benjaoming
Copy link
Contributor Author

Oh game over, looks like we are stuck on Python 2 because of Wyrm.

bourbon-neat 1.9 is required, Wyrm is not compatible with Neat 2.0+.
The expected selector for the bourbon-neat dependency in package.json is "~1.9".

@benjaoming benjaoming changed the title Updating node-sass Python 3 support for node-js build stack Aug 17, 2022
@agjohnson
Copy link
Collaborator

Python 3 is definitely usable, but you cannot upgrade SASS, Wyrm, or Bourbon.

Upgrading these is a known issue, and the fix so far has be to drop the entire dependency chain and switch to Bootstrap. I would avoid trying to change these dependencies, as there was a fair amount of work that went into finding the correct configuration of dependencies that worked

@agjohnson
Copy link
Collaborator

There is a good amount of information on the dependencies here:
#771

@benjaoming
Copy link
Contributor Author

Are we talking about the same thing? The Node stack needs Python 2 in order to work AFAICT. There are for instance print "foobar" statements in the executed code of the node packages, which fail hard.

@agjohnson
Copy link
Collaborator

agjohnson commented Aug 17, 2022

Yeah, I'm not quite sure where you are running into errors, but perhaps having more information on how you're noticing errors would help.

The theme definitely builds on Python 3, I am using 3.8 locally only because I haven't tried upgrading to 3.11 yet, but I'm sure that also works.

Python 3 tests are all passing here (note, no dependency changes besides Jinja2<3.1 and Node==14.20):
#1316

image

@benjaoming
Copy link
Contributor Author

benjaoming commented Aug 19, 2022

There is something smelly going on.

While I have definitely had issues with a ghost Node 18 installation (which was supposed to be Node 14), I am now also having error-free builds with Python 3!

node-gyp 3.8 (which we are using) is NOT supposed to work with Python 3. That support landed in version 5: https://github.com/nodejs/node-gyp/blob/main/CHANGELOG.md#v500-2019-06-13

@benjaoming
Copy link
Contributor Author

Btw my errors happened during npm run build and contained errors about print statements in node-gyp, which are definitely indicating python 2 code :)

@agjohnson
Copy link
Collaborator

I'm mostly sure we worked past these issues, but just for historical note:

The answer here is that a system level node-gyp will take precedence over the locally installed version. So as long as you have a modern node-gyp installed, this package will build. Unfortunately, node-gyp here is a bit more tedious because of the dependency chain back to Wyrm and Bourbon.

@benjaoming
Copy link
Contributor Author

It seems perfectly fine to close this 👍 I think you could be right that there is something going on with system-level.

@benjaoming
Copy link
Contributor Author

Thanks for your patient feedback @agjohnson ❤️

@agjohnson
Copy link
Collaborator

Always! Closing does make sense I suppose, I don't think we have the appetite to fork Wyrm to upgrade Bourbon -> Neat -> node-gyp 😰

And the answer almost certainly is to drop all of them for modern dependencies or bootstrap.

@benjaoming
Copy link
Contributor Author

Yes definitely agree: I want to stress that the point here like originated from a misconception... but the intention was fix an issue so we can have an isolated and reliable Docker environment for all this old stuff... but I still hadn't dipped my toes into the full stack :)

I think the Docker environment needs a few tweaks, but they're easier to handle at an appropriate occasion, not as some pro-active venture :)

@agjohnson
Copy link
Collaborator

agjohnson commented Mar 13, 2023

I want to stress that the point here like originated from a misconception

Hah well I'd say it's more than a misconception too, the original issue originated from a very annoying bug. We definitely shouldn't need system level dependencies to build this here 🙃

I believe I tried it here, and I'm not sure if there is an easy way to make Wyrm and friends happy with it, but dropping node-sass entirely, for the Dart sass implementation does work around the node-gyp issue entirely:

readthedocs/ethical-ad-client#149

So, there are maybe 2 options there that don't involve us fiddling with node-gyp any further. I'm hopefully we can avoid applying our efforts to patching up node-gyp installation issues.

At least we have a way to avoid being blocked here for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants