Bump `windows-core` to `0.53-0.54` range #131

MarijnS95 · 2024-03-26T20:03:30Z

Depends on #161

This includes the necessary binding changes for compatibility, provided by windows-bindgen 0.53-0.54. Generated code by windows-bindgen now also expects TryInto to be in scope via Rust's prelude, which is only the case in Rust edition 2021.

Note that `windows 0.55` was not released, and any implementation in the wild depending on `windows` will get `windows-core 0.54` in their dependency tree for obvious compatibility reasons, and would get a duplicate if `iana-time-zone` decides to switch to o`windows-core 0.55` prematurely. This PR depends on strawlab#131 and can be updated and merged whenever there is a compelling reason to bump further. This requires a strange change to remove a "comment" from `bindings.txt`: Running `api_gen/target/debug/api_gen` Error: Error { message: "unused filters\n Dependencies\n above\n the\n of\n #", path: "", span: None }

MarijnS95 · 2024-03-26T20:11:53Z

Bumping the edition for having TryInto in the prelude will break MSRV, let's see if we can find a way around that.

MarijnS95 · 2024-06-28T22:10:52Z

@astraw I don't see any strategy as to what the policy is regarding MSRV bumps in https://github.com/strawlab/iana-time-zone?tab=readme-ov-file#minimum-supported-rust-version-policy, and it seems likely that an internal version bump for a newer crate that doesn't provide any useful/fixed functionality otherwise (besides getting rid of duplicate deps downstream) isn't very compelling to break MSRV for. Despite that, what are your thoughts? The requested version here for windows-core 0.53 is Rust 1.62.

Cargo.toml

lopopolo · 2025-03-23T19:59:32Z

Cargo.toml

@@ -26,7 +26,7 @@ android_system_properties = "0.1.5"
 core-foundation-sys = "0.8.6"

 [target.'cfg(target_os = "windows")'.dependencies]
-windows-core = { version = ">=0.50, <=0.52" }
+windows-core = { version = ">=0.53, <=0.54" }


are we able to lower the bound on this so we can get the crate building with Rust 1.61.0?

Let me do some experimentation to see how far we can stretch this bound without bumping the generator and hence requirements for MSRV/edition, and keep separate PRs open when we do end up bumping those.

0.53 already requires MSRV 1.62, the resulting compatibility issues with older windows-bindgen output don't even matter at that point.

lopopolo · 2025-03-24T02:03:48Z

whoops sorry, didn't mean to close this. #161 is merged and when I deleted the dev/lopopolo-2021-edition branch, this closed. those changes are in main now

lopopolo · 2025-03-24T02:13:55Z

@MarijnS95 Just to confirm my understanding of the current state:

This PR incrementally bumps windows-core to versions 0.53.0 and 0.54.0, which are now significantly behind the latest available release.
The specified version range (>=0.53, <=0.54) requires bumping the MSRV to Rust 1.62.0.
Supporting a broader range isn’t possible here due to breaking changes in the output of windows-bindgen, which is also upgraded as part of this PR.

