servo / unicode-bidi

Implementation of the Unicode Bidirection Algorithm in Rust
Other
74 stars 34 forks source link

Semantic versioning breakage in 3.4 #46

Closed neithernut closed 7 years ago

neithernut commented 7 years ago

Version 3.4 uses features which were unstable in rust-1.16. Older versions appear to build fine with rust-1.16. This means unicode-bidi is not abiding semantic versioning strictly, since an upgrade of the library introduces build failures although the major version was not bumped and nothing else in the build environment changed.

I guess this detail is generally not considered in the rust community. Sadly, due to how cargo works, this also forces depending packages to break their contracts with regard to compatibility.

This is pretty much a wont-fix, but I suggest making the CI also build this project with some fixed versions of the compiler rather than with only the current stable, beta and unstable versions of rust.

neithernut commented 7 years ago

I just realized stupid, tired me misread the version number and this is still pre-1.0.

behnam commented 7 years ago

We track the minimum-rustc-version-support in the CI file here: https://github.com/servo/unicode-bidi/blob/master/.travis.yml#L10

We're at rustc:1.17.0 at the moment.

apoelstra commented 7 years ago

My build was also broken by this, it is a dependency of the idna crate which is a dep of the post-1.0 url crate.

behnam commented 7 years ago

unicode-bidi was updated after url/idna got a bump: https://github.com/servo/rust-url/commit/52699f746ac4123d5d6a0fa1cebe22d116abcef5

Unfortunately, I believe there's no common best practice about version bumps on min-lang-version-needed updates. If there's guideline, we would be glad to follow. But looks minor bumps are a the common practice at the moment.

Please see also discussion on the PR: https://github.com/servo/unicode-bidi/pull/41

/cc @rillian

rillian commented 7 years ago

It's an interesting question, whether min-rust-version should be part of semver. Nothing-ever-breaks argues "yes", Rust's presumptive moving wave of stabilization-with-backward-compatibility argues "no".

I think "no" makes more sense, at least until tooling evolves to the point where it can offer semver guidance. It's hard enough to keep track of API stability. I don't think checking no new language features have crept in anywhere in the dependency chain is feasible right now, and most people are using at least the latest stable.

apoelstra commented 7 years ago

It's an interesting question, whether min-rust-version should be part of semver. Nothing-ever-breaks argues "yes",

The point of semver is that things don't break in minor point releases.

Rust's presumptive moving wave of stabilization-with-backward-compatibility argues "no".

This is a non-sequitor. Whatever guarantees the language is supposed to provide, crates shouldn't break in minor point releases.

anowell commented 7 years ago

As a transitive dependency (through url 1.5.1), this broke compilation for all (though still small in number) rust users on our platform.

It's an interesting question, whether min-rust-version should be part of semver. Nothing-ever-breaks argues "yes", Rust's presumptive moving wave of stabilization-with-backward-compatibility argues "no".

I find myself split between those. As an individual tracking stable (and nightly), "no" makes sense for almost everything I build (and I appreciate this as a crate author). But maintaining a platform that exposes rust to our users and that tends to lag stable by a few months (1.18 is queued up but blocked by unrelated-to-rust issues), this is the 2nd major ecosystem crate to suddenly break our platform for rust users. It seems clear at this point that we should generate projects with a known-working Cargo.lock (and be less aggressive in updating deps), but in the short-term, it seems I need to update our client to lock unicode-bidi to =0.3.3.

Manishearth commented 7 years ago

I don't see how this is relevant here.

unicode-bidi is pre-1.0, which means each version bump may contain a breaking change. unicode-bidi didn't do anything wrong here, this is following semver.

The problem here is that the url crate bumped it without realizing. This is a common problem, where a child crate makes a breaking version bump and the parent crate doesn't realize it's a bump that will affect others. Transitive dependency management is pretty hard wrt semver , and that's a rather intrinsic problem to the system.

anowell commented 7 years ago

pre-1.0, which means each version bump may contain a breaking change

I agree per strict semver, but I'd argue cargo's behavior makes it such that pre-1.0 breaking changes should generally be minor version bumps, and most crates seem to treat it that way - but I'm not trying to make this a black or white issue, as you put it, "transitive dependency management is pretty hard".

