Rewrite in N-API and update libgit2#6
Conversation
Wasn’t a pressing need, but I wanted some more practice at converting modules that use `nan`.
…and fix issue revealed by failing tests.
2509503 to
1723dfa
Compare
|
The size of the diff should be explained. We used to explicitly vendor Either way, this PR would touch a lot of files; if the submodule approach works, then future |
|
OK, CI currently only tests in Linux. Good to know Linux passes, but I should add Windows to the matrix or else I can't be certain that this is a safe change to make! I'll revisit this tomorrow. |
|
PR #5 has more context about why
|
|
Hey! Got a passing test suite on all platforms. I think some of those Windows bugs had been around for a while. Taking this out of draft! |
|
Ping! |
There was a problem hiding this comment.
I don't deeply understand the changes, but specs are passing on all three OSes, so...
None of the individual commits look particularly crazy, either. From a quick look at each...
Perhaps we can add ubuntu-24.04-arm to the ci.yml matrix? To try compiling on ARM Linux? Maybe even macos-15-intel or macos-26-intel if feeling adventurous... But uh... optional. Adding Windows and macOS was already a great thing to do. I do think it'd be a good idea, though.
But anyway, probably as close as I'm going to get to "meaningfully reviewing" this ever. I reckon we dogfood it and make sure it behaves IRL, beyond what the specs show (which is looking good, much appreciated there).
Somewhat of a rubber-stamp Approval, admittedly.
But thanks for the efforts, and hopefully this helps us compile on newer macOS versions without breaking other (relevant) macOS/other OS versions? (Compiles on my macOS Catalina x64 machine, by the way! No node-gyp errors, just a few warnings! 👍)
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
git-utilsis failing to build altogether on my work laptop (which runs macOS 26, despite my best efforts), so I figured this was a good time to bumplibgit2.This also adopts the needed rewrite of
git-utilsin N-API; this should probably fix pulsar#1468.The tests pass for me locally, but I'll open this as a draft to start just to see what CI thinks of it on the other platforms.