Stub out tzdb support to unblock `Now` and `ZonedDateTime` #99

nekevss · 2024-10-25T21:33:34Z

And thus begins the Great Time Zone / ZonedDateTime implementation. 😅

This stubs out tzdb support by relying solely on the user's zoneinfo files, and should allow us to move forward with implementing and testing ZonedDateTime without being blocked by a proper tzdb provider for Boa.

This PR implements:

Now builtin
FsTzdbProvider stub
Some ZonedDateTime APIs along with experimental, non-engine focused APIs.

Things of note that are not in this PR:

Handling POSIX's proleptic tz format and non-fat version of the TZDB.
Any thing zoneinfo / zic related.
Tests for more time zone edge cases
Many ZonedDateTime methods and abstract ops
Windows Support

Let me know what you think :)

jasonwilliams

LGTM!
Good start, nice to have a way forward which will unblock us for now

jasonwilliams · 2024-10-27T17:51:47Z

src/tzdb.rs

+
+use crate::{components::tz::TzProvider, iso::IsoDateTime, TemporalError, TemporalResult};
+
+const ZONEINFO_DIR: &str = "/usr/share/zoneinfo/";


I assume this is why you have the non-windows flags everywhere else, is there even a window equivalent file?

No, there's not really a windows equivalent, so we will need to package tzdb for windows.

That's where we may be able to use jiff_tzdb as an intermediate step, but we would need to add steps for resolving from the POSIX tz string as jiff is compiled in the "slim" format of tzdb while linux and macos are compiled in "fat"

jasonwilliams · 2024-10-27T17:53:31Z

Cargo.toml

+
+tzif = "0.2.3"
+iana-time-zone = "0.1.61"
+once_cell = "1.20.2"


Where is this being used? Also I think once_cell is built into the standard library now, the crate shouldn't be needed

Good catch! Forgot to remove it when I moved to std.

jedel1043

Thank you for all your work! I'm really excited to start playing with the timezone pieces.

Since this is still experimental, we can delay the resolution of my timezone related comments by opening an issue to address them if it would block merging this by a considerable amount.

src/components/tz.rs

jedel1043 · 2024-10-31T04:44:47Z

src/tzdb.rs

+        }
+    }


Just a small cleanup

Suggested change

}

}

if let Some(tzif) = self.cache.get(identifier) {

return Ok(identifier);

}

let tzif = Tzif::read_tzif(identifier)?;

Ok(self.cache.entry(identifier.into()).or_insert(tzif))

Huh, I really like this solution, but it looks like it has a lifetime issue around self.cache.get having an immutable borrow and self.cache.entry borrowing it mutably.

Granted that's probably fixed after adjusting for moving TzProvider to &self

src/tzdb.rs

jedel1043 · 2024-10-31T05:07:41Z

src/tzdb.rs

+        let record_minus_one = get_local_record(data_block, estimated_idx - 1);
+
+        // Potential shift bugs with odd historical transitions?
+        let shift_window = usize::from((record.utoff + record_minus_one.utoff).0.signum() >= 0);


It took me a bit of time to understand that this is just checking if the sum of offsets is positive, so maybe just do that?

Suggested change

let shift_window = usize::from((record.utoff + record_minus_one.utoff).0.signum() >= 0);

let shift_window = usize::from(record.utoff + record_minus_one.utoff >= Seconds(0))

jedel1043 · 2024-10-31T05:22:35Z

src/tzdb.rs

+            .expect("binary_search validates that data_block2 exists.");
+
+        let estimated_idx = match b_search_result {
+            Ok(idx) => idx,


Can't we just return the timezone data when the binary search finds a corresponding entry (Ok(idx))?

jedel1043 · 2024-10-31T05:25:20Z

src/tzdb.rs

+        let record = get_local_record(data_block, estimated_idx);
+        let record_minus_one = get_local_record(data_block, estimated_idx - 1);


I don't think this handles indexing to the transition table correctly. What if the estimated index is one past the end of the table, because the epoch is bigger than any of the transition times?

Yeah, if I remember correctly, I think there may have been a bug here, but it should also be solved by #100. If the value is past the transition times, then we need to use the POSIX tz string to resolve.

Ideally, at least the way I am understanding the behavior, we should return the first index if the seconds < transition_times[0]. if the value is greater than transition_times[n-1], then the POSIX tz string should be used to resolve the time zone.

src/tzdb.rs

jedel1043

Looks great!

Builds on #99. This PR adds resolving the POSIX tz string and should allow handling time zones on Windows via `jiff_tzdb` as a source, which should make the stub overall somewhat stable overall until a different solution can be put into place.

Re: title, depends on #99 and #100 This PR implements `ZonedDateTime`'s add and subtract methods along with related `TimeZone` operations that were needed. There is also one test to verify the results based off test262.

nekevss marked this pull request as ready for review October 25, 2024 21:37

nekevss requested review from jasonwilliams and jedel1043 October 25, 2024 21:44

jasonwilliams reviewed Oct 27, 2024

View reviewed changes

This was referenced Oct 29, 2024

Stub out tzdb support for Windows and POSIX tz string #100

Merged

WIP: Build out support for Now component #70

Closed

Implement ZonedDateTime's add and subtract methods #102

Merged

nekevss force-pushed the stub-tzdb-support branch from fc03b68 to 953d56e Compare October 31, 2024 05:36

jedel1043 reviewed Oct 31, 2024

View reviewed changes

src/tzdb.rs Outdated Show resolved Hide resolved

nekevss added 9 commits October 31, 2024 09:12

Stub out a temporary tzdb support to build zoneddatetime

215e394

Fix some bugs and add a couple tests

966f295

Remove printlns that were left in

1e22915

Fix a bug around ordering of records

f6890f8

Adjustments to handle no_std for tzdb after rebase

add8c69

Review feedback

64d2b3f

Remove hanging comment

83d2742

Add some docs

4676df6

Binary search changes

e00e658

nekevss force-pushed the stub-tzdb-support branch from 8e293fa to e00e658 Compare October 31, 2024 14:23

Gate windows support for now

d5c5ef3

jedel1043 approved these changes Oct 31, 2024

View reviewed changes

jedel1043 requested a review from jasonwilliams October 31, 2024 15:06

nekevss merged commit 0686135 into main Oct 31, 2024
5 checks passed

nekevss added the C-enhancement New feature or request label Nov 3, 2024

nekevss deleted the stub-tzdb-support branch November 5, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stub out tzdb support to unblock `Now` and `ZonedDateTime` #99

Stub out tzdb support to unblock `Now` and `ZonedDateTime` #99

nekevss commented Oct 25, 2024 •

edited

Loading

jasonwilliams left a comment

jasonwilliams Oct 27, 2024

nekevss Oct 27, 2024

jasonwilliams Oct 27, 2024

nekevss Oct 27, 2024

jedel1043 left a comment •

edited

Loading

jedel1043 Oct 31, 2024

nekevss Oct 31, 2024 •

edited

Loading

jedel1043 Oct 31, 2024

jedel1043 Oct 31, 2024 •

edited

Loading

jedel1043 Oct 31, 2024

nekevss Oct 31, 2024

jedel1043 left a comment


		use crate::{components::tz::TzProvider, iso::IsoDateTime, TemporalError, TemporalResult};

		const ZONEINFO_DIR: &str = "/usr/share/zoneinfo/";

-        }
-    }
+        if let Some(tzif) = self.cache.get(identifier) {
+            return Ok(identifier);
+        }
+        let tzif = Tzif::read_tzif(identifier)?;
+        Ok(self.cache.entry(identifier.into()).or_insert(tzif))

	let shift_window = usize::from((record.utoff + record_minus_one.utoff).0.signum() >= 0);
	let shift_window = usize::from(record.utoff + record_minus_one.utoff >= Seconds(0))

		let record = get_local_record(data_block, estimated_idx);
		let record_minus_one = get_local_record(data_block, estimated_idx - 1);

Stub out tzdb support to unblock Now and ZonedDateTime #99

Stub out tzdb support to unblock Now and ZonedDateTime #99

Conversation

nekevss commented Oct 25, 2024 • edited Loading

jasonwilliams 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

jedel1043 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekevss Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

Stub out tzdb support to unblock `Now` and `ZonedDateTime` #99

Stub out tzdb support to unblock `Now` and `ZonedDateTime` #99

nekevss commented Oct 25, 2024 •

edited

Loading

jedel1043 left a comment •

edited

Loading

nekevss Oct 31, 2024 •

edited

Loading

jedel1043 Oct 31, 2024 •

edited

Loading