sourcegraph / scip

SCIP Code Intelligence Protocol
Apache License 2.0
262 stars 31 forks source link

Rust bindings: please update protobuf dependency #284

Closed RalfJung closed 2 weeks ago

RalfJung commented 2 weeks ago

https://github.com/stepancheg/rust-protobuf/pull/737 fixes a bug in the protobuf crate where it wouldn't interact properly with build systems that use RUSTC_WRAPPER. This was released as a semver-compatible update (v3.7.1), but this crate pins protobuf to =3.2.0 (which is quite unusual in Rust), so sadly reverse dependencies can't pick up the new protobuf.

Would it be possible to update the protobuf dependency?

This is a blocker for https://github.com/rust-lang/rust/pull/127682.

RalfJung commented 2 weeks ago

Btw, after posting this issue I got a strange email about a failed pipeline linking to this.

varungandhi-src commented 2 weeks ago

Btw, after posting this issue I got a strange email about a failed pipeline

Created a PR to delete the job. https://github.com/sourcegraph/scip/pull/285

varungandhi-src commented 2 weeks ago

The 3.7.1 version of the protobuf crate code doesn't compile with Rust 1.71.0 (version used in this repo), looking into upgrading that first.

error[E0445]: crate-private trait `MapFieldAccessor` in public interface
   --> /Users/varun/.local/share/mise/installs/rust/1.71.0/registry/src/index.crates.io-6f17d22bba15001f/protobuf-3.7.1/src/reflect/acc/v2/map.rs:115:1
    |
17  |   pub(crate) trait MapFieldAccessor: Send + Sync + 'static {
    |   -------------------------------------------------------- `MapFieldAccessor` declared as crate-private
...
115 | / pub fn make_map_simpler_accessor_new<M, T>(
116 | |     name: &'static str,
117 | |     get_field: for<'a> fn(&'a M) -> &'a T,
118 | |     mut_field: for<'a> fn(&'a mut M) -> &'a mut T,
...   |
121 | |     M: MessageFull,
122 | |     MapFieldAccessorImpl<M, T>: MapFieldAccessor,
    | |_________________________________________________^ can't leak crate-private trait

error[E0446]: private type `MapFieldAccessorImpl<M, T>` in public interface
   --> /Users/varun/.local/share/mise/installs/rust/1.71.0/registry/src/index.crates.io-6f17d22bba15001f/protobuf-3.7.1/src/reflect/acc/v2/map.rs:115:1
    |
33  |   struct MapFieldAccessorImpl<M, T>
    |   --------------------------------- `MapFieldAccessorImpl<M, T>` declared as private
...
115 | / pub fn make_map_simpler_accessor_new<M, T>(
116 | |     name: &'static str,
117 | |     get_field: for<'a> fn(&'a M) -> &'a T,
118 | |     mut_field: for<'a> fn(&'a mut M) -> &'a mut T,
...   |
121 | |     M: MessageFull,
122 | |     MapFieldAccessorImpl<M, T>: MapFieldAccessor,
    | |_________________________________________________^ can't leak private type

Some errors have detailed explanations: E0445, E0446.
For more information about an error, try `rustc --explain E0445`.

Created a PR for the MSRV update here: https://github.com/stepancheg/rust-protobuf/pull/746

varungandhi-src commented 2 weeks ago

but this crate pins protobuf to =3.2.0 (which is quite unusual in Rust),

The reason for this is that the rust-protobuf set of crates doesn't make any guarantees on whether codegen by one version of the protobuf-codegen tool will be compatible with a different version of the protobuf crate. In this repo, we check in generated code using protobuf-codegen (so that downstream users don't need to run extra codegen steps), and we don't want downstream users to have the risk of run time errors due to a codegen-runtime version mismatch. So we've erred on the side of caution by only having one pinned version to reduce the chances of errors downstream.

RalfJung commented 2 weeks ago

Thanks for doing the dependency bump :)