Minimum supported Rust version v.s. SemVer

[rjloura]

When running clippy on rust 1.35 I encounter the following error:

error[E0658]: use of unstable library feature 'alloc': this library is unlikely to be stabilized in its current form or name (see issue #27783)
  --> /home/rui/.cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-1.0.0/lib.rs:38:1
   |
38 | extern crate alloc;
   | ^^^^^^^^^^^^^^^^^^^

For my project the url crate is pulled in by gotham, but this occurs when I run clippy on rust-url by itself. The tree is:

$ cargo tree
url v2.1.0 (/home/rui/git/rust-url)
├── idna v0.2.0 (/home/rui/git/rust-url/idna)
│   ├── matches v0.1.8
│   ├── unicode-bidi v0.3.4
│   │   └── matches v0.1.8 (*)
│   └── unicode-normalization v0.1.11
│       └── smallvec v1.0.0 <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
├── matches v0.1.8 (*)
└── percent-encoding v2.1.0 (/home/rui/git/rust-url/percent_encoding)
SNIP!

What I don't understand is how this tree was built. idna is currently at version 0.2.0 and it specifies unicode-normalization version of 0.1.5 (https://github.com/servo/rust-url/blob/master/idna/Cargo.toml).

The only thing I did notice was that unicode-normalization was bumped to it's new version 3 days ago and that brought in the smallvec v1.0.0 dependency. (https://github.com/unicode-rs/unicode-normalization/blame/master/Cargo.toml)

For this project I need to stay on rust 1.35. I am not sure why the dependency tree changed without the rust-url version changing.

[SimonSapin]

It looks like unicode-rs/unicode-normalization#46 increased our minimum Rust version from under us. The last PR that we merged succeeded with 1.33:

rust-url/.travis.yml

Line 6 in 7d2c9d6

- rust: 1.33.0

[SimonSapin]

For this project I need to stay on rust 1.35.

You may be able to do this by adding a dependency from your crate unicode-normalization, with a requirement for some older version number.

[rjloura]

For this project I need to stay on rust 1.35.

You may be able to do this by adding a dependency from your crate unicode-normalization, with a requirement for some older version number.

@SimonSapin thank you for taking a look at this. Unfortunately, including unicode-normalization = "0.1.5" doesn't fix the issue.

Can you help me understand how this is happening? If the versions of rust-url and idna haven't changed, and idna @ v0.2.0 includes a dependency on unicode-normalization with a version of 0.1.5 (https://github.com/servo/rust-url/blob/master/idna/Cargo.toml#L3), why is rust-url > idna bringing in unicode-normalization 0.1.11.

It is also worth noting that unicode-normalization @ v0.1.5 has NO dependency on smallvec at all. The smallvec dependency came in at unicode-normalization @ v0.1.7. The issue is actually with smallvec v1.0.0 which shouldn't be brought in at all under idna v0.2.0 > unicode-normalization 0.1.5

[SimonSapin]

unicode-normalization = "0.1.5" in the [dependencies] section of Cargo.toml means any version from 0.1.5 but less than 0.2.0: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html

Try unicode-normalization = "= 0.1.5"

[rjloura]

Try unicode-normalization = "= 0.1.5"

Ah thank you @SimonSapin! That worked. Sorry for the noise.

[SimonSapin]

I didn’t realize that this would also affect our CI since we’re testing on 1.33 which was the minimum supported version before this unicode_normalization change. Right now any PR fails: #571 (comment)

The maintainers of that crate have decided that they do not consider increasing the minimum supported Rust version to be a breaking change that requires a corresponding SemVer version number: unicode-rs/unicode-normalization#50

For this repository we never formalized a policy, although url 1.x was still tested with Rust 1.17 when we released url 2.0.0.

So as far as I can tell we’ll have to pick one of:

• Restrict our dependency (in idna) on unicode_normalization to something like >= 0.1.5, <= 0.1.9
• Or adopt the same policy of accepting MSRV increase and not versioning them a as breaking change

@nox, what do you think?

[nox]

I think it's pretty bad for unicode-normalization to not care about MSRV, but this is not a hill I'm willing to die on. Restricting dependencies with ranges cause more issues than fix them in the long run, so I'm not in favour of that. I guess we can just document that this crate doesn't care about MSRV. :(

[SimonSapin]

Well I wouldn’t phrase it as “not care”, but yeah that might be easiest.

[o0Ignition0o]

In order to reply to this comment
I have just bumped the minimum supported rustc version (in the pull request branch), which to me is a breaking change IMO (hence me saying i can probably rollback the minimum rustc version support, and pin unicode-normalization instead).

I feel a bit stuck because I can either keep an outdated and possibly unsound (yet rustc 1.33 compatible) dependency by pinning unicode-normalization version 0.1.9, or bump the minimum supported rustc version to 1.36, which probably requires to bump rust-url as well in order to respect semver.

This issue adds even more weight as to why we might want to pin unicode-normalization (0.1.5 if i get the thread correctly ?)

[rjloura]

For my use case it would be more beneficial to have rust-url pin the unicode-normalization version.

If I understand the other alternative (not considering raising MSRV to be a breaking change) correctly, anyone attempting to use rust-url with rust 1.35 (or lower) would have to pin rust-url and (probably) its dependencies explicitly. The decision by by unicode-normalization to not regard MSRV as a breaking change seems to have a cascade effect, and from my view it would be good to stop that cascade as close to the source as possible.

Just my 2 cents as a random user of these crates.

[SimonSapin]

It looks like https://crates.io/crates/smallvec/0.6.13 depends on https://crates.io/crates/maybe_uninit, and so should be sound on 1.36+

[o0Ignition0o]

I would say soundness is the most important, so I should definitely bump the MSRV in my PR. But it would also require a major bump in rust-url as well since there's no backwards compatibility wrt the changes it implies ?

Edit: Actually it doesn't have to unicode-rs/unicode-normalization#46 (comment)

[SimonSapin]

• On Rust 1.36+, smallvec 0.6.13 is as sound as smallvec 1.0.0 since they both use MaybeUninit

• On older Rusts, smallvec 0.6.13 technically has Undefined Behavior since it uses mem::uninitialized to initialize a generic array. In unicode_normalization that array contains char which is effectively a u32 normally guaranteed to be no larger than 0x10FFFF. Uninitialized memory can break that guarantee. However I am not aware of any actual miscompilation ever happening in this cases. We are not concerned about new optimizations that a future version of the compiler could develop relying on that Undefined Behavior not happening, since mem::uninitialized is only used on older Rust version.

So I’m skeptical of the soundness justification for what unicode_normalization did.

Making the next version of url be 3.0.0 would not help: existing versions are similarly affected by the unicode_normalization change.

Whatever we end up doing about all this (this might be incrementing the version in .travis.yml to 1.36, with no other change), let’s do it in a dedicated PR that closes this issue rather than as part of #537.

[dekellum]

You might consider releasing an idna 0.2.1 with unicode_normalization and possibly other deps constrained. Then eventually releasing an idna 0.3.0 (and other MINOR version bumps, incl url) with MSRV increased, and the constraint lifted?

[mbrubeck]

To fix CI, we could check in a Cargo.lock or run cargo update -p unicode-normalization --precise 0.1.9 in the CI script for rustc 1.33.

I think it's reasonable to expect that people who need to stick with old toolchains can also stick with old versions of pinned dependencies in Cargo.lock.

[mbrubeck]

You might consider releasing an idna 0.2.1 with unicode_normalization and possibly other deps constrained.

I'm against releasing versions with max-version constraints. This will end up breaking builds for any crate whose dependency graph includes both idna 0.2.1 and some crate with a dependency like unicode-normalization = "0.1.10", since these now have conflicting constraints.

The solution for people who need to keep their builds pinned to an old Rust toolchain is to also keep their dependencies pinned in Cargo.lock. This happens automatically in the normal Rust workflow! We don't need to abuse Cargo.toml to do this for them. (Yes, if they are adding new dependencies to a project using an old toolchain, they may need to do some extra work to find compatible dependencies.)

The people who do feel pain from this type of "breakage" are library crate maintainers, because most of us unfortunately have CI that runs without the benefit of Cargo.lock. This is one reason I don't like the "official" recommendation for library crates not to version Cargo.lock. It would be better to version Cargo.lock and also have a CI job that runs with the latest dependencies and latest toolchain. Library crates need reproducible builds in CI also! If every build pinned to old Rust toolchains were using Cargo.lock, then almost nobody would have been inconvenienced by the unicode-normalization update.

[dekellum]

I'm against releasing versions with max-version constraints. This will end up breaking builds for any crate whose dependency graph includes both idna 0.2.1 and some crate with a dependency like unicode-normalization = "0.1.10", since these now have conflicting constraints.

I agree that such a patch fix (idna 0.21 constrained) is only a temporary solution, but one that IMO is preferable to abandoning the MSRV policy here.

Its also interesting that if cargo implements the constraint as per rust-lang/rfcs#2495, the eventuality frequently used as an excuse to raise MSRV in patch releases, that the same conflicting constraints could still occur.

I've spent my 2¢, thanks for reading.

[SimonSapin]

This will end up breaking builds for any crate whose dependency graph includes both idna 0.2.1 and some crate with a dependency like unicode-normalization = "0.1.10", since these now have conflicting constraints.

Wouldn’t they just get two copies of unicode-normalization? It’s a private dependency of idna, so there won’t be type-level errors.

I agree that such a patch fix (idna 0.21 constrained) is only a temporary solution,

Temporary until what?

[dekellum]

Private dependency is good for the situation. I just mean that at some point you will want to upgrade to latest unicode_normalization and thus MSRV 1.36. Once releases of url (≥ 2.2.0 is sufficient for the MSRV increase, IMO) and idna (0.3.0 or 1.0.0), then any conflict issues (or duplicates) can be fixed by upgrade downstream. Said another way, it seems to me there is little risk in attempting to fix this by constraining the dep in an idna patch release.

[dekellum]

It would be better to version Cargo.lock and also have a CI job that runs with the latest dependencies and latest toolchain.

I agree with this also. It's exactly what I do with all my crates (lib or bin). But MSRV bumps in patch releases still break the CI jobs with latest dependencies, so I still need to constrain those or increase my own MSRV (as you are trying to deal with here.) If I were to allow those failures my MSRV claims would not be upheld.

[SimonSapin]

≥ 2.2.0 is sufficient for the MSRV increase, IMO

Why 2.2.0? The next version number that is not backward-compatible in Semantic Versioning is 3.0.0. If we decide like unicode_normalization that MSRV increase is not considered a breaking that need to be reflected in the version number, then 2.0.1 and 2.74.13 are the same as far as SemVer-compat and Cargo are concerned.

[dekellum]

Other's have suggested bumping MINOR for MSRV and I agree and apply that approach, as pragmatic and clearly superior to the status quo of 0.x PATCH release MSRV bumps. I view rustc as conceptually similar to other dependencies where I would also tend to make MINOR bumps for MINOR dependency upgrades.

[dekellum]

Also, 2.2.0 is not the same as 2.1.1 (you've released url 2.1.0), in the sense that 2.2.0 enables backporting truely PATCH safe changes (e.g. bug fixes) to 2.1.1, etc. For certain dependencies I do specify constraints like version =">=3.1.0, < 3.3.0" in Cargo.toml, for example to deal with MSRV issues!

Another also: You wouldn't want to release a dependency upgrade to idna 0.3.0 in a url 2.1.1, IMO.

[mbrubeck]

Wouldn’t they just get two copies of unicode-normalization? It’s a private dependency of idna, so there won’t be type-level errors.

Cargo refuses to include two versions that it considers "compatible" (i.e., their first non-zero component is equal) in the same dependency graph.

[SimonSapin]

Oh. TIL.

Somehow I was convinced it would be just fine with having both versions, and that duplicate versions usually having "incompatible" numbers was only because most dependency (implicitly) use caret contraints. But:

error: failed to select a version for `url`.
    ... required by package `c v0.1.0 (/tmp/a/c)`
    ... which is depended on by `a v0.1.0 (/tmp/a)`
versions that meet the requirements `= 1.6.0` are: 1.6.0

all possible versions conflict with previously selected packages.

  previously selected package `url v1.5.0`
    ... which is depended on by `b v0.1.0 (/tmp/a/b)`
    ... which is depended on by `a v0.1.0 (/tmp/a)`

Not that blocking on changing Cargo would be a solution here, but I wonder why Cargo does this. Having this limitation in Cargo seems artificial and unhelpful to me.

[dekellum]

I would optimistically hope that rust-lang/rfcs#1977 could enable lifting that limitation when one side of conflict is private. Cargo tracking issue: rust-lang/cargo#6129

[valenting]

@SimonSapin how should we proceed here? I would like to land #537 next week, and a green CI badge would be preferable. 🙂

[SimonSapin]

I still feel that unicode_normalization could or should have handled things differently. But now that it’s done I’m inclined to declare 1.36.0 the new MSRV, stop testing with 1.33 at all (effectively adopting their policy), and call it a day. Thoughts?

[SimonSapin]

#573

[o0Ignition0o]

Totally agree, it is already broken and there unfortunately isn't much else we can do :/