Replace `winapi` with `windows-sys` #712

hotate29 · 2022-06-16T15:45:22Z

Thanks for contributing to chrono!

Have you added yourself and the change to the changelog? (Don't worry
about adding the PR number)
If this pull request fixes a bug, does it add a test that verifies that
we can't reintroduce it?

hotate29 · 2022-06-16T15:50:27Z

This change brings MSRV to (maybe) 1.35.0. For the time being, wait for CI.

esheppa · 2022-06-17T02:01:49Z

Hi @hotate29 - thanks for this PR. If possible could you do the following:

Update the CI yaml to use 1.46 so we can ensure the CI does pass
Add an empty feature called "winapi" to avoid potential breakage (as per here)

As far as the increase in MSRV we may be better off waiting for an 0.5 release. It looks like debian stretch is at version 1.41 but I don't have a good understanding of where might be currently using version 1.32 and also packaging chrono or packages that depend on chrono. Also from a quick investigation windows-rs lists the rust-version as 1.46 which I think will likely be too much of an increase in the short term.

A solution in the meantime could be instead having a feature like "use-windows-rs" which would use "windows-rs" instead of "winapi" - however as per this issue I don't think this is currently possible.

Please let me know your thoughts?

hotate29 · 2022-06-17T15:22:57Z

Update the CI yaml to use 1.46 so we can ensure the CI does pass

Add an empty feature called "winapi" to avoid potential breakage (as per here)

Both done.

As far as the increase in MSRV we may be better off waiting for an 0.5 release. It looks like debian stretch is at version 1.41 but I don't have a good understanding of where might be currently using version 1.32 and also packaging chrono or packages that depend on chrono. Also from a quick investigation windows-rs lists the rust-version as 1.46 which I think will likely be too much of an increase in the short term

Ah, I completely forgot to check Cargo.toml in windows-sys...
As you pointed out, it may be abrupt to suddenly change MSRV to 1.46.0.

A solution in the meantime could be instead having a feature like "use-windows-rs" which would use "windows-rs" instead of "winapi" - however as per rust-lang/cargo#1839 I don't think this is currently possible.

I did not know about this issue. (Thanks for pointing it out!) Since there is no need to immediately switch to windows-sys, it may be safe to put this PR aside for now until there is more activity regarding version 0.5. (or close PR)

djc · 2022-06-17T20:27:56Z

Would like to keep this open -- I tagged with the API-incompatible tag so we can easily find such changes in the future.

hotate29 · 2022-06-18T08:25:05Z

Update the CI yaml to use 1.46 so we can ensure the CI does pass

@esheppa Do you mean to replace the part using 1.32 with 1.46?

esheppa · 2022-06-18T11:39:13Z

@hotate29 - yes in the .github/workflows/test.yml. It's up to you whether you wan't to do this as it might be a while before it gets merged, but its nice to leave it in a green state for when it gets picked up at the point that the MSRV can increase to >= 1.46

hotate29 · 2022-06-18T13:00:55Z

@esheppa I understand. thanks!

kennykerr · 2022-09-06T15:27:03Z

Curious, what's holding this change up? Let me know if I can help.

djc · 2022-09-06T16:02:25Z

@kennykerr what's the MSRV for the windows crates these days? We recently bumped to 1.38 but are being conservative about bumping more (probably the upper bound we'd consider is 1.48 at this point).

kennykerr · 2022-09-06T16:08:24Z

The MSRV for windows-sys 0.36 is 1.46.

parking_lot has already moved to 1.49 so I would recommend that version if you're planning to switch. The next version of windows-sys will likely require 1.49 as well.

djc · 2022-09-06T20:31:39Z

I think the limitation here is stable Debian, which IMO still affects a sizable contingent of downstream users. Anyway -- I'm generally interested in moving to windows-sys but this is why it's stalled. If you want to be lower in the stack than chrono, I think you should maybe be more conservative about MSRV bumps as well? 🙂

From my perspective (admittedly as a user of macOS on the desktop and Linux on servers -- and for other people I work with either at my day job or in OSS, it seems like Windows is a minority) I don't see a big benefit to switching away from winapi (and it's just a few small API calls, too) whereas I feel a substantial amount of responsibility towards downstream projects who want to serve users which have older compilers.

kennykerr · 2022-11-18T17:37:28Z

Sorry for the delay - I lost track of this issue.

