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

upgrade deps including tokio 1.0 #43

Closed
wants to merge 2 commits into from

Conversation

naamancurtis
Copy link

Hey,

I guess this relates to #41. This just bumps up a couple of dependencies to their latest versions, it appears to pass all tests. I've tried it with an application I use this with and all seems to be working as intended.

Hopefully this helps, let me know if you'd like anything extra done on it.

Thanks

@gklijs
Copy link
Owner

gklijs commented Feb 15, 2021

Thanks for the pr. I have some time this week to finish up.
Something is breaking now, seems like different versions of the bytes crate.

error[E0308]: mismatched types
  --> src/async_impl/proto_decoder.rs:52:50
   |
52 |             BytesResult::Null => Ok(Value::Bytes(Bytes::new())),
   |                                                  ^^^^^^^^^^^^ expected struct `bytes::bytes::Bytes`, found struct `bytes::Bytes`
   |
   = note: perhaps two different versions of crate `bytes` are being used?
error[E0308]: mismatched types
  --> src/async_impl/proto_decoder.rs:56:56
   |
56 |             BytesResult::Invalid(i) => Ok(Value::Bytes(Bytes::from(i))),
   |                                                        ^^^^^^^^^^^^^^ expected struct `bytes::bytes::Bytes`, found struct `bytes::Bytes`
   |
   = note: perhaps two different versions of crate `bytes` are being used?
error[E0308]: mismatched types
  --> src/blocking/proto_decoder.rs:45:50
   |
45 |             BytesResult::Null => Ok(Value::Bytes(Bytes::new())),
   |                                                  ^^^^^^^^^^^^ expected struct `bytes::bytes::Bytes`, found struct `bytes::Bytes`
   |
   = note: perhaps two different versions of crate `bytes` are being used?
error[E0308]: mismatched types
  --> src/blocking/proto_decoder.rs:49:56
   |
49 |             BytesResult::Invalid(i) => Ok(Value::Bytes(Bytes::from(i))),
   |                                                        ^^^^^^^^^^^^^^ expected struct `bytes::bytes::Bytes`, found struct `bytes::Bytes`
   |
   = note: perhaps two different versions of crate `bytes` are being used?

Also since the api strictly doesn't change, and nothing new was added, I was thinking about having 2.0.2 as the new number. Although it might be breaking when using avro.. But having a major release just for updating dependencies feels a bit awkward.

@gklijs
Copy link
Owner

gklijs commented Feb 15, 2021

Protofish has it's bytes dependency as bytes ^0.5 nicest would be if there could be a new version with increased version for bytes.

@naamancurtis
Copy link
Author

Apologies, I didn't check the bytes versions from Protofish.

I can drop the Bytes dep back to what it was before.

My reasoning for the major was that this is a breaking change with regards to the Tokio Runtime. If you try and run a tokio 0.2 app with this new version, this version will panic, and the same in reverse.

@gklijs
Copy link
Owner

gklijs commented Feb 15, 2021

Created Rantanen/protofish#1, It would be nice if all dependencies used the same bytes version.
I also want to clean up the code a little, seems like or_insert_with_key which is now stable should clean the cashing code a bit.
Also might expand the integration tests, by also including reading message from Java send with this library (extending https://github.com/gklijs/schema_registry_test_app).

@gklijs
Copy link
Owner

gklijs commented Feb 15, 2021

Tokio is just a dev depency, so I think it should work either way? I might ask around what's common in these cases. Because of the tight integration with some of the dependencies, almost any bump could be breaking. But maybe that's just the way it is for crates like these.

@naamancurtis
Copy link
Author

Reqwest v0.11 has a dependency on Tokio 1.0, Reqwest v.0.10 has one on Tokio 0.2, so I think there is an inherent breaking change there.

@gklijs
Copy link
Owner

gklijs commented Feb 20, 2021

I just did the fixes, thanks for trying out earlier, so that protofish could now be updated as well. Locally all tests run fine. There seems something strange with travis, where the exit code is always 1 now. Might go for github actions or something else in the future.

@gklijs gklijs closed this Feb 20, 2021
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.

None yet

2 participants