Skip to content
This repository was archived by the owner on Jan 3, 2024. It is now read-only.

patches: add patch for node 19.6#274

Merged
robertsLando merged 3 commits into
vercel:mainfrom
phated:phated/node19
Feb 14, 2023
Merged

patches: add patch for node 19.6#274
robertsLando merged 3 commits into
vercel:mainfrom
phated:phated/node19

Conversation

@phated

@phated phated commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

I prepared a patch for node 19.6 and then linked/tested it within our project. These seem to work based on our usage.

@robertsLando

robertsLando commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

review this when possible @baparham, dunno why but cannot ask you for review directly

@baparham

baparham commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

@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.

@baparham

baparham commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

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.

@phated

phated commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

My manual test was on Mac Arm64. Thanks for the quick replies!

@phated

phated commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

Regarding the Windows failure, the dtrace and etw support was dropped in nodejs/node@aa3a572 so the noetw arg is no longer needed in node 19+

I just pushed a commit that should fix it.

@robertsLando

Copy link
Copy Markdown
Contributor

@baparham could you spawn new tests?

@baparham

baparham commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

running here: https://github.com/baparham/pkg-fetch/actions/runs/4112226627

@robertsLando

Copy link
Copy Markdown
Contributor

Perfect thanks LMK

@phated

phated commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

This run looks all green to me 🎉 https://github.com/baparham/pkg-fetch/actions/runs/4112226627

@robertsLando robertsLando left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@robertsLando

Copy link
Copy Markdown
Contributor

@leerob kindly merge this 🙏🏼

@phated

phated commented Feb 14, 2023

Copy link
Copy Markdown
Contributor Author

Anything else needed to land this?

@robertsLando robertsLando changed the title patches: Add patch for node 19.6 patches: add patch for node 19.6 Feb 14, 2023
@robertsLando
robertsLando merged commit 7b23050 into vercel:main Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants