Closed zebp closed 1 year ago
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.
This looks good to me - anything you're still planning to add or should we merge?
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.
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.
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 ajs
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.
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).
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.
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
socket2 = { version = "0.5", features = ["all"] }
should be sufficient without any resolver customization I think?
[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.
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.
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.
build's red
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
toconnect_raw
.The motivation for this is an open PR to the Rust framework for Cloudflare Workers where Cloudflare recently announced support for TCP sockets.