-
Notifications
You must be signed in to change notification settings - Fork 195
Conversation
review this when possible @baparham, dunno why but cannot ask you for review directly |
@phated thanks for the contribution! You may have better luck building the binaries in your fork if you also use the patch from #265 to enable building on linux after node LTS switched from 16x to 18x. There was also a weird windows error that I don't recognize but it seems unrelated to these changes...I'll try to reproduce in my fork. What OS-arch are you testing within your project, so we know what's been 'manually tested' so to speak? I'd like to see successful builds of each platform/node version combo before suggesting this should go in. I'll kick off some builds on my own to check this patch out as well. |
I have a full set of builds (all except non-windows arm platforms) kicked off here with this patch: https://github.com/baparham/pkg-fetch/actions/runs/4103184419 to see how they all build, or if the errors I saw in Blaine's fork actions are reproducible. |
My manual test was on Mac Arm64. Thanks for the quick replies! |
Regarding the Windows failure, the dtrace and etw support was dropped in nodejs/node@aa3a572 so the I just pushed a commit that should fix it. |
@baparham could you spawn new tests? |
Perfect thanks LMK |
This run looks all green to me 🎉 https://github.com/baparham/pkg-fetch/actions/runs/4112226627 |
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.
LGTM
@leerob kindly merge this 🙏🏼 |
Anything else needed to land this? |
I prepared a patch for node 19.6 and then linked/tested it within our project. These seem to work based on our usage.