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.

Sign up for GitHub

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

Jump to bottom

[rust] upstreaming rust/cxx integration #2987

Merged
merged 2 commits into from
Oct 31, 2024
Merged

[rust] upstreaming rust/cxx integration #2987

merged 2 commits into from
Oct 31, 2024

Conversation

mikea
Copy link
Collaborator

@mikea mikea commented Oct 23, 2024 •
edited
Loading

TODO after this PR would be to reuse these parts of cxx support downstream. (will require small tweaks to init tokio),
but want to land this now to unblock.

@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch from 42df254 to e94d859 Compare October 23, 2024 17:02
@mikea
Copy link
Collaborator Author

mikea commented Oct 23, 2024

blocked by capnproto/capnproto#2169

@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch 2 times, most recently from b4f375e to bf629a2 Compare October 23, 2024 19:45
@mikea mikea marked this pull request as ready for review October 23, 2024 19:47
@mikea mikea requested review from a team as code owners October 23, 2024 19:47
danlapid
danlapid requested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@danlapid danlapid left a comment

Choose a reason for hiding this comment

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

This seems to break windows build 😢

@mikea
Copy link
Collaborator Author

mikea commented Oct 23, 2024

This seems to break windows build 😢

Yeah, windows rust build is a bit tricky for some reason. Will require some fiddling.

@fhanau
Copy link
Collaborator

fhanau commented Oct 23, 2024

This seems to break windows build 😢

Yeah, windows rust build is a bit tricky for some reason. Will require some fiddling.

Two things stick out:
This happens when linking external/capnp-cpp/src/kj/kj_c9e87b54.dll – Bazel does not enable shared linkage by default on Windows and we have never enabled it, no idea why it is happening here.
The build is complaining about undefined symbol: __declspec(dllimport) inet_pton being missing. When we link Rust binaries within workerd, we have to add ntdll.lib on Windows to avoid missing symbol warnings. It appears that this might be needed here too (if we link statically, the __declspec(dllimport) part should not appear here).

Overall, rules_rust does not support Windows well unfortunately so I'd expect more such issues along the way (well, perhaps Windows is just not very suitable for software development in general).

@danlapid
Copy link
Collaborator

inet_pton

inet_pton is from Ws2_32.lib not ntdll.lib

@mikea
Copy link
Collaborator Author

mikea commented Oct 23, 2024

This happens when linking external/capnp-cpp/src/kj/kj_c9e87b54.dll – Bazel does not enable shared linkage by default on Windows and we have never enabled it, no idea why it is happening here.

maybe this is what rust_crate does on windows?

@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch 3 times, most recently from 6916176 to c96d8bd Compare October 24, 2024 15:50
@mikea
Copy link
Collaborator Author

mikea commented Oct 24, 2024

I'll be CI-debugging windows build by commenting things out, sorry for the noise.

@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch 3 times, most recently from b4dc134 to 53b2564 Compare October 24, 2024 17:59
@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch 2 times, most recently from 220bb7c to 040be41 Compare October 24, 2024 18:16
@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch 2 times, most recently from 3620e56 to b47ac61 Compare October 31, 2024 20:39
@mikea mikea force-pushed the maizatskyi/2024-10-23-rust branch from b47ac61 to 7001aaf Compare October 31, 2024 21:24
@mikea
Copy link
Collaborator Author

mikea commented Oct 31, 2024

Windows build issue is fixed by:

  • upgrading to rust 1.82.0 (linker issue)
  • switching to mainline cxx crate. I do not think it is possible to build workerd-cxx on windows using bazel, so I will try to publish to crates.io later. For now cxx will do to unlock everyone

PTAL @danlapid

@mikea mikea requested a review from danlapid October 31, 2024 21:27
@fhanau
Copy link
Collaborator

fhanau commented Oct 31, 2024

Windows build issue is fixed by:

  • upgrading to rust 1.82.0 (linker issue)
  • switching to mainline cxx crate. I do not think it is possible to build workerd-cxx on windows using bazel, so I will try to publish to crates.io later. For now cxx will do to unlock everyone

PTAL @danlapid

Glad you figured it out!

@danlapid
Copy link
Collaborator

👑

@mikea mikea merged commit e1c8c53 into main Oct 31, 2024
13 checks passed
@mikea mikea deleted the maizatskyi/2024-10-23-rust branch October 31, 2024 21:52
@danlapid
Copy link
Collaborator

@hoodmane we can integrate ruff now! 🤩

@hoodmane
Copy link
Contributor

Thanks @mikea !

@mikea
Copy link
Collaborator Author

mikea commented Oct 31, 2024

Np. I'll also follow up with wd_rust_binary tomorrow (me and @anonrig need it too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhanau fhanau fhanau approved these changes

@danlapid danlapid danlapid approved these changes

@jasnell jasnell Awaiting requested review from jasnell jasnell is a code owner automatically assigned from cloudflare/workers-runtime-1

@a-robinson a-robinson Awaiting requested review from a-robinson a-robinson is a code owner automatically assigned from cloudflare/workers-runtime-1

@hoodmane hoodmane Awaiting requested review from hoodmane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mikea @fhanau @danlapid @hoodmane