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

chore: bump hyper to 1.0.1 #527

Merged
merged 11 commits into from
Mar 11, 2024

Conversation

mraszyk
Copy link
Contributor

@mraszyk mraszyk commented Mar 7, 2024

This PR bumps hyper to 1.0.1.

@mraszyk mraszyk requested a review from a team as a code owner March 7, 2024 22:43
@ghost ghost requested a review from adamspofford-dfinity March 7, 2024 23:11
@adamspofford-dfinity
Copy link
Contributor

Is this prompted by something? Otherwise I would prefer to keep 0.14 until reqwest updates to hyper 1.0, so there is only one copy in the tree.

@mraszyk
Copy link
Contributor Author

mraszyk commented Mar 8, 2024

Is this prompted by something? Otherwise I would prefer to keep 0.14 until reqwest updates to hyper 1.0, so there is only one copy in the tree.

This blocks bumping axum to 0.7 in icx-proxy. [BOUN-1008]

hyper-rustls = { version = "0.24.1", features = [
http-body-to-bytes = "0.2.0"
http-body-util = "0.1.0"
hyper-util = { version = "0.1.3", features = ["client", "client-legacy", "http2"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

These should all be optional dependencies too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

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've added optional = true to all of them if that's what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, they should only be enabled when the hyper feature is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok now?

Copy link
Contributor

@adamspofford-dfinity adamspofford-dfinity left a comment

Choose a reason for hiding this comment

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

This code does not build with --features hyper.

@mraszyk
Copy link
Contributor Author

mraszyk commented Mar 9, 2024

This code does not build with --features hyper.

What do you mean? The CI pipeline has succeeded and I could also successfully run cargo build --release --features hyper locally.

@adamspofford-dfinity
Copy link
Contributor

adamspofford-dfinity commented Mar 11, 2024

Apologies for not adding detail - the new errors look like this:

error[E0432]: unresolved import `http_body_to_bytes`
 --> ic-agent/src/agent/http_transport/hyper_transport.rs:7:5
  |
7 | use http_body_to_bytes::{http_body_to_bytes, http_body_to_bytes_with_max_length};
  |     ^^^^^^^^^^^^^^^^^^ use of undeclared crate or module `http_body_to_bytes`

Because the optional dependencies were not added to the hyper feature. I am not sure why it is succeeding on your local machine, you may be on a different commit, but it definitely does not succeed on mine. I am fixing the CI to detect this in #528.

@mraszyk
Copy link
Contributor Author

mraszyk commented Mar 11, 2024

I could reproduce the build failure and fixed it now.

@adamspofford-dfinity adamspofford-dfinity merged commit 5c34d88 into dfinity:main Mar 11, 2024
24 checks passed
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.

2 participants