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

feat: add support for wasm #1040

Merged
merged 5 commits into from Jun 10, 2023
Merged

feat: add support for wasm #1040

merged 5 commits into from Jun 10, 2023

Conversation

zebp
Copy link
Contributor

@zebp zebp commented Jun 3, 2023

Adds support for compiling to WASM environments that provide JS via wasm-bindgen. Because there's no standardized socket API the caller must provide a connection that implements AsyncRead/AsyncWrite to connect_raw.

The motivation for this is an open PR to the Rust framework for Cloudflare Workers where Cloudflare recently announced support for TCP sockets.

tokio-postgres/Cargo.toml Outdated Show resolved Hide resolved
@sfackler
Copy link
Owner

sfackler commented Jun 3, 2023

We'll want to add a wasm32 build to CI - even if we aren't going to run tests we'll want to know the build isn't broken.

Adds support for compiling to WASM environments that provide JS via
wasm-bindgen. Because there's no standardized socket API the caller must
provide a connection that implements AsyncRead/AsyncWrite to
connect_raw.
Adds a CI job for ensuring the tokio-postgres crate builds on
the wasm32-unknown-unknown target without the default features.
@sfackler
Copy link
Owner

sfackler commented Jun 3, 2023

This looks good to me - anything you're still planning to add or should we merge?

@zebp
Copy link
Contributor Author

zebp commented Jun 3, 2023

I still want to see if the addition of resolver = 2 to the workspace is going to cause any issues, it's required for the wasm target because of the [target.'cfg(not(target_arch = "wasm32"))'.dependencies] in the tokio-postgres crate. The property is used from the top level crate that you're compiling but I'm not 100% sure that this is a non-breaking change for people still using the old resolver.

@sfackler
Copy link
Owner

sfackler commented Jun 4, 2023

The resolver selection is local to each workspace. Rather than unconditionally enabling the js feature on getrandom for wasm32, I think it would be better to add a js feature to postgres-protocol and tokio-postgres. That way we wouldn't be unconditionally locking all wasm users into an assumption of a JS environment if they were instead running in e.g. WASI.

That should remove the need for resolver 2 as well.

@zebp
Copy link
Contributor Author

zebp commented Jun 4, 2023

The resolver selection is local to each workspace. Rather than unconditionally enabling the js feature on getrandom for wasm32, I think it would be better to add a js feature to postgres-protocol and tokio-postgres. That way we wouldn't be unconditionally locking all wasm users into an assumption of a JS environment if they were instead running in e.g. WASI.

That should remove the need for resolver 2 as well.

Agreed about the js feature but I don't think that's going to be possible with the old feature resolver unless we add dep:socket2 as a default feature because [target.'cfg(not(target_arch = "wasm32"))'.dependencies] isn't respected in the old feature resolver causing socket2 to get included for all targets, including wasm. I have a branch here with those changes if we want to proceed down this path.

The current solution of excluding socket2 on non-wasm targets still works as expected for projects using resolver 1 because resolver 1 includes all the dependencies regardless of target. If adding an extra default feature is off the table this could be a workaround where resolver 2 is required for wasm support but not necessary on other platforms.

@sfackler
Copy link
Owner

sfackler commented Jun 4, 2023

The socket2 dependency can be unconditionally disabled on wasm - the only thing gated by the feature would be the getrandom feature (which is a no-op on not-wasm so it shouldn't matter if people are using resolver 2 or not).

@zebp
Copy link
Contributor Author

zebp commented Jun 5, 2023

Yeah I agree with you on putting getrandom behind a feature, but which approach do you prefer for making socket2 an optional dependency depending on the target.

The two approaches outlined would be making it a default feature, which would be a breaking change for people using tokio-postgres without default features. Or adding resolver 2 back to the workspace and requiring people using WASM to use the new resolver in their workspace as well, introducing no breaking change for non-wasm users.

@sfackler
Copy link
Owner

sfackler commented Jun 5, 2023

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
socket2 = { version = "0.5", features = ["all"] }

should be sufficient without any resolver customization I think?

@zebp
Copy link
Contributor Author

zebp commented Jun 7, 2023

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
socket2 = { version = "0.5", features = ["all"] }

should be sufficient without any resolver customization I think?

On resolver 1 that'll still include socket2 for wasm targets because cfg(not( doesn't seem to be respected on resolver 1. We could keep that in our Cargo.toml but the downstream project would need resolver 2 and we would need resolver 2 in this project (although that's only so we can run CI on wasm and won't affect anyone else). If that's an acceptable tradeoff then I can move this PR out of draft.

@sfackler
Copy link
Owner

sfackler commented Jun 8, 2023

I don't think that's correct - resolver 1 vs 2 affects how features of enabled dependencies are unified across cfg-specific dependencies. Platform dependent crates have always been supported. See for example native-tls which has cfg'd dependencies on schannel and security framework crates that wouldn't build off of their native platform.

@zebp
Copy link
Contributor Author

zebp commented Jun 8, 2023

Oh wow, yeah I totally had some wires cross in my head 😅. With resolver 1 the issue is that tokio gets compiled with features (including socket2, which is where I made the mistake) that aren't compatible with WASM. By running cargo check --target wasm32-unknown-unknown --no-default-features --features js -v we eventually end up with cargo running rustc with the tokio like this:

Running `rustc --crate-name build_script_build --edition=2021 /Users/zeb/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.28.2/build.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 --cfg 'feature="bytes"' --cfg 'feature="default"' --cfg 'feature="io-util"' --cfg 'feature="libc"' --cfg 'feature="macros"' --cfg 'feature="mio"' --cfg 'feature="net"' --cfg 'feature="num_cpus"' --cfg 'feature="rt"' --cfg 'feature="rt-multi-thread"' --cfg 'feature="socket2"' --cfg 'feature="sync"' --cfg 'feature="time"' --cfg 'feature="tokio-macros"' --cfg 'feature="windows-sys"' -C metadata=712f08d5e5bd4c22 -C extra-filename=-712f08d5e5bd4c22 --out-dir /Users/zeb/dev/rust/rust-postgres/target/debug/build/tokio-712f08d5e5bd4c22 -L dependency=/Users/zeb/dev/rust/rust-postgres/target/debug/deps --extern autocfg=/Users/zeb/dev/rust/rust-postgres/target/debug/deps/libautocfg-6f9803c3213f701a.rlib --cap-lints allow`

The features are coming from the dev dependencies which is an issue with resolver 1, so to work around that without using resolver 2 just for the CI check I hackily added a sed to ignore the dev dependencies for the workflow.

@zebp zebp marked this pull request as ready for review June 8, 2023 01:25
@sfackler
Copy link
Owner

sfackler commented Jun 8, 2023

build's red

@sfackler sfackler merged commit 852869d into sfackler:master Jun 10, 2023
4 checks passed
@zebp zebp deleted the feat/wasm-support branch June 10, 2023 14:11
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