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

Fix unintentional breaking API change in `Serializer`/`Deserializer` #128

Merged
merged 1 commit into from Mar 2, 2017

Conversation

@antrik
Copy link
Contributor

antrik commented Mar 1, 2017

While introducing selectable endianness in
#103 , the new type parameter
has been hidden from the public serialize(), deserialize() etc.
functions, and only made available through an alternate API entry point.
The same kind of encapsulation also needs to be performed for the public
Serializer and Deserializer types.

While introducing selectable endianness in
#103 , the new type parameter
has been hidden from the public `serialize()`, `deserialize()` etc.
functions, and only made available through an alternate API entry point.
The same kind of encapsulation also needs to be performed for the public
`Serializer` and `Deserializer` types.
@antrik
Copy link
Contributor Author

antrik commented Mar 1, 2017

Relevant issue: servo/ipc-channel#154

I'm not entirely sure what I'm doing here -- but this patch does seem to work for fixing the regression, as well as making the endianness selection available through the endian_choice entry point.

I guess I should add some test cases for this?...

@TyOverby
Copy link
Collaborator

TyOverby commented Mar 2, 2017

Don't worry about tests. This looks good to me.

@TyOverby TyOverby merged commit 5219ac8 into servo:master Mar 2, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@TyOverby
Copy link
Collaborator

TyOverby commented Mar 2, 2017

My question is: how did this break people? I pushed a new version to crates.io immediately after merging the endian_choice branch. Shouldn't this should break people when they specify alpha-3 not randomly?

@TyOverby
Copy link
Collaborator

TyOverby commented Mar 2, 2017

I've published alpha-5

@dtolnay
Copy link
Collaborator

dtolnay commented Mar 2, 2017

Cargo will silently upgrade 1.0.0-alpha2 to 1.0.0-alpha3. I would not consider this a breaking change in Bincode. Cargo should be smarter (rust-lang/cargo#2222) and ipc-channel should have used "=1.0.0-apha2".

@TyOverby
Copy link
Collaborator

TyOverby commented Mar 2, 2017

Cargo will silently upgrade 1.0.0-alpha2 to 1.0.0-alpha3

Wtf. I could have sworn that "alpha" releases were always precise

@dlrobertson
Copy link

dlrobertson commented Mar 2, 2017

Why was LittleEndian used instead of NativeEndian?

@dlrobertson
Copy link

dlrobertson commented Mar 2, 2017

Not super important... Just wondered

@antrik
Copy link
Contributor Author

antrik commented Mar 2, 2017

@dtolnay well, it clearly was a breaking change in bincode -- the actual question at hand is whether a breaking change from one alpha version to another is a valid thing to do. (Ignoring for now that AIUI it was an unintended breaking change in this case, and thus a bug either way.)

The discussion in rust-lang/cargo#2222 , and in particular rust-lang/cargo#2222 (comment) , suggests that going from alpha2 to alpha3 should be considered safe. I'd argue this violates SemVer rules, since SemVer states there are no API guarantees whatsoever for pre-release versions... But then again, Cargo doesn't really follow SemVer rules anyway.

@antrik
Copy link
Contributor Author

antrik commented Mar 2, 2017

@danlrobertson from a management point of view, the answer is simply that I followed what the original PR introducing the problem did in other places...

From a technical point of view, it is quite debatable whether NativeEndian would indeed be a better default here than LittleEndian. Since bincode can in fact be used in network communication, not forcing a fixed endianness would potentially break things. (Assuming NativeEndian actually did what the name suggests...)

For ipc-channel, I suggested that hard-coding NativeEndian would be preferable, because there we know that we never change endianness between sender and receiver -- so it's a different situation from bincode in general.

(And in any case, this is an orthogonal concern from the purpose of my PR; so probably should be discussed in a separate issue :-) )

@TyOverby
Copy link
Collaborator

TyOverby commented Mar 2, 2017

@danlrobertson

Why was LittleEndian used instead of NativeEndian?

I use bincode for game networking and save-file writing. I think that the default should be the "safer" option.

@dlrobertson
Copy link

dlrobertson commented Mar 2, 2017

Makes sense... My thinking was too tightly coupled with ipc-channel. Didn't think about communication accros the wire

@dtolnay dtolnay mentioned this pull request Apr 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.