Skip to content

Makes reqwest optional#724

Open
awesterb wants to merge 1 commit into
apache:mainfrom
awesterb:main
Open

Makes reqwest optional#724
awesterb wants to merge 1 commit into
apache:mainfrom
awesterb:main

Conversation

@awesterb
Copy link
Copy Markdown

Which issue does this PR close?

Closes #723 .

Rationale for this change

We're using object_store with a code base that already uses awc as HTTP-client, and would like to avoid having reqwest as additional HTTP client alongside awc. An alternative HttpConnector can already be provided currently, but the reqwest dependency can not yet be avoided.

Also, since reqwest can't be used for wasm32-wasip1 anyways, not having to package it there is nice too.

What changes are included in this PR?

Adds a new reqwest feature that gates dep:reqwest, but is enabled by cloud by default. This means reqwest will still be pulled in by the azure, gcp, aws and http features, as is the case now. For avoiding reqwest, the features cloud-base, azure-base, gcp-base, aws-base and http-base are added.

Some features that depend on reqwest types I gated behind reqwest too, such as client::Certificate. To keep the pull request minimal and non-breaking, I did not attempt to rewrite those to be reqwest-independent. I believe all of the gated features can be avoided when providing your own HttpConnector. (Besides, most of these features were already gated behind !wasm32.)

Tests that use reqwest-gated types were gated behind reqwest too.

Added to CI extra clippy jobs to catch missing gates in the future, and a build job for wasm32-wasip1 without reqwest.

Where reqwest::{Method,StatusCode,header} was used, this was replaced by the http types.

I considered copying the X-no-crypto naming scheme (aws-no-crypto, gcp-no-crypto ...) from GH-707, but decided not to use aws-no-reqwest (etc.) since that would necessitate aws-no-crypto-no-reqwest (and worse) in the future. (Combining !707 could make sense, having X-base mean no crypto and no reqwest.)

Are there any user-facing changes?

The feature reqwest that gates dep:reqwest, and the features cloud-base, azure-base, gcp-base, aws-base and http-base that avoid reqwest.

No breaking changes (according to https://crates.io/crates/cargo-semver-checks.)

Copy link
Copy Markdown
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Thank you this seems sensible to me and makes sense, I like the formulation of having a minimal feature flag and a more batteries included one. We should roll this into the crypto work as well.

Comment thread Cargo.toml
default = ["fs"]
cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest", "reqwest/stream", "chrono/serde", "base64", "rand", "ring", "http-body-util", "form_urlencoded", "serde_urlencoded", "tokio"]
azure = ["cloud", "httparse"]
cloud = ["cloud-base", "reqwest"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we discuss feature naming? Either now or in an issue before the next release? Because now cloud is an alias for reqwest, essentially. Would it read clearer if say azure was defined as

- azure = ["azure-base", "cloud"]
+ azure = ["azure-base", "reqwest"]

because azure-base already depends on cloud-base. We have a diamond dependency graph here.

Should we have some docs about feature flag options?

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.

Make reqwest optional

3 participants