probably the upper bound we'd consider is 1.48 at this point

I just tested 1.48 and that works great: microsoft/windows-rs#2173

I'm happy to enforce that. As a practical matter, the current version of windows-sys works fine with 1.48 if you'd like to go ahead with that.

esheppa · 2022-11-19T11:33:56Z

Thanks for this @kennykerr.

@djc - what are your thoughts on using this in 0.5 with a MSRV of 1.48?

djc · 2022-11-20T19:59:37Z

Yes, that seems fine. @hotate29 are you interested in rebasing this and updating it to bump the MSRV to 1.48?

hotate29 · 2022-11-21T10:01:51Z

@djc Yes, I am interested. I will work on that.

hotate29 · 2022-11-21T10:40:05Z

I have added an empty feature for compatibility (e65acd1), but if this change is included in the 0.5 release, I don't think it is needed.
Should I remove it?

djc · 2022-11-21T16:13:58Z

Yes, please remove the empty feature.

Would you mind rebasing this so that there are two commits:

One to bump the MSRV and deal with all the fallout (clippy fixes etc)
Another separate one that actually replaces winapi

Can also do two separate PRs if that's easier for you (then we can squash while merging).

This would make it easier for me to review the changes.

hotate29 · 2022-11-21T17:00:13Z

I rebased.

hotate29 · 2022-11-21T17:13:29Z

Oops, found a mistake.

hotate29 · 2022-11-21T17:36:39Z

Fixed.

djc

Looks great, one small issue to address, please fix it in the originating commit?

djc · 2022-11-21T19:42:37Z

Cargo.toml

@@ -11,6 +11,7 @@ readme = "README.md"
 license = "MIT/Apache-2.0"
 exclude = ["/ci/*"]
 edition = "2018"
+rust = "1.48.0"


This is supposed to be rust-version and that only starts working after 1.56, so I don't think we should add it here.

Deleted. thanks!

So please squash this into the first commit?

esheppa

Thanks for this @hotate29 - apologies to ask you to rebase this again, but we have recently merged the past while of work from 0.4.x to main and if you can rebase this on the HEAD of main that would clean up the diff in this PR. (Separately if you could use git rebase -i to move 288a5a6 into ef3fe078 that would be great)

hotate29 · 2022-11-23T10:26:23Z

@esheppa Rebased.

esheppa

Thanks @hotate29 - there are just a few more changes I'd like you to take a look at if possible

esheppa · 2022-11-23T10:42:30Z

src/offset/local/windows.rs

@@ -36,7 +37,7 @@ pub(super) fn naive_to_local(d: &NaiveDateTime, local: bool) -> LocalResult<Date
        tm_yday: 0,                // and this
        tm_isdst: -1,
        // This seems pretty fake?
-        tm_utcoff: if local { 1 } else { 0 },
+        tm_utcoff: local as i32,


do you mind using i32::from(local) here as per line 223 below?

esheppa · 2022-11-23T10:45:17Z

src/offset/local/windows.rs

        tm.tm_hour as u32,
        tm.tm_min as u32,
        tm.tm_sec as u32,
        tm.tm_nsec as u32,
-    );
+    )
+    .expect("invalid time");


this could be cast to a LocalResult::None which would avoid the expect call (this is semantically sub-optimal, but is consistent with usage elsewhere):

let time = NaiveTime::from_hms_nano_opt(...).ok_or(LocalResult::None)?;

Pushed changes (will squash later).

hotate29 · 2022-11-23T15:23:10Z

src/offset/local/windows.rs

+    let datetime = tm_to_datetime(Timespec::now().local());
+    datetime.single().expect("invalid time")


An expectation is that the system will not return an invalid time.

This looks reasonable, in the case of now()

esheppa · 2022-11-26T10:51:54Z