Looking ahead to your other PR (#133):

It bumps windows-bindgen to v0.59.
It expands the supported windows-core version range to >=0.56, <=0.61, which includes the latest release.
It maintains an MSRV of Rust 1.62.0 (aligned with [email protected]).

Assuming I’ve summarized this correctly, this means that if we’re comfortable raising our MSRV by one version (to Rust 1.62.0), we can adopt a significantly more up-to-date version range of windows-core, covering both the newest version and compatibility with our MSRV.

Could you please confirm this assessment?

If this understanding is correct, the next step would be discussing with the other maintainers and the Chrono team whether this MSRV increase is acceptable.

Thanks again for your patience and for driving these improvements. I really appreciate your efforts here!

astraw · 2025-03-24T05:47:25Z

Whoops, now I apparently accidentally deleted the git branch associated this PR. Sorry! (Actually, I thought I was deleting the dev/lopopolo-2021-edition branch which was already merged into main.) This closed this PR and Github is not letting me simply reopen it. I hope @lopopolo or @MarijnS95 you have the branch locally and push it to either re-open the PR or, worst case, open a new PR.

Many thanks for your ongoing work on this - I really appreciate it. I apologize for this.

@MarijnS95 I also missed your question about the MSRV policy. My main thought is that increases up to chrono's level are likely fine and beyond that should ideally be done in consultation with the chrono team.

lopopolo · 2025-03-24T05:55:04Z

@astraw you can just push up the branch with the same name on current main and it will let you reopen the PR.

I'm used to work GitHub where stacking branches like this would automatically update the base of a PR to the primary branch on merge, but I guess that doesn't work with forks.

astraw · 2025-03-24T06:02:44Z

OK, I was able to restore the branch I deleted and the GitHub let me reopen the PR. Sorry for the noise. (I didn't anticipate that deleting the name of a merged branch would cause this. Now I know how GitHub keeps force push commits associated with the same PR.)

lopopolo · 2025-03-25T06:10:20Z

@astraw the bump we just landed to 1.61.0 matches Chrono. In order to move forward here, we have to bump again to 1.62.0.

This includes the necessary binding changes for compatibility, provided by `windows-bindgen 0.53-0.54`. Generated code by `windows-bindgen` now also expects `TryInto` to be in scope via Rust's `prelude`, which is only the case in Rust edition `2021`.

MarijnS95 · 2025-03-25T15:10:44Z

Looks like GitHub is just broken. It used to automatically reset the target branch to main again when a "random" target branch disappears 🙃

I've updated the target branch (hopefully maintainers can do that as well?) so it should once again be safe to remove dev/lopopolo-2021-edition.

@lopopolo your points above are all correct, expanding on these:

Supporting a broader range isn’t possible here due to breaking changes in the output of windows-bindgen, which is also upgraded as part of this PR.

Correct, both forwards and backwards.

It expands the supported windows-core version range to >=0.56, <=0.61, which includes the latest release.

Yes, and I don't think/remember if there was another combination that is useful to us (i.e. if we use windows-bindgen 0.58, can we use an even wider version range?). windows-bindgen 0.59 is a massive improvement over older versions and generates a lot less code.

I've pushed a preliminary MSRV bump to this PR just to see if the CI is happy with these claims (here and in #133), feel free to pull that in through a separate PR when upstream consumers like chrono are ready to update.

I didn't anticipate that deleting the name of a merged branch would cause this. Now I know how GitHub keeps force push commits associated with the same PR

Yeah a PR tracks a branch, wherever that branch ends up pointing to (and doesn't prune "unreachable" commits, since they're commonly still "reachable" from the UI here, like the force-pushed from X to Y event markers).

lopopolo · 2025-03-26T05:00:40Z

It looks like CI is passing with the MSRV bump. Thanks again @MarijnS95 for your efforts!

I’d like to move forward with this proposal, but recognize it might cause some friction for downstream consumers, so I want to ensure we’re fully aligned before proceeding.

Specifically, we’re proposing to raise this crate's tier 1 target MSRV from 1.61 to 1.62. This change enables support for a broader range of windows-core versions, including the latest releases. This update is particularly beneficial to downstream libraries heavily utilizing windows-bindgen, as it reduces duplicate dependencies in their lock files.

The update is also beneficial to us as maintainers of iana-time-zone since the updated bindings are smaller and we will have an easier time tracking upstream.

@djc, from Chrono’s standpoint, is it acceptable to include this MSRV increase in another 0.1.x release? Alternatively, would you prefer we mark this clearly by releasing it as version 0.2.0?

I’m open to feedback and eager to find a path forward that aligns with everyone’s needs.

complexspaces · 2025-03-26T05:23:25Z

Hey everyone 👋. I wanted to chime in as a downstream user of iana-time-zone (via chrono).

At work this is currently blocking us from cleanly migrating to the latest versions of windows and windows-core which we need to support new COM functionality because iana-time-zone is the one of the only third-party crates in our tree using something that isn't windows-sys or windows-bindgen. We would love to not introduce this duplication since it hurts compile times.

On my side as an OSS developer, I've worked around windows-* versioning conflicts in a few ways:

Allowing a wider range of windows-sys versions and using trait glue to handle source incompatibility (ref).
- At a glance, I don't know if this approach would work for iana-time-zone today because it uses WinRT APIs as opposed to Win32 C ones.
Use windows-bindgen with the minimal config option, which tells windows-bindgen to "vendor" types that would usually be provided via a windows-core import (ref)

If the MSRV arrangements here end up being problematic, I'd like to propose a different approach: Dropping the use of Windows.Globalization.Calendar and instead using C APIs that have been available since Windows 10 1703 (2016) per this blog on converting time zone formats on Windows. This would require supporting a 1 year newer version of Windows 10 as opposed to the initial release, but in exchange I believe it would let iana-time-zone drop its windows-core dependency in favor of using purely windows-bindgen-generated code instead, unblocking all of this.

Note that while in theory Rust 1.61-1.62 supports versions of Windows before 10, the current use of WinRT APIs in iana-time-zone today mean that Windows 10 is the minimum supported OS version of the crate anyway. 1703 is so old that I do not think this will realistically affect downstream users because its been EoL for so long.

djc · 2025-03-26T06:27:33Z

Bumping MSRV to 1.62 is fine at this point. Though, following through on @complexspaces points would be nice. (chrono itself already uses windows-bindgen too.)

astraw · 2025-03-27T07:17:04Z

Thanks for the feedback, everyone. As I rarely use Windows, and even more rarely program low-level Windows-specific stuff, I really depend on your input.

These 1k+ lines of autogenerated code, with lots of unsafe, "just" to get the timezone on Windows seem suboptimal. What's not clear to me is if any of the ideas here, either @MarijnS95's updated PRs (thanks!) or @complexspaces idea of using the timezoneapi.h C API and windows-bindgen, would get us to a much more minimal code base (even in the more distant future in case it cannot be done right now). @MarijnS95, what do you think of the C API suggestion?

From my side, bumping MSRV to 1.62 would be fine with me, too.

lopopolo · 2025-03-27T14:46:01Z

Merging this—thanks everyone for the thoughtful discussion and collaboration here!

I’ve opened #166 to continue the conversation around simplifying the Windows implementation and potentially dropping the windows-core dependency. Follow-ups and architectural ideas welcome there.

@MarijnS95 once this is in, I believe we can rebase #133 and land that pretty quickly. @astraw, I think we owe folks a release once all of these PRs are in.

MarijnS95 mentioned this pull request Mar 26, 2024

Update windows-core requirement from >=0.50, <=0.52 to >=0.50, <=0.53 #128

Closed

MarijnS95 mentioned this pull request Mar 26, 2024

Bump windows-core to 0.55 #132

Closed

Kijewski added dependencies Pull requests that update a dependency file Tier-1 Rust Tier-1 platform labels Aug 26, 2024

MarijnS95 force-pushed the windows-core-0.54 branch from a7a0907 to a478270 Compare January 13, 2025 13:21

MarijnS95 mentioned this pull request Jan 13, 2025

Bump windows-core to 0.56-0.61 range and regenerate with 0.59 #133

Merged

MarijnS95 force-pushed the windows-core-0.54 branch from a478270 to 06fe3ee Compare March 23, 2025 19:34

MarijnS95 mentioned this pull request Mar 23, 2025

Upgrade windows-core dependency #158

Closed

MarijnS95 force-pushed the windows-core-0.54 branch from 06fe3ee to 43cac50 Compare March 23, 2025 19:42

lopopolo assigned lopopolo and unassigned lopopolo Mar 23, 2025

lopopolo self-requested a review March 23, 2025 19:50

lopopolo reviewed Mar 23, 2025

View reviewed changes

Cargo.toml Show resolved Hide resolved

lopopolo reviewed Mar 23, 2025

View reviewed changes

MarijnS95 force-pushed the windows-core-0.54 branch from 43cac50 to c6ace10 Compare March 23, 2025 20:00

MarijnS95 changed the base branch from main to dev/lopopolo-2021-edition March 23, 2025 20:01

lopopolo deleted the branch strawlab:main March 24, 2025 01:59

lopopolo closed this Mar 24, 2025

lopopolo reopened this Mar 24, 2025

astraw deleted the branch strawlab:main March 24, 2025 05:25

astraw closed this Mar 24, 2025

astraw reopened this Mar 24, 2025

MarijnS95 force-pushed the windows-core-0.54 branch from c6ace10 to 88b8858 Compare March 25, 2025 14:45

MarijnS95 changed the base branch from dev/lopopolo-2021-edition to main March 25, 2025 14:46

MarijnS95 added 2 commits March 25, 2025 15:58

WIP: Bump MSRV to 1.62

5bad558

MarijnS95 force-pushed the windows-core-0.54 branch from 88b8858 to a26e540 Compare March 25, 2025 15:01

lopopolo mentioned this pull request Mar 27, 2025

Refactor Windows time zone support to reduce dependency on windows-core and autogenerated bindings #166

Open

lopopolo approved these changes Mar 27, 2025

View reviewed changes

lopopolo merged commit 10c843d into strawlab:main Mar 27, 2025
51 checks passed

MarijnS95 deleted the windows-core-0.54 branch March 27, 2025 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump `windows-core` to `0.53-0.54` range #131

Bump `windows-core` to `0.53-0.54` range #131

MarijnS95 commented Mar 26, 2024 •

edited

Loading

MarijnS95 commented Mar 26, 2024

MarijnS95 commented Jun 28, 2024 •

edited

Loading

lopopolo Mar 23, 2025

MarijnS95 Mar 23, 2025

MarijnS95 Mar 23, 2025 •

edited

Loading

lopopolo commented Mar 24, 2025

lopopolo commented Mar 24, 2025

astraw commented Mar 24, 2025

lopopolo commented Mar 24, 2025

astraw commented Mar 24, 2025

lopopolo commented Mar 25, 2025

MarijnS95 commented Mar 25, 2025 •

edited

Loading

lopopolo commented Mar 26, 2025

complexspaces commented Mar 26, 2025 •

edited

Loading

djc commented Mar 26, 2025

astraw commented Mar 27, 2025

lopopolo commented Mar 27, 2025

Bump windows-core to 0.53-0.54 range #131

Bump windows-core to 0.53-0.54 range #131

Conversation

MarijnS95 commented Mar 26, 2024 • edited Loading

MarijnS95 commented Mar 26, 2024

MarijnS95 commented Jun 28, 2024 • edited Loading

lopopolo Mar 23, 2025

Choose a reason for hiding this comment

MarijnS95 Mar 23, 2025

Choose a reason for hiding this comment

MarijnS95 Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

lopopolo commented Mar 24, 2025

lopopolo commented Mar 24, 2025

astraw commented Mar 24, 2025

lopopolo commented Mar 24, 2025

astraw commented Mar 24, 2025

lopopolo commented Mar 25, 2025

MarijnS95 commented Mar 25, 2025 • edited Loading

lopopolo commented Mar 26, 2025

complexspaces commented Mar 26, 2025 • edited Loading

djc commented Mar 26, 2025

astraw commented Mar 27, 2025

lopopolo commented Mar 27, 2025

Bump `windows-core` to `0.53-0.54` range #131

Bump `windows-core` to `0.53-0.54` range #131

MarijnS95 commented Mar 26, 2024 •

edited

Loading

MarijnS95 commented Jun 28, 2024 •

edited

Loading

MarijnS95 Mar 23, 2025 •

edited

Loading

MarijnS95 commented Mar 25, 2025 •

edited

Loading

complexspaces commented Mar 26, 2025 •

edited

Loading