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

Upgrade dependencies, including http and hyper, where possible. #233

Merged
merged 3 commits into from
Dec 2, 2024

Conversation

ozabalaferrera
Copy link
Contributor

Description

This PR is the result of the discussion in #227. It upgrades various dependencies, including http and hyper, to the latest versions.

Status

This PR is ready for review. I worked on this sporadically, so I am sure there are better ways to accomplish some of what I did here.

Excluded

warp and actix have not been upgraded to http v1; I found it difficult to convert between types or implement traits. Therefore, those bindings here are mostly unchanged. I also created a separate feature flag for the old http v0.2.

Testing

I ran all the commands on the contributing guide. I also ran cargo test --all --features with all the features individually.

Breaks docs.

Signed-off-by: Omar Zabala-Ferrera <[email protected]>
@ozabalaferrera
Copy link
Contributor Author

@jcrossley3 or @Lazzaretti would you be able to review this?

@ozabalaferrera
Copy link
Contributor Author

I forgot about the example projects and other archs 🤦‍♂️. I'll work on fixing these.

@ozabalaferrera
Copy link
Contributor Author

I could use some help with the failing tests. I'm not sure what is causing this error:

It happens when running the test:

cargo test --target wasm32-wasi --features "http-0-2-binding hyper-0-14 hyper_wasi"

But the below works, so I imagine some testing dependency is the issue.

cargo build --target wasm32-wasi --features "http-0-2-binding hyper-0-14 hyper_wasi"

I'm not really sure if that is indicative of a real error, especially since building the example works:

cd example-projects/wasi-example/
cargo build --target wasm32-wasi

That being said, running causes an error, but I think it is due to my environment.

cd example-projects/wasi-example/
cargo run --target wasm32-wasi
target/wasm32-wasi/debug/wasi-example.wasm: 1: : not found
target/wasm32-wasi/debug/wasi-example.wasm: 1: : not found
target/wasm32-wasi/debug/wasi-example.wasm: 1: : not found
target/wasm32-wasi/debug/wasi-example.wasm: 1: : not found
target/wasm32-wasi/debug/wasi-example.wasm: 1: ~: not found
target/wasm32-wasi/debug/wasi-example.wasm: 1: : not found

I've not worked with wasm, so any help would be appreciated.

@sophokles73
Copy link

@ozabalaferrera the problem seems to be that the updated Mockito crate pulls in tokio (which the original version did not).

I am able to build and test for the wasm32-wasi target when I use Cargot.toml

...
mockito = "0.31.1"
...

I have then installed wasmedge (https://wasmedge.org/) and then run the tests successfully using

CARGO_TARGET_WASM32_WASI_RUNNER=wasmedge cargo test --target wasm32-wasi

@ozabalaferrera
Copy link
Contributor Author

Thanks, @sophokles73 ! I've been pretty busy, but took a minute to apply your fix. It looks like all tests are passing now.
@jcrossley3 or @Lazzaretti, sorry for the delay. Could you take another look at this and let me know if there is anything else I should address?

@sophokles73
Copy link

@jcrossley3 @Lazzaretti any chance you'll find the time to take a look?

Copy link
Contributor

@jcrossley3 jcrossley3 left a comment

Choose a reason for hiding this comment

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

This is a lot of work, thanks!

Comment on lines 25 to 27
unsafe {
std::env::set_var("RUST_LOG", "axum_example=debug,tower_http=debug")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the unsafe needed here? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vaguely remember it was needed because set_var is unsafe now. But I removed it and it seems fine. Not sure what happened before 🤷 ... I pushed a commit removing the two uses of unsafe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to sign off the commit, so I had to overwrite that commit history to fix it.
Let me know if you'd like me to squash or clean things up in any other way.

Copy link
Member

Choose a reason for hiding this comment

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

The pipeline still has a problem because of the signoff.

Copy link
Member

@Lazzaretti Lazzaretti left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, and sorry for the late review!

src/binding/poem/extractor.rs Outdated Show resolved Hide resolved
delegate-attr, base64, snafu, bitflags, hostname, and serde_yaml.

Signed-off-by: Omar Zabala-Ferrera <[email protected]>
@ozabalaferrera
Copy link
Contributor Author

ozabalaferrera commented Nov 7, 2024

@sophokles73 do you happen to know what this is about:
https://github.com/cloudevents/sdk-rust/actions/runs/11712115261/job/32666055306?pr=233#step:8:33

error: toolchain 'nightly-x86_64-unknown-linux-gnu' does not support target 'wasm32-wasi'; did you mean 'wasm32-wasip1'?

I didn't even know wasm32-wasip1 existed!
Everything seemed to work on my repo just a bit ago: https://github.com/ozabalaferrera/cloudevents-sdk-rust/actions/runs/11712114968

EDIT:

Ah, looks like we maybe hit the 2024-10-17 deadline here: https://blog.rust-lang.org/2024/04/09/updates-to-rusts-wasi-targets.html
Though I am not sure why it worked last night 🤷

I changed the target in the workflow. This should be ready for another test run, @Lazzaretti or @jcrossley3. Sorry for the back and forth!

@ozabalaferrera
Copy link
Contributor Author

@jcrossley3 , are you able to reach @Lazzaretti and see if we can get this PR reviewed and merged? I'd like to help with other issues after this.

@Lazzaretti
Copy link
Member

Sorry @ozabalaferrera, approved :)

@Lazzaretti Lazzaretti merged commit 09661dd into cloudevents:main Dec 2, 2024
10 checks passed
@ozabalaferrera
Copy link
Contributor Author

ozabalaferrera commented Dec 2, 2024

@Lazzaretti, no worries, thanks for the merge! What is the process for cutting and publishing a release to crates.io? Any estimate on when that might be?

EDIT:
Since my lasts tests, there is now an issue with the stable / wasm32-unknown-unknown test. Seems related to the use of #[wasm_bindgen] in reqwest-wasm-example . Maybe related: rustwasm/wasm-bindgen#4304 ?

@Lazzaretti
Copy link
Member

Hi @ozabalaferrera , if we can fix the issue in the main branch and have a green pipeline, I think we will soon be ready to release :)

@ozabalaferrera
Copy link
Contributor Author

@Lazzaretti , I opened a PR to address that issue here: #248
I hope that helps!

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.

4 participants