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

0.5.0-rc2 break 0.5.0-rc1 #2166

Closed
Stargateur opened this issue May 9, 2022 · 13 comments
Closed

0.5.0-rc2 break 0.5.0-rc1 #2166

Stargateur opened this issue May 9, 2022 · 13 comments
Labels
triage A bug report being investigated

Comments

@Stargateur
Copy link

Stargateur commented May 9, 2022

0.5.0-rc2 make 0.5.0-rc1 not compile:

error[E0432]: unresolved import `crate::http::private::bind_tcp`
  --> C:\Users\Star\.cargo\registry\src\github.com-1ecc6299db9ec823\rocket-0.5.0-rc.1\src\server.rs:19:5
   |
19 | use crate::http::private::bind_tcp;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `bind_tcp` in `private`

error[E0432]: unresolved import `crate::http::private::tls`
   --> C:\Users\Star\.cargo\registry\src\github.com-1ecc6299db9ec823\rocket-0.5.0-rc.1\src\server.rs:371:39
    |
371 |             use crate::http::private::tls::bind_tls;
    |                                       ^^^ could not find `tls` in `private`

error[E0432]: unresolved import `crate::http::hyper::Bytes`
  --> C:\Users\Star\.cargo\registry\src\github.com-1ecc6299db9ec823\rocket-0.5.0-rc.1\src\ext.rs:13:5
   |
13 | use crate::http::hyper::Bytes;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^ no `Bytes` in `hyper`

error[E0407]: method `remote_addr` is not a member of trait `Connection`
   --> C:\Users\Star\.cargo\registry\src\github.com-1ecc6299db9ec823\rocket-0.5.0-rc.1\src\ext.rs:299:5
    |
299 | /     fn remote_addr(&self) -> Option<std::net::SocketAddr> {
300 | |         self.io.remote_addr()
301 | |     }
    | |_____^ not a member of trait `Connection`

error[E0412]: cannot find type `Bytes` in module `hyper`
  --> C:\Users\Star\.cargo\registry\src\github.com-1ecc6299db9ec823\rocket-0.5.0-rc.1\src\data\data_stream.rs:55:27
   |
55 |     Partial(Cursor<hyper::Bytes>),
   |                           ^^^^^ not found in `hyper`

Thus I fixed it by update my project to rc2 also cargo semver rule consider "0.5.0-rc2" a minor update of "0.5.0-rc1" that quite annoying since it's breaking change https://internals.rust-lang.org/t/changing-cargo-semver-compatibility-for-pre-releases/14820.

Repro cargo new my_project:

[dependencies.rocket]
version = "=0.5.0-rc.1"

(This issue is more to report this than to require any fix of this problem)

@Stargateur Stargateur added the triage A bug report being investigated label May 9, 2022
@SergioBenitez
Copy link
Member

Thank you for reporting this. Had we named the releases -rc1 and -rc2, instead of -rc.1 and -rc.2, would we have avoided this? This is an incredible wart in Cargo's semver policy. Unfortunately I think the ship has sailed on preventing this from happening, but thank you for shedding light into this issue. Should it cause significant problems, I will yank rc.2 in favor of -rc2.

@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

I have no idea if this will work but that a nice idea.

Also, just to be clear cause I think I mix up a little two issues together, the current code of version = "=0.5.0-rc.1" don't compile any more at all. I think there is a github dep somewhere that make the code of 0.5.0-rc.1 that was depending on it fail to compile. (or tell me if I'm wrong I may have fail somewhere)

@SergioBenitez
Copy link
Member

Oh, that's fascinating. I believe what's happened is that rocket 0.5.0-rc.1 depends on rocket_http 0.5.0-rc.1, which cargo now updates to 0.5.0-rc.2, which is of course not compatible. The easiest way to fix this is to pin the dependencies in your own Cargo.toml for rocket_http and any other affected crates. This is an unfortunate situation, however, so I will push a new release that cargo considers breaking and yank this one. Thank you again.

@SergioBenitez SergioBenitez reopened this May 9, 2022
@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

I suggest something similar to tokio versioning https://crates.io/crates/tokio/0.2.0-alpha.1

0.5.0-alpha.0
0.5.0-alpha.1 // no breaking change
0.5.0-alpha.2 // no breaking change
0.5.0-beta.0 // breaking change
0.5.0-beta.1 // no breaking change
0.5.0-gamma.0 // breaking change

// etc https://www.rapidtables.com/math/symbols/greek_alphabet.html

