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

use windows crate directly #97

Merged
merged 4 commits into from
Mar 15, 2023
Merged

use windows crate directly #97

merged 4 commits into from
Mar 15, 2023

Conversation

astraw
Copy link
Member

@astraw astraw commented Jan 30, 2023

This drops a lot of unsafe code on Windows. For now, this pulls in the windows crate using git. When this is released it we can update this PR.

See microsoft/windows-rs#2317 and microsoft/windows-rs#2318

Thanks to @Kijewski @kennykerr and @riverar

@astraw astraw added the Tier-1 Rust Tier-1 platform label Jan 30, 2023
@astraw astraw requested a review from Kijewski January 30, 2023 21:06
@riverar
Copy link

riverar commented Jan 30, 2023

Sweet! ✌️

@astraw
Copy link
Member Author

astraw commented Jan 30, 2023

(The failing tests are unrelated noise - they were already failing on a scheduled rebuild of the main branch. I guess it has something to do with the recently released rust 1.67, cross compilation, and freebsd not playing nicely together.)

src/tz_windows.rs Outdated Show resolved Hide resolved
@Kijewski
Copy link
Collaborator

Windows-rs 0.46 was released, and should be usable for us now. 🎉

Copy link
Collaborator

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

Looks good and I love dropping all of this unsafe. Thanks all!

@astraw astraw marked this pull request as ready for review March 15, 2023 06:52
@astraw astraw merged commit bfacc8c into main Mar 15, 2023
@astraw astraw deleted the new-win branch March 15, 2023 07:07
@kennykerr
Copy link

:shipit:

@astraw
Copy link
Member Author

astraw commented Mar 15, 2023

Thanks everyone, especially @kennykerr.

@MarijnS95
Copy link
Contributor

Looks like windows 0.47 already dropped: can we get a bump and release to that?

@Jake-Shadle this is also a prime candidate for windows-bindgen except for the use of COM objects, for which we're waiting on @kennykerr to separate manual code out into windows-core first; and then hope that crate releases less often if breaking changes are only in the metadata and generated output.

@astraw
Copy link
Member Author

astraw commented Mar 30, 2023

Looks like windows 0.47 already dropped: can we get a bump and release to that?

Done in v0.1.55

@MarijnS95
Copy link
Contributor

That's fast, thanks!

@MarijnS95
Copy link
Contributor

MarijnS95 commented Aug 18, 2023

Looks like windows 0.47 already dropped: can we get a bump and release to that?

@Jake-Shadle this is also a prime candidate for windows-bindgen except for the use of COM objects, for which we're waiting on @kennykerr to separate manual code out into windows-core first; and then hope that crate releases less often if breaking changes are only in the metadata and generated output.

I recently tried this now that windows-bindgen is capable of generating "minimal" bindings that leverage windows-core 0.51 rather than the full windows crate.

Unfortunately/obviously this doesn't suffice the MSRV 1.48 policy of this crate thanks to edition = "2021" rust-version = "1.56" in the windows-core crate.

Diff is here: https://github.com/MarijnS95/iana-time-zone/compare/windows-core

I generated the bindings without --config package because this crate really shouldn't have Windows features leaking into its [features] array (beyond those being appended at the end of Cargo.toml). Unfortunately this means the whole Windows.Globalization.Calendar type is generated including function signatures that depend on other types/modules, without cfg guards. All these extra types (and all their dependencies, etc) have to be included to make the Windows module compile; while users may end up not having to download the windows crate as a result, they may end up compiling more code that way.

Still appears to be a half-second (approx!) win though, running:

cargo clean && cargo b --timing

Before:
afbeelding

After:
afbeelding

CC @kennykerr, you might be interested in this. Note also that the compiler suggested that Windows.Foundation.DateTime and Windows.Foundation.TimeSpan could have been imported from windows_core::imp::*, should/could bindgen take care of that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-1 Rust Tier-1 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants