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

Bump napi-rs #9699

Draft
wants to merge 7 commits into
base: v2
Choose a base branch
from
Draft

Bump napi-rs #9699

wants to merge 7 commits into from

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented May 6, 2024

Bump napi-rs to include the fix for napi-rs/napi-rs#2085 and revert Rust version change (#9696, #9697) again

  • It's still not fixed completely in napi-rs
  • Waiting for napi-rs release

@mischnic mischnic marked this pull request as draft May 6, 2024 11:29
@mischnic mischnic force-pushed the upgrade-napi-rs branch 2 times, most recently from 5cdd7ae to e0fc759 Compare May 7, 2024 10:54
@@ -21,7 +21,7 @@ jobs:
cache: yarn
- uses: dtolnay/rust-toolchain@master
with:
toolchain: 1.77
toolchain: stable
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should stay on a pinned release of Rust, and update it manually? Or just stay on stable and pin only when it breaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or just stay on stable and pin only when it breaks?

Yes, that's what I would have suggested (= what we've done so far). Requires less maintenance from our side as well

@marcins
Copy link
Contributor

marcins commented May 14, 2024

Do you have any clues as to why integration tests are still failing? I cloned this branch locally and built it and it seems to work okay for a quick smoke test (building a smaller part of our application as a production build locally). I've double checked and it's build with Rust 1.78 and running with Node v18.20.1 (on an M1 Mac)

@mischnic
Copy link
Member Author

My only guess is that it's the same stack overflow as #9574

But we'll see what happens when rebasing this after merging the swc bump.

@mischnic
Copy link
Member Author

Now it's again crashing only on Windows, so probably the same stack problem as with the swc PR

@marcins
Copy link
Contributor

marcins commented May 16, 2024

It looks like everything is working again now - likely as you suspected, the SWC stack changes might've fixed this.

Are there any blockers to taking this out of Draft and getting it shipped? For context, keen on being able to get a Rust 1.78 based build as the RwLock implementation has changed to a Rust based one from the existing pthread one - keen to see if this fixes the issues we see internally with lock based crashes in the resolver coming from the macOS implementation.

EDIT: I pulled in latest v2 changes and still getting Windows integration test failures with error Command failed with exit code 3221225725.

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.

None yet

2 participants