Should work no ? (I'm testing with https://crates.io/crates/semver I will update with result)

@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

Okay so, summary, there is no breaking change concept for prerelease, https://semver.org/#spec-item-11 it's just sorted so there is two solution:

  • use = to match exact version (this require the user to know this trap)
  • do it backward like:
0.5.0-z.0
0.5.0-z.1 // no breaking change
0.5.0-z.2 // no breaking change
0.5.0-y.0 // breaking change
0.5.0-y.1 // no breaking change
0.5.0-x.0 // breaking change

with semver = "1.0":

use semver::{BuildMetadata, Prerelease, Version, VersionReq};

fn main() {
    let req = VersionReq::parse("0.5.0-z.0").unwrap();
  let a = Version::parse("0.5.0-z.1").unwrap();
  let b = Version::parse("0.5.0-y.0").unwrap();
  assert!(req.matches(&a));
  assert!(!req.matches(&b));
}

And yank all previous pre-realease... I afraid such convention would make https://crates.io/crates/rocket/versions appear backward for "major" prerelease version. Also this doesn't solve the final problem that any prerelease version match 0.5.0. dtolnay/semver#236 (comment)

@SergioBenitez
Copy link
Member

SergioBenitez commented May 9, 2022

Right, so there's nothing we can do about Cargo updating a dep written as 0.5.0-rc.2 or 0.5.0-anything to `0.5.0', unfortunately, but we can:

  • fix rc.1 by yanking rc.2 and publishing rc2 instead.
  • potentially write =rc.1, =rc2 and so in in the docs in the future, though this might lead to lots of =0.5.0 when the general release is published, which would be unfortunate.

I'll start with the first point now.

@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

agree for yank but 1 will not work see:

use semver::{BuildMetadata, Prerelease, Version, VersionReq};

fn main() {
    let req = VersionReq::parse("0.5.0-rc.1").unwrap();
    let a = Version::parse("0.5.0-rc2").unwrap();

    assert!(req.matches(&a));
}

@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

I think the only ok solution is to make the use of = in all rocket internal prerelease dep, yank rc.2 (yank rc.1 too after I think) and make a prerelease that is not compatible with rc.1 so something like alpha.1:

use semver::{BuildMetadata, Prerelease, Version, VersionReq};

fn main() {
    let req = VersionReq::parse("0.5.0-rc.1").unwrap();
    let a = Version::parse("0.5.0-alpha.0").unwrap();

    assert!(!req.matches(&a));
}

This way the 0.5.0-rc.1 will still compile and user will just have to fix with a =0.5.0-rc.1 on their side (not strictly needed since rc.2 will be yank)

@SergioBenitez
Copy link
Member

SergioBenitez commented May 9, 2022

Isn't that the same problem but backwards? That is:

  • req 0.5.0-rc.1 doesn't match 0.5.0-alpha.0
  • but req 0.5.0-alpha.0 matches 0.5.0-rc.1 which is > 0.5.0-alpha.0

So if users write 0.5.0-alpha.0 in their Cargo.toml, they'll actually get rc.1.

@Stargateur
Copy link
Author

Stargateur commented May 9, 2022

yes but that why we should yank 0.5.0-rc.1 too and never release any prerelease that match 0.5.0-rc.1 requirement. (thus the final 0.5.0 will, time will have past soo.... user would have updated to recent rc since, anyway there is nothing we can do for this (AFAIK))

never mind we could release a 0.6.0 instead of 0.5.0:

use semver::{BuildMetadata, Prerelease, Version, VersionReq};

fn main() {
    let req = VersionReq::parse("0.5.0-rc.1").unwrap();
    let a = Version::parse("0.6.0").unwrap();

    assert!(!req.matches(&a));
}

@SergioBenitez
Copy link
Member

SergioBenitez commented May 10, 2022

Right, that's the only alternative I see (a 0.6.0). But we'd need to do that for every single pre-release, and I'm not in favor of being forced to release a major breaking version for every pre-release.

I think Cargo's approach makes little sense here and unfortunately, since we didn't specify = in the first place, there's little to nothing reasonable that we can do now. We'd need to yank rc.2 and release 0.6.0 or 0.6.0-rc or whatever, which seems like a bit of a stretch.

I'll close this issue while completely agreeing that we've done the wrong thing - I just see no reasonable recourse. On the bright side, rc.2 will see a lot more testing. And as long as cargo update isn't run, Cargo.lock should keep existing builds working.

@Stargateur
Copy link
Author

fine by me

@Stargateur
Copy link
Author

BTW made a RFC rust-lang/rfcs#3263 I would be interest by any review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage A bug report being investigated
Projects
None yet
Development

No branches or pull requests

2 participants