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 dev dependencies #967

Closed
wants to merge 3 commits into from

Conversation

tcharding
Copy link
Member

Just the last patch, the first two are #964

Now we have removed Rust 1.29 from CI we can upgrade the serde dev dependency and remove the ryu dependency altogether.

Suggested during review of #964.

In preparation for starting to merge patches that rely on an MSRV of
1.41 update the CI pipeline.
We no longer need to test Rust 1.29, remove the pinning from the test
script.
Now we no longer need to support Rust 1.29 we can use latest `serde` and
remove the `ryu` dev dependency altogether.
@tcharding tcharding marked this pull request as draft April 22, 2022 00:25
@@ -45,12 +45,10 @@ serde = { version = "1", features = [ "derive" ], optional = true }
hashbrown = { version = "0.8", optional = true }

[dev-dependencies]
serde_json = "<1.0.45"
serde_json = "1"
serde_test = "1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK version specification like this (1 instead of 1.0.0) is considered an anti-pattern but I myself don't care much. It's equivalent to 1.0.0 and it'd only be a problem if it didn't work with minimal versions.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't really care (or fully understand the distinction) but we might as well follow best practices.

Copy link
Contributor

@dpc dpc Apr 22, 2022

Choose a reason for hiding this comment

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

1 allows deps resvoler to pull in any version. And oftentimes what you actually tested with is not 1.0.0 but 1.0.666 or someting, and when resolver for whatever reason pulls in 1.0.3 and things are not working, user will get a weird error in your crate, not the crate that actually caused the 1.0.3 to pulled in.

Because of that it is recommend to put in a version that you actually know is working and thus enforce it as a minimum one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That cargo +nightly -Z minimal-versions update trick is pretty cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I had a bit of a play with it and was getting grief from rust-secp256k1, I chased that down to rand and it gets resolved during the MSRV and rand 0.8 bump PR. So I'll just leave this sitting here until we do secp. Cheers

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tcharding was just thinking about opening an issue to audit. But maybe we could also add a test into CI? The problem is minimal-versions could cause breakage even if dependency of dependency is incorrect. OTOH we would get the chance to fix it for them by just setting the correct version. :)

The main difference between 1 and 1.0.0 is the explicitness. 1 may indicate "I didn't make effort to check which one works". However given how many versions serde has, I'd personally be suspicious even if there was 1.0.0. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not really sure whats the best thing to do about our version numbers. Any more suggestions from anyone on how to find a suitable version number?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting the correct minimal version is obviously the most useful (flexible) for consumers but may be a bit annoying to figure out depending on circumstances. I wouldn't mind trying at some point though.

@tcharding
Copy link
Member Author

Leaving this one for you @Kixunil, if you feel like it ever.

@tcharding tcharding closed this May 26, 2022
@tcharding tcharding deleted the upgrade-dependencies branch May 26, 2022 05:27
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

4 participants