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

Rust bindings: please update protobuf dependency #284

Closed
RalfJung opened this issue Oct 16, 2024 · 5 comments · Fixed by #287
Closed

Rust bindings: please update protobuf dependency #284

RalfJung opened this issue Oct 16, 2024 · 5 comments · Fixed by #287

Comments

@RalfJung
Copy link

RalfJung commented Oct 16, 2024

stepancheg/rust-protobuf#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 rust-lang/rust#127682.

@RalfJung RalfJung changed the title Please update protobuf dependency Rust bindings: please update protobuf dependency Oct 16, 2024
@RalfJung
Copy link
Author

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

@varungandhi-src
Copy link
Contributor

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

Created a PR to delete the job. #285

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Oct 17, 2024

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: stepancheg/rust-protobuf#746

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Oct 17, 2024

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.

bors added a commit to rust-lang/rust-analyzer that referenced this issue Oct 17, 2024
Bump version of scip crate

Follow up to sourcegraph/scip#284

Manually verified that SCIP generation works OK for rust-analyzer itself.

cc `@RalfJung`
@RalfJung
Copy link
Author

Thanks for doing the dependency bump :)

lnicola pushed a commit to lnicola/rust that referenced this issue Oct 17, 2024
Bump version of scip crate

Follow up to sourcegraph/scip#284

Manually verified that SCIP generation works OK for rust-analyzer itself.

cc `@RalfJung`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants