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

v0.12.11 breaks my wasm-unknown-unknown build #2509

Closed
jonatino opened this issue Dec 29, 2024 · 8 comments · Fixed by #2510
Closed

v0.12.11 breaks my wasm-unknown-unknown build #2509

jonatino opened this issue Dec 29, 2024 · 8 comments · Fixed by #2510

Comments

@jonatino
Copy link

It adds a dependency on tower which adds a dependency on tokio and mio which does not support wasm. Lots of build errors for UdpPacket and TcpPacket bindings.

I think it was this commit specifically. v0.12.9 builds perfectly.
2a7c1b6

@seanmonstar
Copy link
Owner

It might be the timeout feature of tower. Interesting, we have a CO job building wasm, it didn't seem to complain.

cc @jlizen

@jlizen
Copy link
Contributor

jlizen commented Dec 29, 2024

Yeah, interesting that the CI job did not complain. I can find a fix for this tomorrow. @jonatino can you share some error outputs to help confirm our reproduction matches your issue?

I agree that it likely looks the transitive dependency on tokio/time that the timeout layer adds. I see that we have that explicitly scoped out of WASM dependencies (https://github.com/seanmonstar/reqwest/blob/master/Cargo.toml#L121). Probably we need to do similar here.

@jlizen
Copy link
Contributor

jlizen commented Dec 29, 2024

I think I see the CI issue. We have:

steps:
  - name: Checkout
    uses: actions/checkout@v4

  - name: Install rust
    uses: dtolnay/rust-toolchain@stable
    with:
      targets: wasm32-unknown-unknown

  - name: Check
    run: cargo check --target wasm32-unknown-unknown

  - name: Install wasm-pack
    run: curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh

  - name: Wasm-pack test firefox
    run: wasm-pack test --headless --firefox

  - name: Wasm-pack test chrome
    run: wasm-pack test --headless --chrome

IE, cargo check and use wasm-pack. No plain cargo build. Presumably dependency resolution is a bit different with wasm-pack.

I can add a plain cargo build to ci along with the fix.

@jlizen
Copy link
Contributor

jlizen commented Dec 29, 2024

I have a change locally that prunes tower out of the wasm target. Was pretty simple, I can cut a PR.

However, I can't actually produce the old failure using:

cargo build --target wasm32-unknown-unknown

So I won't be able to directly confirm the fix without more information from @jonatino . I'll also hold off on including changes to github CI until I can repro.

@jonatino
Copy link
Author

jonatino commented Dec 29, 2024

@jlizen was able to get a minimal example to reproduce it.

Run cargo build --target "wasm32-unknown-unknown" inside of .\test reqwest breaking\common dir. I also got these errors using the wasm-pack tool so I'm very surprised your CI build did not fail.

Fails to build with reqwest 0.12.11. Change the version inside of the common project's Cargo.toml, build works with v0.12.9.

test reqwest breaking.zip

Lmk if there's anything else you need!

@jlizen
Copy link
Contributor

jlizen commented Dec 29, 2024

@jonatino thanks! I was able to reach the build failure with your reproduction, and I can confirm that my fix in #2510 addresses it.

I'll chase down the CI gap sometime this coming week @seanmonstar

@seanmonstar
Copy link
Owner

Woops, didn't read the last comment that wasm-pack also fails for you. I tried adjust CI in #2513 to use cargo build but it seems happy too.

@jlizen
Copy link
Contributor

jlizen commented Dec 30, 2024

I cut #2514 to continue the investigation into CI gap

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 a pull request may close this issue.

3 participants