You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "awesterb (via GitHub)" <gi...@apache.org> on 2026/05/15 12:28:49 UTC
[PR] Makes `reqwest` optional [arrow-rs-object-store]
awesterb opened a new pull request, #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724
# 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`](https://docs.rs/object_store/latest/object_store/client/trait.HttpConnector.html) 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`](https://docs.rs/object_store/latest/object_store/client/struct.Certificate.html). 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>.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3399476082
##########
.github/workflows/ci.yml:
##########
@@ -57,6 +57,14 @@ jobs:
run: cargo clippy --features azure -- -D warnings
- name: Run clippy with http feature
run: cargo clippy --features http -- -D warnings
+ - name: Run clippy with aws-base feature
+ run: cargo clippy --no-default-features --features aws-base -- -D warnings
+ - name: Run clippy with azure-base feature
+ run: cargo clippy --no-default-features --features azure-base -- -D warnings
+ - name: Run clippy with gcp-base feature
+ run: cargo clippy --no-default-features --features gcp-base -- -D warnings
+ - name: Run clippy with http-base feature
+ run: cargo clippy --no-default-features --features http-base -- -D warnings
Review Comment:
```suggestion
run: cargo clippy --no-default-features --features http-base -- -D warnings
- name: Verify base features do not enable reqwest
run: |
if cargo tree \
--no-default-features \
--features aws-base,azure-base,gcp-base,http-base \
--edges normal \
--prefix none \
-f '{p}' \
| grep -E '^reqwest v'; then
echo "reqwest must not be enabled by *-base features"
exit 1
fi
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4690595677
I merged up to resolve some conflicts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685144311
Thanks everyone! Glad we found a way to move forward with this.
I did a brief review, it looks great directionally.
One related question: should we also add a small CI check that asserts the dependency contract directly, i.e. that the `*-base` features do not pull in `reqwest` at all? Maybe we could add one lightweight check for the combined base-feature case, something like:
```bash
cargo tree \
--no-default-features \
--features aws-base,azure-base,gcp-base,http-base \
--edges normal \
-i reqwest
```
and fail CI if `reqwest` is present.
Not a blocker, but if `*-base` is meant to be a supported “no reqwest” contract, this seems like a useful guardrail against accidentally reintroducing it in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Makes `reqwest` optional [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3342155266
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
Review Comment:
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
```diff
- 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4693276049
Thanks @kevinjqliu; I'll make that change on this branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3404932918
##########
.github/workflows/ci.yml:
##########
@@ -57,6 +57,14 @@ jobs:
run: cargo clippy --features azure -- -D warnings
- name: Run clippy with http feature
run: cargo clippy --features http -- -D warnings
+ - name: Run clippy with aws-base feature
+ run: cargo clippy --no-default-features --features aws-base -- -D warnings
+ - name: Run clippy with azure-base feature
+ run: cargo clippy --no-default-features --features azure-base -- -D warnings
+ - name: Run clippy with gcp-base feature
+ run: cargo clippy --no-default-features --features gcp-base -- -D warnings
+ - name: Run clippy with http-base feature
+ run: cargo clippy --no-default-features --features http-base -- -D warnings
Review Comment:
I applied this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "awesterb (via GitHub)" <gi...@apache.org>.
awesterb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685577129
Thanks for all the movement on this! I've added the comments to `Cargo.toml` [proposed](https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3399237618) by @kevinjqliu.
Updating the `docs.rs` documentation, and adding additional checks to the CI are worth doing, but I'd prefer to keep the scope of my own changes limited.
Edits by maintainers are enabled in case anyone wants to contribute to this PR. Feel free!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4684505837
I merged this PR up from main to resolve a conflict.
It seems like @kevinjqliu has gotten consensus on the naming here (`base`)
- https://github.com/apache/arrow-rs-object-store/issues/744
@awesterb are you willing to implement @kylebarron 's suggestion on https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3342155266 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4693311783
ty! i couldnt verify the wasm32 ci issue, but i suspect its the same issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron merged PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "awesterb (via GitHub)" <gi...@apache.org>.
awesterb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4684529170
> @awesterb are you willing to implement @kylebarron 's suggestion on [#724 (comment)](https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3342155266) ?
Will do!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Makes `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3391252684
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
Review Comment:
i actually like `-base` more 😄 This is my mental model:
```
cloud-base = shared cloud code
*-base = provider-specific code without default reqwest
reqwest = built-in HTTP transport
cloud = compatibility alias for cloud-base + reqwest
aws/azure/gcp/http = provider base + reqwest
```
```
# Shared cloud/provider implementation dependencies.
# This intentionally does NOT include reqwest.
cloud-base = [
"serde",
"serde_json",
"quick-xml",
"hyper",
"chrono/serde",
"base64",
"rand",
"ring",
"http-body-util",
"form_urlencoded",
"serde_urlencoded",
"tokio",
]
# Built-in reqwest-based HTTP transport.
reqwest = [
"dep:reqwest",
"reqwest/stream",
]
# Compatibility/convenience alias.
# Historically, cloud meant "common cloud support", including reqwest.
# Keep that behavior for existing users.
cloud = [
"cloud-base",
"reqwest",
]
# Provider base features.
# These compile provider logic without forcing reqwest.
azure-base = [
"cloud-base",
"httparse",
]
gcp-base = [
"cloud-base",
"rustls-pki-types",
]
aws-base = [
"cloud-base",
"crc-fast",
"md-5",
]
http-base = [
"cloud-base",
]
# Batteries-included provider features.
# These preserve existing behavior: enabling aws/azure/gcp/http gives
# the default reqwest transport.
azure = [
"azure-base",
"reqwest",
]
gcp = [
"gcp-base",
"reqwest",
]
aws = [
"aws-base",
"reqwest",
]
http = [
"http-base",
"reqwest",
]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4687892629
the CI failure is not related to this PR, fixing the issue in #746
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4690578473
> the CI failure is not related to this PR, fixing the issue in #746
I merge that in and will merge up from main for this one too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685635040
thank you @awesterb for all the changes. Im happy to follow up and add the docs changes + CI checks if necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4690692566
I have filed follow on tickets
- https://github.com/apache/arrow-rs-object-store/issues/748
- https://github.com/apache/arrow-rs-object-store/issues/749
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4693219736
looks like the ci failed on `cargo test --features=aws,azure,gcp,http`, could due to the 2 `feature = "cloud",` declarations:
https://github.com/apache/arrow-rs-object-store/blob/f554e600e83a43275367db3f6b0eb15343865873/src/integration.rs#L1440-L1527
we need to use `feature = "reqwest",` instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4690582675
> Updating the docs.rs documentation, and adding additional checks to the CI are worth doing, but I'd prefer to keep the scope of my own changes limited.
Agreed - I will file follow on tickets for these items
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685786301
Is the CI failure pre-existing or new in this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685036495
> Done. Strictly speaking, it does cause a breaking change according to cargo semver-checks:
Yes I agree -- this is going to be a breaking change, but that is ok as we are about to put out a non semver compatible release
- https://github.com/apache/arrow-rs-object-store/issues/657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Makes `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3382450414
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
Review Comment:
+1 we'd want to coordinate on naming since #707 is also renaming these features.
Also see https://github.com/apache/arrow-rs-object-store/pull/707#pullrequestreview-4460657534
both `-base` and `-custom` makes sense to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "awesterb (via GitHub)" <gi...@apache.org>.
awesterb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4698300168
> 💯 thanks @awesterb for the PR and everyone else for the reviews
No, thank you all for willing to incorporating this feature! This helps us cut down our dependency tree a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Makes `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3391275345
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
Review Comment:
created an issue to facilitate with discussion
https://github.com/apache/arrow-rs-object-store/issues/744
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Makes `reqwest` optional [arrow-rs-object-store]
Posted by "awesterb (via GitHub)" <gi...@apache.org>.
awesterb commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3388015903
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
Review Comment:
Renaming `-custom` to `-base` is completely fine by me too. (Though it might read a bit odd in the `object_store` code: custom is not an additional feature.)
I agree the `cloud` adds nothing to `reqwest`. It is only there for backwards compatibility. If you want to make this a breaking change it can be removed, of course.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "awesterb (via GitHub)" <gi...@apache.org>.
awesterb commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4685021716
> @awesterb are you willing to implement @kylebarron 's suggestion on [#724 (comment)](https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3342155266) ?
Done. Strictly speaking, it does cause a breaking change according to `cargo semver-checks`:
```
Description:
A feature no longer enables another feature in this package's Cargo.toml. This will break downstream crates which rely on the implied feature being enabled.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-feature-remove-another
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/feature_no_longer_enables_feature.ron
Failed in:
feature gcp no longer enables cloud
feature azure no longer enables cloud
feature aws no longer enables cloud
feature http no longer enables cloud
```
(But there are already other breaking changes.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#discussion_r3399237618
##########
Cargo.toml:
##########
@@ -82,12 +82,18 @@ futures-channel = {version = "0.3", features = ["sink"]}
[features]
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"]
+cloud-base = ["serde", "serde_json", "quick-xml", "hyper", "chrono/serde", "base64", "rand", "ring", "http-body-util", "form_urlencoded", "serde_urlencoded", "tokio"]
Review Comment:
nit: do you think its helpful to include some comments to explain why the features are structured this way?
```
# Shared cloud/provider implementation dependencies.
# This intentionally does NOT include reqwest.
cloud-base = [...]
# Built-in reqwest-based HTTP transport.
reqwest = [
"dep:reqwest",
"reqwest/stream",
]
# Compatibility/convenience alias.
# Historically, cloud meant "common cloud support", including reqwest.
# Keep that behavior for existing users.
cloud = [...]
# Provider base features.
# These compile provider logic without forcing reqwest.
azure-base = [...]
gcp-base = [...]
aws-base = [...]
http-base = [...]
# Batteries-included provider features.
# These preserve existing behavior: enabling aws/azure/gcp/http gives
# the default reqwest transport.
azure = [...]
gcp = [...]
aws = [...]
http = [...]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make `reqwest` optional [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #724:
URL: https://github.com/apache/arrow-rs-object-store/pull/724#issuecomment-4693504187
💯 thanks @awesterb for the PR and everyone else for the reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org