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

Switch to our npm fork, to get newer node-gyp (node-gyp 9.x) #79

Merged
merged 4 commits into from
Jul 22, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Jul 12, 2023

Move to our fork of npm, which changes nothing other than depending on newer node-gyp. For ppm, this effectively bumps node-gyp from 5.1.1 to ^9.4.0.

This adds support for newer OSes/compiler toolchains/etc. (And drops support for Python 2.x, and Python 3.6 or older.)

See the node-gyp changelog for more info:
https://github.com/nodejs/node-gyp/blob/main/CHANGELOG.md

This node-gyp upgrade has been a wishlist item for the development team (and others?) for quite a while now.

DeeDeeG added 3 commits June 18, 2023 00:10
This entry only stabilizes the path of node-gyp -- it should be
deduped with the copy from npm. Its only purpose is to make the path
stable for one of our tests that needs node-gyp to be in that exact
location on disk.
@confused-Techie
Copy link
Member

@DeeDeeG mind me adding a commit to your branch here that switches our CI to use yarn as we discussed on Discord?

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jul 22, 2023

@confused-Techie I really wanna get this one working with npm also, but... Maybe I should do that as follow-up work when I get around to it. Perhaps it shouldn't hold this all up.

Like somewhere down the line, I can do a silly little PR to "fix package-lock.json that we totally don't use so npm 8+ users who we officially totally do not support don't get bothered by this being broken for them"

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jul 22, 2023

@DeeDeeG mind me adding a commit to your branch here that switches our CI to use yarn as we discussed on Discord?

Ok 👍

@confused-Techie
Copy link
Member

@DeeDeeG If that's something your comfortable with, that sounds awesome to me. Since once we get this in, I was hoping to myself get around to some of the changes I've been meaning to make in this repo for a while, such as decaffing and what not. So if we get everything merged I can do that real fast

@confused-Techie
Copy link
Member

confused-Techie commented Jul 22, 2023

@DeeDeeG So interestingly enough, we are getting consistent failures on Windows NodeJS 16 only, it's failing to build oniguruma.

Which is especially strange since NodeJS 16 works on macOS and Linux, and Windows NodeJS 14 and 18 both work fine

@confused-Techie
Copy link
Member

Alright, so over on #82 I've tested some solutions here.

It seems that if we migrate to second-mate and get an updated oniguruma Windows (along with all other platforms) are able to build everything just fine with these changes. Except, since await doesn't seem to be supported in CoffeeScript 1.x (which we are using) we can't actually migrate to second-mate as that requires proper async support.

Sure we could figure out how to get that code implemented, but it may be better to just let this one spec fail for now and merge, then once we have decaffed our code, we can migrate to second-mate with proper async support, and then we can get everything happy here.

So that is to say, I think we should just merge with our one failure here

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Gonna merge this one now, so we can decaf with these changes, and migrate to second mate to get all versions of windows working properly, like mentioned above

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

Successfully merging this pull request may close these issues.

2 participants