The problem here is that the url crate bumped it without realizing

Unless I'm missing something, that is not correct. url has depended on idna ^0.1.0 for several months, and idna on unicode-bidi ^0.3.0 for basically 2 months at this point.

But my intent here isn't to criticize unicode-bidi (and I apologize if my tone indicated otherwise). Rather I wanted to share an example of where a decision made by this crate caused breakage so that we (I have also caused breakage in my own crates) as an ecosystem can be more mindful of the impact of breaking changes, and perhaps it will inform future improvements to cargo/rustc around how to make these less common. Read the icebox commit message above - I would rather that not become a common reaction in the rust ecosystem; that is my real goal here.

I am extremely grateful crates like unicode-bidi - even if I didn't know it existed until I saw it fail to compile.

Manishearth commented 7 years ago

Ah, right.

Yeah, that's fair. It's pretty hard to keep track of all versioning things here and we made a mistake. This stuff happens, and we'll try to be more careful in the future.

Manishearth commented 7 years ago

Ultimately I don't like setting a precedent that all compiler upgrades should require a major semver bump. This is nearly impossible to support, and effectively means that crates get stuck on older versions for way too long. You can try to avoid it as much as possible, but that's about it; there's nothing stopping new code from being written that would not work on an older rust versions. Some crates test on older versions to prevent this; but then how far back do you go?

Because backwards compatibility is not an issue in Rust (or, it's not supposed to be), we shouldn't have to worry about forwards compatibility that much. Because it is supposed to be effortless to upgrade compilers, it may mean that a crate contains a change that needs an upgrade, and you should do it at that stage.

For custom setups with pinned rustc this doesn't work as well, so yeah, we should still try to avoid it, but these are special cases anyway. Heck, Firefox does this and we've had issues with it before, but ultimately we're violating the assumption that a compiler upgrade is easy.

I really think this discussion should be moved to users.rust-lang.org. I feel like it's an intractable problem in general, but let's see what the rest of the community things.

neithernut commented 7 years ago

Ultimately I don't like setting a precedent that all compiler upgrades should require a major semver bump.

Well, compiler versions typically don't. But language versions do. They are just quite intermingled in the case of rust.

Manishearth commented 7 years ago

Yeah, language version upgrades is what I meant.

With the Rust epoch stuff happening this may become clearer.

cardoe commented 6 years ago

For custom setups with pinned rustc this doesn't work as well, so yeah, we should still try to avoid it, but these are special cases anyway. Heck, Firefox does this and we've had issues with it before, but ultimately we're violating the assumption that a compiler upgrade is easy.

@Manishearth I'm a bit late to the party but yes, ultimately making the assumption that you can just upgrade compilers breaks the semver promise that is aimed at by the crates.io versioning system. Don't forget that there's also two things that technically get upgraded, the compiler and the standard library. There's currently no way to depend on either. But there is rust-lang/rfcs#1133 for the later however.

cardoe commented 6 years ago

In fact the release of 0.3.4 was actually a semver break and should have been called 0.4.0 based on http://semver.org/ The release comments note that Unicode 10 was added which required changes to a test. This implies there was functionality added in a backwards compatible fashion which implies that the minor version should have received a bump since this was not just a bug fix release.

behnam commented 6 years ago

@cardoe,

The release comments note that Unicode 10 was added which required changes to a test. This implies there was functionality added in a backwards compatible fashion which implies that the minor version should have received a bump since this was not just a bug fix release.

Under unicode-bidi crate, there was no test data update, but only how tests were executed. (Under url crate, yes, there was a bunch of fixes for compatibility.) But, in general, that logic doesn't stand, because then every bug fix would be a behavior change and would need a breaking upgrade.

On the other hand, I agree that Unicode version upgrades should be more deliberate. It may not be necessary for some algorithms that are generally stable, like unicode-bidi, but could be much desired for other algorithms, like segmentation.

We have had some discussion about this exact matter here: https://users.rust-lang.org/t/introducing-unic-unicode-and-internationalization-crates-for-rust/11447


Anyway, I should note that this specific GH Issue is not about Unicode data update, but it's about the rustc dependency. I believe that's still an open question in general and varies project to project. IMHO, it's a conversation that should be taken to the users forum, since it's not specific to this module.