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

Implement RFC 2052: Epoches #5011

Merged

Conversation

Manishearth
Copy link
Member

Todo:

  • Make epoches affect the fingerprint
  • Tests

cc rust-lang/rust#44581

Rust PR: rust-lang/rust#48014

r? @acrichto

@alexcrichton
Copy link
Member

Thanks! Could some tests be added as well? Something like:

  • A test to ensure it's unstable
  • A test for a malformed epoch key
  • A test that we pass the right epoch
  • A test that we unconditionally pass the epoch

@withoutboats
Copy link
Contributor

withoutboats commented Feb 6, 2018

I think it would be more user-friendly to call the field in the toml rust. While we've talked about the idea among contributors in terms of "epochs," I think in terms of marketing these releases we will not emphasize the "epoch" language. Among the broader user base and potential use base, this will just be known as "Rust 2018."

If/when we grow the ability to specify a particular toolchain version (e.g. rustc-1.25.0), that should be called toolchain.

[package]
name = "failure"
version = "1.2.0"
rust = "2018"
toolchain = "1.31.0"

#[derive(Clone, Copy, Debug, Hash, PartialOrd, Ord, Eq, PartialEq)]
pub enum Epoch {
/// The 2015 epoch
Epoch2015 = 2015,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cute, but isn't the only effect of this to make this enum a u16?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more there for forcing the PartialOrd to be the right order. But it's not necessary for Cargo; I just copied this definition from the Rust PR (where comparing epochs is more important)

@Manishearth
Copy link
Member Author

Yeah, was writing the tests on the flight. Pushing now, will add the other tests you mentioned too.

@Manishearth Manishearth force-pushed the stealing-chickens-off-the-internet branch from af7dd7c to ea76f99 Compare February 6, 2018 02:48
@Manishearth
Copy link
Member Author

Pushed up the tests.

Note that right now it demands a string key, epoch = "2015". epoch = 2015 will not work. I'm opting for a string in case in the future we wish to do unstable epoch release candidates via epoch = "2018-next" or whatever. Or even epoch = "2018-second" if for some reason we do two epochs in a year. However I'm not sure how to tell Serde to accept integer keys and turn them into strings.

Regarding the outward facing name of the feature, @aturon, thoughts?

(it's unstable so we don't need to decide this immediately, but there's also no rush on landing this anyway)

@Manishearth Manishearth force-pushed the stealing-chickens-off-the-internet branch from ea76f99 to 5d615a6 Compare February 6, 2018 03:02
@withoutboats
Copy link
Contributor

We tend to require strings even when integers could suffice; we don't accept a version number of 1 for example, it would have to be "1".

@Manishearth
Copy link
Member Author

wfm. Though I think not allowing 2015 would be a bit surprising

@alexcrichton
Copy link
Member

@Manishearth wanna switch to rust and we can land this on nightly?

@Manishearth
Copy link
Member Author

Done

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 6, 2018

📌 Commit 270f6e2 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 6, 2018

⌛ Testing commit 270f6e2 with merge 750df0a...

bors added a commit that referenced this pull request Feb 6, 2018
… r=alexcrichton

Implement RFC 2052: Epoches

Todo:

 - Make epoches affect the fingerprint
 - Tests

cc rust-lang/rust#44581

Rust PR: rust-lang/rust#48014

r? @acrichto
@bors
Copy link
Contributor

bors commented Feb 6, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 750df0a to master...

@bors bors merged commit 270f6e2 into rust-lang:master Feb 6, 2018

consider adding `cargo-features = [\"epoch\"]` to the manifest
")));
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing newline

@Manishearth Manishearth deleted the stealing-chickens-off-the-internet branch February 14, 2018 14:56
@ehuss ehuss added this to the 1.25.0 milestone Feb 6, 2022
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.

6 participants