Skip to content

Commit

Permalink
Bump smallvec version to 1.0
Browse files Browse the repository at this point in the history 
See servo/rust-smallvec#175 for changelog
  • Loading branch information
@messense
messense committed Nov 9, 2019
1 parent 307a2e6 commit cd63ded
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 3 deletions.
2 changes: 1 addition & 1 deletion .travis.yml
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
language: rust
rust:
- 1.21.0
- 1.36.0
- stable
- nightly
sudo: false
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ Unicode Standard Annex #15.
exclude = [ "target/*", "Cargo.lock", "scripts/tmp", "*.txt", "src/normalization_tests.rs", "src/test.rs" ]

[dependencies]
smallvec = "0.6"
smallvec = "1.0"
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Unicode character composition and decomposition utilities
as described in
[Unicode Standard Annex #15](http://www.unicode.org/reports/tr15/).

This crate requires Rust 1.21+.
This crate requires Rust 1.36+.

```rust
extern crate unicode_normalization;
Expand Down

7 comments on commit cd63ded

@mathstuf
Copy link

Choose a reason for hiding this comment

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

Why was such a large MSRV bump released as a patch release?

@mathstuf
Copy link

Choose a reason for hiding this comment

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

@messense

@Manishearth
Copy link
Member

Choose a reason for hiding this comment

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

Not all crates mark MSRV changes as breaking changes, it depends on the crate.

@mathstuf
Copy link

Choose a reason for hiding this comment

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

While I agree in general, this crate is very low-level and used in quite a number of places, so this may have moved the MSRV for quite a number of crates in the ecosystem.

@Manishearth
Copy link
Member

Choose a reason for hiding this comment

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

These crates should be pinning dependency versions if they care strongly about MSRV. Besides, users who care about msrv can pin this crate in their lockfiles: crates depending on this still satisfy their MSRV, just for a specific set of versions, which is pretty common.

The smallvec MSRV update moved to MaybeUninit which crates should be doing as soon as possible to avoid potential UB; in this case I care more about that than MSRV.

The minor bump here is good for non-MSRV reliers for the same reason it's bad for MSRV reliers: it avoids having to go across the entire ecosystem and bump everything. In this case the tradeoff is between MSRV and non-MSRV, and I've picked non-MSRV. Properly supporting MSRV here requires work that I don't have time to do.

@mathstuf
Copy link

Choose a reason for hiding this comment

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

Sorry for coming across as harsh; I was more seeking an explanation for the bump in a patch release (I should have worded it better). I had already bumped my crate's MSRV in CI before any response here and I wasn't expecting a revert to happen.

In my case, this crate is about 4 levels deep in the graph (reqwest -> url -> idna -> unicode-normalization). I can't pin it if they don't pin it (it's a library and, AFAIK, not using a Cargo.lock is the norm there and is ignored if it isn't an executable crate anyways (because otherwise you'd be stuck updating until all your deps update too for any crate C that A and B want to talk to each other about)).

UB

That's a good reason to update it; I'm satisfied that this made sense. It was just a surprise when my CI started breaking last night.

MSRV conflicts

I'm aware. It's very hard to ensure MSRV for a variety of reasons:

  • -Z minimal-versions is not standard (I've been filing dozens of PRs to fix minimum dependency specifications over the past month or so).
  • The crate code might be OK with 1.x, but if the test code needs 1.y, I can't say (with confidence) that 1.x is my MSRV because I can't test it with that version (the new cfg RFC for the Rust version should help alleviate this by at least allowing the parts of the test suite that do work to work).
  • Not many crates test their MSRV.
  • An even smaller number tests -Z minimal-versions with their MSRV (harder/more tedious in CI configurations because you need nightly for the generate-lockfile step and your MSRV for the rest).

@Manishearth
Copy link
Member

Choose a reason for hiding this comment

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

In my case, this crate is about 4 levels deep in the graph (reqwest -> url -> idna -> unicode-normalization). I can't pin it if they don't pin it

If you add an explicit dep on the crate you basically can. But that's kind of my point, Cargo's not designed for this, you can't impose additional requirements on transitive dependencies easily without locking down the whole dep tree. unicode-normalization mentions an MSRV but does not promise that MSRV bumps will be breaking, and I bet that's the case with most of your transitive dependencies. Folks distributing stuff on older Rust can use Cargo.lock, and crates that have a strong desire to maintain MSRV can pin transitive deps in their Cargo.toml.

A good solution here is for Cargo to support mentioning an MSRV and building for an MSRV, where it only looks for semver-compat crates that also satisfy the MSRV, when specified.

Please sign in to comment.