- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
feat: support node 22 #14
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
Conversation
I'm going to delay fixing the ARM bug
we want to run the tests on the cross-compiles
one thing at a time
This reverts commit 57d7b39.
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.
good idea
| @@ -1,2 +1 @@ | |||
| node_modules | |||
| test | |||
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.
Why exclude node_modules but allow test? Are we running tests in the Docker container?
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.
- node_modulesis excluded because the docker container may have a different arch so native builds will not work
- testwas excluded to speed up cross-compilation but:- it's tiny so kind of useless
- I want to run the tests on cross-compiles before releasing. I actually had it in this PR but it was too much work so I pushed it off to the future
 
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.
not that it would be accepted in a timely manner but should this get upstreamed?
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.
I left a comment on their issue tracker, I'm not gonna bother with a PR because the maintainer doesn't seem to accept them
(If you're the maintainer reading this though, hmu I'd be happy to PR a fix with you)
🧰 Changes
get the tests passing locallyadd tests to the publish jobfix the issue uncovered by running the linux-arm testsThe patch in this commit resolves the build issue noted in nodegit#2023 by applying a patch to zlib, which is a little gross but gets us building at least