-
Notifications
You must be signed in to change notification settings - Fork 13
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
Parsing and unparsing leads to 1h time shift? #17
Comments
The timestamp |
This was not on master but on |
I believe this happens on master as well. Running the tests on master (on macOS) I also see a test failure:
I tried to use the branch from #5 but it is quite out of date (using |
Ah my bad, what about |
On branch
|
Interesting, I don't see a test error on my end. I'd like to merge this at some point but am not confident enough in its correctness. Frankly, help would be very appreciated. |
Tests only work for me on branch |
This looks suspicious (on master):
This computes an
I don't understand what it tries to do. |
well, me neither. I think it's different on #5 ? |
What's the timezone of your system set to?
|
|
the Stdlib does a system call, don't see anything egregious at first sight. |
On my computer, I am at GMT+1, do I have to add
EDIT: In #5, I see that it has been removed. It would be nice to test the library in different time zones. It might give good results in UK but false ones in other places. |
As you will have noticed, I'm in GMT/UTC and still get the same |
Yes indeed, but according to what documentation says, you should not. Unless I misunderstood the documentation. |
I had asked @undu and he sees it on his system (also in GMT/UTC) as well. And given that this is coming from a system call, I am reluctant to believe that this is a bug in |
A similar issue seems to appear during the tests of
From what I understand of the discussion it seems to be a known issue so I'm just going to disable the tests for this package. |
Heard about this issue from Discord and made some comments over there, thought I'd copy the gist of my observation over here. My speculation is that line 6 A straightforward fix would be to tune said offset calculation according to whether DST was being observed at the time of Unix epoch. But this would still assume the base offset remains unchanged over the years, which might hold for contemporary periods, but I'll need to verify. EDIT: I haven't read any of the PR's in detail, so I'm not sure if there's already a fix somewhere. |
My above observation as it turns out was somewhat getting close, but not entirely correct. I made some code to test if any of the time zones observe changed "base" offset, and a lot of them failed, which includes let l =
let open Timere in
let unix_epoch = 0L in
(* Time_zone.available_time_zones *)
[ "Europe/London" ]
|> List.map (fun name ->
let tz = Time_zone.make_exn name in
(name,
Time_zone.Raw.to_transitions tz
|> List.filter (fun ((start, end_exc), _entry) ->
unix_epoch < end_exc
)
|> List.map (fun ((start, end_exc), Time_zone.{ is_dst; offset }) ->
(* Printf.printf "start: %Ld, end_exc: %Ld, is_dst: %b, offset: %d\n" start end_exc is_dst offset; *)
offset - if is_dst then 3600 else 0
)
|> List.sort_uniq compare
)
)
|> List.filter (fun (name, offsets) -> List.length offsets > 1) gives
indicating two unique "base" offsets. In other words, a "base" offset does not seem to be necessarily well defined even if we only start at Unix epoch (1970 Jan 1 00:00:00 UTC). Examining the incongruence further, we can indeed observe that via zdump command (lines of interest are marked with <== on the right hand side)
via
So at least for Namely we have a case where sampling at Unix epoch does not yield a useful base offset. I'll add that my initial observation was based on information from https://www.timeanddate.com/time/change/uk/london where it reads "DST observed all year" for 1969 and 1970, but this doesn't seem to line up properly with the |
A small update: I've patched ISO8601 to use Timere on my fork at Timere does bring in a lot more complexity and dependencies, so I am not entirely sure if pushing toward using Timere under the hood is necessarily a good option for all users, but it doesn't seem to be a bad tradeoff if having difficult to resolve bugs is the only other option. |
The actual Unix
This causes the Unix implementation to guess DST, which is almost always possible except for the hour when DST comes in or out of effect. I believe we should not guess any offset but just accept the result of |
I suspect there is some misconception about the Unix epoch at play here: On a Linux system the timestamp for Jan 01 1970 is not zero, as we (and the unit tests) expect:
This agrees with what I see on macOS as well. The timestamp at the epoch is not zero. |
The current implmentation calculates a an offset based on Daylight Saving Time being in effect at the Epoch: t -. offset +. (if tm.Unix.tm_isdst then 3600. else 0.) This leads to suprises in the Europe/London timezone where unparsing and parsing a date leads to an offset - see this issue: ocaml-community#17 The following change eliminates this and makes all unit tests pass. Somewhat surprisingly the knowledge about this offset is encoded both in the implementation and the unit tests. Likewise surprising, the offset is not zero in the Europe/London timezone but it seems to be consistent over various Unix implementations. Signed-off-by: Christian Lindig <[email protected]>
The failure of the unit test can be easily reproduced (on macOS for me, on two systems):
There is no unit test failure for Looking at @darrenldl's comment I agree that looking at the UTC offset at timestamp 0 is not helpful because it is wrong for London and this is coming from the underlying time zone information in the OS and not the OCaml implementation.
|
I assume I'm doing something wrong here but I am baffled by this behaviour:
The time printed is shifted by one hour from the time that was parsed. How would I prevent that?
The text was updated successfully, but these errors were encountered: