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

request for guidance: accessing GetTimeZone in iana-time-zone crate #2317

Closed
astraw opened this issue Jan 29, 2023 · 18 comments · Fixed by #2318
Closed

request for guidance: accessing GetTimeZone in iana-time-zone crate #2317

astraw opened this issue Jan 29, 2023 · 18 comments · Fixed by #2318
Labels
enhancement New feature or request

Comments

@astraw
Copy link
Contributor

astraw commented Jan 29, 2023

Hi, I am a developer of the iana-time-zone crate. I would like to ask for advice/help from the experts about how to best support Windows moving forward. At the high level, our goal seems very simple. We want to call the Windows.Globalization Calendar.GetTimeZone function from Rust. As our crate currently gets over 100k downloads most days and we are contemplating a substantial rewrite of our Windows specific code - with lots of unsafe - I would like to ask for help here.

Narrowly, the subject at hand is strawlab/iana-time-zone#96, a PR which drops windows-sys in favor of windows-targets and contains plenty of unsafe. As I am no expert in the relevant Windows APIs, I have a difficult time deeply evaluating this PR. So, at the shallowest level, it would be great if we could get some eyes on that PR for better review. It seems that #2165 removed the APIs we were using from windows-sys 0.45.

More broadly, I would ask if there is a chance that the relevant APIs will get added back to windows-sys? If not, should we consider that the correct thing to do is indeed to use windows-targets?

The following is perhaps a question for a different venue, but also relevant in my mind to the issues at hand: why are crates such as tokio and chrono NOT depending on the high-level windows crate but instead on the low-level windows-sys crate? A long time ago, iana-time-zone depended on the windows crate (version 0.7) and our implementation was simple and used no unsafe. It seems to me like we are going backwards by introducing so much low-level unsafe-using code. As we are a dependency now of the very popular chrono, I do not want to introduce a dependency which they themselves do not have.

My overall fear is that we are going backwards here - keeping or adding substantial complexity (which we also have to maintain). Would it be possible to drop all this low-level stuff and let the experts (i.e. the developers of windows-rs here) handle the relevant details? That would be my goal.

Thanks for your work on windows-rs and thanks in advance for help here!

@kennykerr
Copy link
Collaborator

I think you may be confusing windows and windows-sys. Here's an explainer:

https://kennykerr.ca/rust-getting-started/windows-or-windows-sys.html

Basically, the windows-sys crate has never included the Windows.Globalization API because it is a WinRT API. The windows-sys crate excludes all COM and WinRT APIs.

The windows crate continues to provide the Windows.Globalization API including Calendar::GetTimeZone as you can see in the docs here:

https://microsoft.github.io/windows-docs-rs/doc/windows/Globalization/struct.Calendar.html#method.GetTimeZone

So you would certainly avoid a ton of unsafe code by using the windows crate to call this API.

@kennykerr kennykerr added the question Further information is requested label Jan 29, 2023
@astraw
Copy link
Contributor Author

astraw commented Jan 29, 2023

Practically speaking, how would you suggest calling Windows.Globalization with Rust 1.48 (our MSRV)? Something like proposed in strawlab/iana-time-zone#96 ?

Our minimum supported rust version of 1.48 comes from tracking this with chrono. This requirement seems to rule out the windows crate itself, at least the latest release as of today. (As I wrote, version 0.7, for example, did work.)

@riverar
Copy link
Collaborator

riverar commented Jan 29, 2023

I glanced at the code and it seems all you really need is a Windows.Globalization.Calendar.GetTimeZone equivalent.

How about simply calling out to the platform provided ICU (via windows-sys)?

Something like...

[dependencies.windows-sys]
version = "0.42.0"
features = [
    "Win32_Globalization"
]
use windows_sys::Win32::Globalization::{ucal_getDefaultTimeZone, U_ZERO_ERROR};

fn main() {
    let mut status = U_ZERO_ERROR;
    let buffer_size = unsafe { ucal_getDefaultTimeZone(std::ptr::null_mut(), 0, &mut status) };

    let mut buffer: Vec<u16> = Vec::new();
    buffer.resize((buffer_size + 1) as _, 0);

    status = U_ZERO_ERROR;
    unsafe {
        ucal_getDefaultTimeZone(buffer.as_mut_ptr(), buffer.capacity() as _, &mut status);
    }

    println!("{}", String::from_utf16_lossy(&buffer));
}
> America/Los_Angeles

@Kijewski
Copy link

@riverar, thank you! That looks exactly like the right function to use.

The only problem is that is the function was only introduced in Windows 10 v1709, which is newer than the Windows 10 v1607, which is not EOL for the next 3 years and 8 month. I know too little about Windows' update policy to know if such functions get backported. Windows 8.1's EOL was earlier this month, so at least this one won't be a problem.

What do you think? Can we already ship a library that uses this "new" function, or is it still too early for that?

@riverar
Copy link
Collaborator

riverar commented Jan 29, 2023

Hmm Windows 10, version 1607 for Education, Enterprise, and IoT Enterprise reached EOL April 9, 2019

Not sure if that third-party site is correct?

@Kijewski
Copy link

The Windows Knowledge Base seems to support that date: When does Microsoft Support end for Windows 10 Enterprise 2016 LTSB 1607?:

The extended support end date for Windows 10 Enterprise 2016 LTSB is 13 October 2026. During the 'extended support' period Microsoft will continue to release security updates and non-security updates, but will not respond to requests to change product design or features

But I'm not an "enterprise user", so maybe I am misinterpreting something.

@riverar
Copy link
Collaborator

riverar commented Jan 29, 2023

Ah Windows 10 LTSB 1607 is on the fixed lifecycle. https://learn.microsoft.com/en-us/lifecycle/products/windows-10-2016-ltsb

Let me take a peek at that build.

@riverar
Copy link
Collaborator

riverar commented Jan 30, 2023

Yeah to support LTSB 1607, you'll need to either consume the timezone mapping file on disk or call into the WinRT implementation.

So perhaps it's best to simply abandon strawlab/iana-time-zone#96 and leave it as-is? Looks solid; what's the downside?

@astraw
Copy link
Contributor Author

astraw commented Jan 30, 2023

The motivating factor for the PR was to drop a no-longer current dependency. (The current implementation in git main depends on windows-sys 0.42 and no longer works in the recently released 0.45.)

If we're going to accept no-longer current dependencies: why not then depend on the oldest windows which still supports our MSRV? This comes with the advantage that we would have no unsafe in our Windows implementation.

@astraw
Copy link
Contributor Author

astraw commented Jan 30, 2023

Can you link to information about the timezone mapping file? That option sounds potentially appealing. After a couple minutes of searching, I still haven't found anything that looks relevant. (But again, I am far from expert with Windows.)

@riverar
Copy link
Collaborator

riverar commented Jan 30, 2023

What breaks when you move to windows-sys 0.45? (We already established the listed reason of WinRT APIs disappearing is invalid.)

@riverar
Copy link
Collaborator

riverar commented Jan 30, 2023

Can you link to information about the timezone mapping file? That option sounds potentially appealing. After a couple minutes of searching, I still haven't found anything that looks relevant. (But again, I am far from expert with Windows.)

The file lives at %WINDIR%\Globalization\Time Zone\timezoneMapping.xml; it maps from Windows timezone IDs/names to TZIDs. I'm not super comfortable with parsing that file yet but the GetTimeZoneInformation Win32 API should help.

@astraw
Copy link
Contributor Author

astraw commented Jan 30, 2023

What breaks when you move to windows-sys 0.45? (We already established the listed reason of WinRT APIs disappearing is invalid.)

The PR for updating to windows-sys 0.45 is strawlab/iana-time-zone#95. Compilation fails because we depend on WinRT. I'm not sure when we "established the listed reason of WinRT APIs disappearing is invalid". We use WinRT for RoActivateInstance and HString operations although, again, this may be far from optimal Windows API usage and help would be appreciated. (Permalink to relevant file in the current git main: https://github.com/strawlab/iana-time-zone/blob/8a2f69aebc3da8978f395469fb5af449e67d5cde/src/tz_windows.rs)

@riverar
Copy link
Collaborator

riverar commented Jan 30, 2023

Ah thanks for your patience, I understand now.

So basically:

  • You can't ingest windows-sys because we nuked RoActivateInstance (and other WinRT related Win32 APIs)
  • You can't ingest windows due to MSRV
  • You can't use Windows ICU due to missing ICU on 1607 LTSB
  • Parsing the timezone mapping XML is not ideal/doesn't appear to be documented officially

Thinking 💭

@kennykerr
Copy link
Collaborator

I had a quick look and many of the windows crate features, including Globalization, could be built with 1.48 with only minor tweaks to the windows crate. If all you need is GetTimeZone, the following should be able to work with 1.48.

use windows::{core::Result, Globalization::Calendar};

fn main() -> Result<()> {
    println!("{}", Calendar::new()?.GetTimeZone()?);
    Ok(())
}

Let me know if that will let you adopt the windows crate directly.

As we are a dependency now of the very popular chrono, I do not want to introduce a dependency which they themselves do not have.

Note that chrono uses windows-sys rather than windows, since it only uses Win32 APIs, but they share the underlying windows-targets crate. But if you've decided against using the windows crate then I can't really help.

@Kijewski
Copy link

Sure, if we could use windows in 1.48, then this would be by far the best solution.

We switched to winapi only quite unwillingly, because windows did not meet our msrv requirements anymore. The version using windows-sys is not even published to crates.io, yet, and I'd be happy to never release that version, and use windows instead.

@kennykerr kennykerr added enhancement New feature or request and removed question Further information is requested labels Jan 30, 2023
@kennykerr
Copy link
Collaborator

#2318 now provides support for using (most) of the windows crate with Rust 1.48.

@Kijewski
Copy link

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants