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

bump the version of croaring to 0.3.9 #2763

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

antiochp
Copy link
Member

Resolves #2693.

Bumps croaring to 0.3.9 which itself uses the latest croaring Unity build -
https://github.com/lemire/CRoaringUnityBuild

@antiochp antiochp added this to the 1.1.0 milestone Apr 19, 2019
@antiochp antiochp added the task label Apr 19, 2019
@ignopeverell
Copy link
Contributor

Just to double-check, the underlying storage formats stay compatible, correct?

@antiochp
Copy link
Member Author

antiochp commented Apr 23, 2019

Just to double-check, the underlying storage formats stay compatible, correct?

Good question - pretty sure yes, but I was making some untested assumptions here.
I will spend some time today testing a few different upgrade scenarios to make sure new code still reads the old local data etc.
I tested "fast sync" process but we do want to make sure a node can be upgraded in place safely.

Edit: Manual testing appears to confirm that these storage formats remain compatible.
There is nothing in the croaring release to indicate anything changed to contradict this.

@@ -14,7 +14,7 @@ bitflags = "1"
byteorder = "1"
failure = "0.1"
failure_derive = "0.1"
croaring = "0.3"
croaring = "0.3.9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any specific reason to stick with exactly 0.3.9?

core/Cargo.toml Outdated
@@ -12,7 +12,7 @@ edition = "2018"
[dependencies]
blake2-rfc = "0.2"
byteorder = "1"
croaring = "=0.3.8"
croaring = "=0.3.9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In chain crate we don't use = to pin the version, even practically it's the same because we specified patch version, but I'd suggest to be consistent

Copy link
Member Author

@antiochp antiochp Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch - I spent more than a few minutes staring at the = inside the quotes and the one outside the quotes and still managed to do this in an inconsistent way...

We should just specify it as croaring = "0.3.9" everywhere right?

And to your earlier point this is interpreted as "^0.3.9", i.e. would allow upgrade to 0.3.10 but not 0.4.0.
So yeah - there's no real point pinning it to an explicit version.

Edit: Actually is that correct? Or does ^0.3.9 allow an upgrade to 0.4.x?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually even0.3 would work, it would give now 0.3.9 (in Cargo.lock) and 0.3.10 (etc) on the next version bump. Cargo always takes the max version, I'm not sure what it chooses when we specify the exact semver like 0.3.9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Just my $0.02 answer on a peripheral question on an old and already completed ticket... :)

tl;dr semver: When you don't really care about the version, specify as little as possible to clearly show you don't care about the details.
Example from above:

blake2-rfc = "0.2"
byteorder = "1"

Then when you do care about the version, you can use =x.y.z to lock to that version, or maybe if there is one upgrade you can't accept you could to "<not.acceptable.version"
In any other case I think it's best to stick with specifying as little as possible in Cargo, and ALWAYS commit the lockfile when something has changed.

If automatic tests are good enough, maybe have a day of the month, or a couple days after having stabilized a major release or similar PR and code-sync, where cargo update and any punted major code refactorings happen. That limits the time span and impact of any workflow disruption.

Anyway, have a great day and thanks for keeping grin humming along!

@ignopeverell ignopeverell merged commit e72409d into mimblewimble:master Apr 24, 2019
@antiochp antiochp deleted the bump_croaring_0_3_9 branch April 25, 2019 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

croaring-rs dependency is out of date (needs updating to latest release)
4 participants