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

bump time to use winapi 0.3 #274

Merged
merged 2 commits into from
Aug 18, 2018
Merged

bump time to use winapi 0.3 #274

merged 2 commits into from
Aug 18, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Aug 17, 2018

This bumps the minimal acceptable versions in Cargo.toml to versions that build on modern rust. this gets chrono working with Cargos -Z minimal-versions. This is part of the process of seeing how hard this is for crates to use in preparation for getting it stabilized for use in CI, specifically upstreaming the changes required to get criterion working with it. It is easy to use if all of your dependencies support it, but much harder if trying to impose it on them.

@quodlibetor
Copy link
Contributor

Awesome, thanks!

Do you know exactly what was causing compilation failures before? I'm not sure if it would be worth preferring to modify chrono to work with winapi 0.2 instead, if possible, depending on the state of the ecosystem and how much work would be required to do that.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 18, 2018

Do you know exactly what was causing compilation failures before?

That is an excellent question. Gathering questions is one of the things needed before we consider stabilizing, so thank you.

I think of minimal-versions as a way of fuzzing lockfiles. The normal resolution algorithm makes sure that users get the latest and greatest dependencies in there lockfiles. This is great for many reasons, but is kinda just testing the happy path.

For example when chrono asks for time = "0.1.36" cargos normal algorithm will give you 0.1.40. If you lockfile means that you get 0.1.38 for some reason then cargos normal algorithm will give you 0.2.8 for winapi = 0.2.0. I.E. the normal algorithm ensures that we stay close to the happy path.

But minimal-versions tests the most diabolical lock file it can find. So when chrono asks for time = "0.1.36" minimal-versions will give you 0.1.36 and when it asks for winapi = 0.2.0 it will give it 0.2.0. But winapi <=0.2.5 does not build on modern rust.

In general I would upstream a change to times requirements on win api to say winapi = 0.2.6 instead of winapi = 0.2.0, but the currently mantend veriton of time has already updated to winapi = 0.3.0. So I made a PR here to use a veriton of time with that update.

Does any of that make sense?

@quodlibetor
Copy link
Contributor

Ah, yes, definitely. The key points seem to be:

  • winapi < 0.2.6 doesn't work on current rust
  • there is no version of time that depends on winapi ^0.2.6, only ^0.3.0 (I've confirmed this)

Since CI passed on windows I guess it's clear that winapi 0.3 works with rust 1.13, which means I don't have to make any difficult decisions about whether or not bumping winapi means we need to change our supported rust version.

It's sort of interesting, though, because chrono does still technically work with winapi 0.2.x.

Instead of bumping our time dependency could we add an explicit dependency on winapi >= 0.2.6,<0.4 so that we don't break builds that are, for some reason, depending on winapi 0.2?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 18, 2018

Yes we can add a synthetic explicit dependency, with some work vs optional = true, if that is how you would prefer to deal with it. Can you elaborate on how this could break builds for someone, I don't think I see it?

Also I just notest the --features rustc-serialize so I pushed a commit bumping the version of that to one that builds.

@quodlibetor
Copy link
Contributor

Can you elaborate on how this could break builds for someone, I don't think I see it?

winapi 0.3 has some breaking changes from winapi 0.2.x -- otherwise they would have just created a new winapi 0.2.x version -- so if developers are using an old version of rust with an old version of winapi that relies on the old API, bumping the required version of winapi will force them to change their usage.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 18, 2018

v0.2 and v0.3 can coexist in the same dependency tree. So if they depend on v0.2 then they will get v0.2, eavan if we as there dependent need/see v0.3.

@quodlibetor
Copy link
Contributor

Gah, right, it's not a public dependency. facepalm

@quodlibetor quodlibetor merged commit a648423 into chronotope:master Aug 18, 2018
@quodlibetor
Copy link
Contributor

Thank you!

@quodlibetor
Copy link
Contributor

And I appreciate the effort in getting minimal-versions working across the ecosystem, I really want it to be a standard testing mode!

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 24, 2018

Is there an ETA for a release that has this merged? I want to continue to spread minimal-versions to the ecosystem.

@quodlibetor
Copy link
Contributor

Yeah I'll get this out this weekend, should be tomorrow.

@quodlibetor
Copy link
Contributor

This is now out in 0.4.6

@Eh2406
Copy link
Contributor Author

Eh2406 commented Aug 25, 2018

Thanks! downstream PRs going in now.

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.

2 participants