You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2024/08/31 11:42:27 UTC
[PR] Update prost and tonic dependencies [arrow-rs]
findepi opened a new pull request, #6337:
URL: https://github.com/apache/arrow-rs/pull/6337
# Which issue does this PR close?
None
# Rationale for this change
Update to latest minor release.
# What changes are included in this PR?
Update prost and tonic depedencies.
# Are there any user-facing changes?
No
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2329555496
thank you @Xuanwo @alamb for explaining this to me!
> We are not postponing the patch version update. Instead, we are letting the downstream project decide. Regardless of our actions, the downstream can update `prost` according to their own requirements.
One more use-case question if I may:
What happens if a downstream project does not depend on prost explicitly at all?
if will never pull in a patch release that `prost` maintainers released.
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2323184864
> DataFusion needs to use prost version matching arrow-flight, or there can be difficult to understand errors.
> This limited ability to update prost there [apache/datafusion#12237](https://github.com/apache/datafusion/pull/12237).
> Can the semver always be trusted, i.e. should different patch versions be _assumed_ to be always compatible?
Hi, as long as all of those crates depend on `prost 0.12`, regardless of which patch they are using, we will ultimately pick the latest one. For example, if `arrow` depends on `prost 0.12` and `datafusion` depends on `prost 0.12.2`, the application built by `datafusion` will use `prost 0.12.2` instead. Cargo will always merge all semver-compatible versions into one. It should be treated as a bug if crate maintainers have incompatibilities in this case.
> Secondly, if we defer the patch version update here to give downstream projects more freedom, when do we do it? "Never" sounds like a wrong answer, so what would the rule be?
We are not postponing the patch version update. Instead, we are letting the downstream project decide. Regardless of our actions, the downstream can update `prost` according to their own requirements.
For more information about dependency versions, we can visit https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio
---
Please let me know if I misunderstood your motivation. Thank you!
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2322889495
> Thank you for your contribution. However, I don't think it's necessary to update the patch versions of `prost` or `tonic` unless the old versions fail to build. Users who want the new versions of `prost` or `tonic` can upgrade on their own.
It is a good question about if we should always update to the most recent minor version
Given our CI tests (implicitly) test agains the most recent versions, updating to the most recent versions is probably the "safest" thing to 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] Update prost and tonic dependencies [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2332439836
> thank you @Xuanwo @alamb for explaining this to me!
>
> > We are not postponing the patch version update. Instead, we are letting the downstream project decide. Regardless of our actions, the downstream can update `prost` according to their own requirements.
>
> One more use-case question if I may: What happens if a downstream project does not depend on prost explicitly at all? if will never pull in a patch release that `prost` maintainers released.
What happens is that when someone runs `cargo update`, after a new `prost` is released, it will be updated in the `Cargo.lock` file (assuming cargo can resolve all the dependencies) and the project will use the latest version of `prost`
DataFusion / Arrow as they are meant to be used as libraries and not apps don't have a `Cargo.lock` file and allow their deps to float
> I'm considering introducing a CI with [`direct-minimal-versions`](https://doc.rust-lang.org/cargo/reference/unstable.html#direct-minimal-versions) to ensure the versions specified in Cargo.toml can build.
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] Update prost and tonic dependencies [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2323319339
> As I understand it, technically the only reason it would be **necessary** to update this project to depend on `prost 0.12.2` (rather than `prost 0.12`) is if this project used some feature that was introduced in `prost 0.12.1` or `prost 0.12.2` that was not present in `prost 0.12.0`.
Exactly! :100:
> I don't think it is particularly easy to check when this "use a new API in a depedency" happens because our CI tests are always run on the latest released version of the dependencies.
I'm considering introducing a CI with [`direct-minimal-versions`](https://doc.rust-lang.org/cargo/reference/unstable.html#minimal-versions) to ensure the versions specified in Cargo.toml can build.
I believe it's a great way to:
- Ensure our users can build with the specified version.
- Reduce the effort in keeping updates, allowing our maintainers to focus only on 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] Update prost and tonic dependencies [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2322912981
> It is a good question about if we should always update to the most recent minor version
>
> Given our CI tests (implicitly) test agains the most recent versions, updating to the most recent versions is probably the "safest" thing to do 🤔
This design makes sense as well.
The benefit of using `0.12` instead of `0.12.2` is that it allows users to continue using `0.12.1` without forcing them to upgrade to the latest version. Personally, I prefer making libraries compatible with most versions. I consider the version specified in `Cargo.toml` as the minimum compatible version of my crate.
Alternatively, we could commit `Cargo.lock` to the repository and keep updating it to ensure we build and test on the specific version.
Anyway, I'm not strongly opposed to this PR; I just have some personal preferences. It's fine to merge it as is, since it doesn't break our users (except for the upgrade of their dependency tree).
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2322985485
thank you @Xuanwo @alamb for your feedback!
> The benefit of using `0.12` instead of `0.12.2` is that it allows users to continue using `0.12.1` without forcing them to upgrade to the latest version.
DataFusion needs to use prost version matching arrow-flight, or there can be difficult to understand errors.
This limited ability to update prost there https://github.com/apache/datafusion/pull/12237.
Can the semver always be trusted, i.e. should different patch versions be _assumed_ to be always compatible?
Secondly, if we defer the patch version update here to give downstream projects more freedom, when do we do it? "Never" sounds like a wrong answer, so what would the rule be?
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6337:
URL: https://github.com/apache/arrow-rs/pull/6337#issuecomment-2323306423
As I understand it, technically the only reason it would be **necessary** to update this project to depend on `prost 0.12.2` (rather than `prost 0.12`) is if this project used some feature that was introduced in `prost 0.12.1` or `prost 0.12.2` that was not present in `prost 0.12.0`.
I don't think it is particularly easy to check when this "use a new API in a depedency" happens because our CI tests are always run on the latest released version of the dependencies.
The CI tests run on the latest version of the dependencies on purpose so that we aren't continually in reactionary mode dealing with updated new releases
If we had bandwidth and this testing gap was a concern, we could implement more sophisticated tests / processes for dependency management (such as using a Cargo.lock file for testing, as @Xuanwo mentioned) or various other ideas
--
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] Update prost and tonic dependencies [arrow-rs]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi closed pull request #6337: Update prost and tonic dependencies
URL: https://github.com/apache/arrow-rs/pull/6337
--
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