Thanks for those fixes @hotate29 - if possible can you squash them down to the original two commits? We are using "Rebase and Merge" rather than "Squash and Merge" which is why we have to be a bit more picky about keeping all the commits within a given PR clean (I'll set up some docs on this to make it an explicit policy)

edit:
I've created #898 for this, mostly as a reminder to myself

hotate29 · 2022-11-26T11:25:38Z

@esheppa Squashed.

esheppa

Thanks for this @hotate29, looks good to go!

hotate29 · 2022-11-28T14:50:27Z

Thanks to all who worked on this PR!

hotate29 changed the title ~~Replace winapi~~ Replace winapi with windows-sys Jun 16, 2022

hotate29 force-pushed the replace-winapi branch from ef33567 to 3c94bd7 Compare June 17, 2022 15:22

hotate29 force-pushed the replace-winapi branch 2 times, most recently from 2911b5c to d57099c Compare June 17, 2022 15:32

djc added the API-incompatible Tracking changes that need incompatible API revisions label Jun 17, 2022

hotate29 force-pushed the replace-winapi branch from d57099c to c283218 Compare June 18, 2022 13:14

kennykerr mentioned this pull request Nov 18, 2022

Test lower MSRV for windows-sys microsoft/windows-rs#2173

Merged

esheppa mentioned this pull request Nov 19, 2022

Consider refactoring chrono to avoid systematic misuse of panics #815

Closed

hotate29 force-pushed the replace-winapi branch from c283218 to aa19a98 Compare November 21, 2022 15:51

hotate29 force-pushed the replace-winapi branch 2 times, most recently from 43bf05e to e0088e1 Compare November 21, 2022 16:46

hotate29 force-pushed the replace-winapi branch from e0088e1 to dc3788e Compare November 21, 2022 17:15

djc approved these changes Nov 21, 2022

View reviewed changes

djc requested a review from esheppa November 21, 2022 19:44

hotate29 force-pushed the replace-winapi branch from b0a52bc to 288a5a6 Compare November 22, 2022 10:09

esheppa approved these changes Nov 23, 2022

View reviewed changes

hotate29 force-pushed the replace-winapi branch 2 times, most recently from 3e1d064 to 313d125 Compare November 23, 2022 10:00

esheppa requested changes Nov 23, 2022

View reviewed changes

hotate29 commented Nov 23, 2022

View reviewed changes

hotate29 force-pushed the replace-winapi branch 2 times, most recently from 823ecac to 221de6e Compare November 25, 2022 15:57

hotate29 added 2 commits November 26, 2022 20:15

Bump MSRV to 1.48.0 and fix clippy warnings

bc42e83

Replace winapi with windows-sys

b3e9664

hotate29 force-pushed the replace-winapi branch from 221de6e to b3e9664 Compare November 26, 2022 11:16

esheppa approved these changes Nov 26, 2022

View reviewed changes

djc merged commit 74ca661 into chronotope:main Nov 28, 2022

hotate29 deleted the replace-winapi branch November 28, 2022 14:55

pitdicker mentioned this pull request Jul 21, 2023

Switch to windows-sys on 0.4.x branch #1190

Closed

		let datetime = tm_to_datetime(Timespec::now().local());
		datetime.single().expect("invalid time")

Replace winapi with windows-sys #712

Replace winapi with windows-sys #712

Conversation

hotate29 commented Jun 16, 2022

Thanks for contributing to chrono!

hotate29 commented Jun 16, 2022 • edited

esheppa commented Jun 17, 2022

hotate29 commented Jun 17, 2022

djc commented Jun 17, 2022

hotate29 commented Jun 18, 2022 • edited

esheppa commented Jun 18, 2022

hotate29 commented Jun 18, 2022

kennykerr commented Sep 6, 2022

djc commented Sep 6, 2022

kennykerr commented Sep 6, 2022

djc commented Sep 6, 2022

kennykerr commented Nov 18, 2022

esheppa commented Nov 19, 2022

djc commented Nov 20, 2022 • edited

hotate29 commented Nov 21, 2022

hotate29 commented Nov 21, 2022

djc commented Nov 21, 2022

hotate29 commented Nov 21, 2022

hotate29 commented Nov 21, 2022

hotate29 commented Nov 21, 2022

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esheppa left a comment

Choose a reason for hiding this comment

hotate29 commented Nov 23, 2022

esheppa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esheppa commented Nov 26, 2022 • edited

hotate29 commented Nov 26, 2022

esheppa left a comment

Choose a reason for hiding this comment

hotate29 commented Nov 28, 2022

Replace `winapi` with `windows-sys` #712

Replace `winapi` with `windows-sys` #712

hotate29 commented Jun 16, 2022 •

edited

hotate29 commented Jun 18, 2022 •

edited

djc commented Nov 20, 2022 •

edited

esheppa commented Nov 26, 2022 •

edited