default to UTC when /etc/localtime is missing #756

esheppa · 2022-08-05T13:33:25Z

This is one potential solution to #755.

I've temporarily commented out some tests in src/offset/local/tz_info/timezone.rs, awaiting some discussion on where our "default to UTC" behavior should be (in unix.rs or in timezone.rs):

If it belongs in unix.rs then the assumption of the test is invalidated as they will all panic
If we move the logic to timezone.rs then the question is which errors should we recover from and which should we propogate (eg should filesystem errors cause a default to UTC as well?)
When we lookup in the current offset we also use an expect - should this default to UTC instead of panicing:

                self.zone
                    .find_local_time_type(d.timestamp())
                    .expect("unable to select local time type")
                    .offset(),

esheppa · 2022-08-05T13:35:33Z

Ideally in 0.5 we could consider whether now() should be fallible which would then allow (and encourage by the type system) users to decide how to fallback

IMO the risk with a silent fallback is there could be subtle bugs caused by expecting to get the local time but actually getting UTC rather than signalling that there is no currently defined local time.

djc · 2022-08-05T13:53:47Z

Nice, thanks for working on this!

My inclination is to keep this scoped to Unix for now, which is where we broke compatibility with 0.4.20. I think for now we should stick to only defaulting to UTC if we get a NotFound error, and keep panicking for other types of errors. I'd like to hear about other types of failures before we tackle them.

Maybe we can also improve on the error message people see in this case, to make it explicit that it's about /etc/localtime, thereby making it easier to fix a problem if it comes up?

djc · 2022-08-05T13:54:44Z

Oh, and we could add a try_now() already and expose it as fallible?

djc · 2022-08-05T14:05:27Z

I guess putting the logic in timezones.rs might work? I'm not sure, did you have a chance to look at how/when libc falls back?

esheppa · 2022-08-07T13:29:08Z

I tested this locally against date command which also seems to default to UTC in the case of no /etc/localtime symlink.

With regards to putting the logic in timezone.rs - I've looked into this again and the caching logic also needs to handle fallback so it's nice to keep it in one file (albeit, there are probably further optimizations of the logic between timezone.rs and unix.rs.

However this may actually not be the right path - these cases where we are falling back to UTC due to a missing /etc/localtime make sense on docker, but not in android. We may need something like an android.rs and others, to handle platform specific logic/APIs.

I'm also somewhat tempted by the suggestion of using iana_time_zone - however this would have to be in the form of a recommendation to use it with chrono-tz as it would need the compiled in time zone database. Potentially a long term solution is refactoring so that chrono-tz is merged into chrono and feature gated, then can be turned on for specific platforms local time implementations.

Alternatively to make this work in chrono for android:

We have to first check TZ (which we do) and then check 'persist.sys.timezone' system property
This only gives us the name of the selected timezone, which we then need to find in /system/usr/share/zoneinfo, then we can parse the correct one and continue as normal

djc · 2022-08-07T14:29:37Z

I think we should go the other way and rely on the system timezone database, I was thinking this morning about exposing other timezones in some way through the type system (where available).

You aren't quite explicit about why this solution doesn't work on Android, is that because we need to check this extra property? How would that work?

djc · 2022-08-08T12:15:01Z

Okay, after reviewing the code in iana-time-zone I can see that it would make sense to pull that in as a dependency. But not sure why that should be used with chrono-tz?

esheppa · 2022-08-08T12:49:36Z

I think we should go the other way and rely on the system timezone database, I was thinking this morning about exposing other timezones in some way through the type system (where available).

This is an excellent idea IMO! As its better to have the automatically updated system tzdb (plus, a "static" feature could be possible for deployments to an environment lacking tzdb).

You aren't quite explicit about why this solution doesn't work on Android, is that because we need to check this extra property? How would that work?

We have to use the android SDK or equivalent to find the name of the currently chosen timezone

Okay, after reviewing the code in iana-time-zone I can see that it would make sense to pull that in as a dependency. But not sure why that should be used with chrono-tz?

It only returns the name of the timezone, like "Asia/Kuala_Lumpur" or "America/Los_Angeles". This works if the system has the tzdb somewhere (eg Android has it, but in a different location), but chrono-tz is needed on systems that don't have a tzdb on the filesystem (potentially iOS, Windows (available but potentially different format/names) almost certainly wasm)

esheppa · 2022-08-08T12:54:33Z

Ideally the fallback here would only be applied if all the following conditions are met:

TZ environment variable is not set, or the file it points to is invalid
Nothing at /etc/localtime
iana-time-zone returns an error

djc · 2022-08-08T12:55:36Z

Yes, that sounds good.

I think we should prioritize shipping 0.4.21 that fixes this for systems that have a tzdb on the file system (ideally today?), we can take some more time to deal with iOS, I suppose (what did your "potentially" refer to?).

esheppa · 2022-08-08T13:01:15Z

One thing to note is that there are two main classes of failures we've seen currently:

Alpine docker images -> no tzdb, no /etc/localtime -> solution: default to UTC (will use iana-time-zone and verify it returns an error before defaulting to UTC)
Android -> has tzdb in different location, no /etc/localtime -> solution: use iana-time-zone to find zone name and then read the file from /system/usr/share/zoneinfo

Fixing these two gets us to a much better state than now, and then further work can be done for iOS etc (I'll raise separate issues for these)

djc · 2022-08-08T13:01:53Z

For iOS I guess we could pull in the core_foundation crate and call CFTimeZone::system().seconds_from_gmt().

esheppa · 2022-08-08T13:03:38Z

For iOS I guess we could pull in the core_foundation crate and call CFTimeZone::system().seconds_from_gmt().
This is ok for Local::now but we also need from_utc_datetime and from_local_datetime

djc · 2022-08-08T13:12:28Z

I guess we can see about getting support for daylightSavingTimeOffset, but yeah, let's defer this until after 0.4.21.

Cargo.toml

src/offset/local/unix.rs

esheppa · 2022-08-08T13:34:26Z

I've kept the MSRV change as a separate commit so it is clear when it happened

djc

Looking good!

Cargo.toml

src/offset/local/unix.rs

src/offset/local/tz_info/timezone.rs

src/offset/local/unix.rs

…ng, support android fix function name

djc

Great!

esheppa · 2022-08-09T13:34:39Z

would be great to add android and iOS to the CI, will investigate in a seperate PR

@yu-re-ka

- Updated from version 0.4.19 to 0.4.22 - Update of rootfile - Update of metadata patch as more windows related entries in Cargo.toml to be excluded - Changelog ## 0.4.22 * Allow wasmbindgen to be optional on `wasm32-unknown-unknown` target [(#771)](chronotope/chrono#771) * Fix compile error for `x86_64-fortanix-unknown-sgx` [(#767)](chronotope/chrono#767) * Update `iana-time-zone` version to 1.44 [(#773)](chronotope/chrono#773) ## 0.4.21 * Fall back to UTC timezone in cases where no timezone is found [(#756)](chronotope/chrono#756) * Correctly detect timezone on Android [(#756)](chronotope/chrono#756) * Improve documentation for strftime `%Y` specifier [(#760)](chronotope/chrono#760) ## 0.4.20 * Add more formatting documentation and examples. * Add support for microseconds timestamps serde serialization/deserialization (#304) * Fix `DurationRound` is not TZ aware (#495) * Implement `DurationRound` for `NaiveDateTime` * Implement `std::iter::Sum` for `Duration` * Add `DateTime::from_local()` to construct from given local date and time (#572) * Add a function that calculates the number of years elapsed between now and a given `Date` or `DateTime` (#557) * Correct build for wasm32-unknown-emscripten target (#568) * Change `Local::now()` and `Utc::now()` documentation from "current date" to "current date and time" (#647) * Fix `duration_round` panic on rounding by `Duration::zero()` (#658) * Add optional rkyv support. * Add support for microseconds timestamps serde serialization for `NaiveDateTime`. * Add support for optional timestamps serde serialization for `NaiveDateTime`. * Fix build for wasm32-unknown-emscripten (@yu-re-ka #593) * Make `ParseErrorKind` public and available through `ParseError::kind()` (#588) * Implement `DoubleEndedIterator` for `NaiveDateDaysIterator` and `NaiveDateWeeksIterator` * Fix panicking when parsing a `DateTime` (@botahamec) * Add support for getting week bounds based on a specific `NaiveDate` and a `Weekday` (#666) * Remove libc dependency from Cargo.toml. * Add the `and_local_timezone` method to `NaiveDateTime` * Fix the behavior of `Duration::abs()` for negative durations with non-zero nanos * Add compatibility with rfc2822 comments (#733) * Make `js-sys` and `wasm-bindgen` enabled by default when target is `wasm32-unknown-unknown` for ease of API discovery * Add the `Months` struct and associated `Add` and `Sub` impls Tested-by: Adolf Belka <[email protected]> Signed-off-by: Adolf Belka <[email protected]>

djc reviewed Aug 8, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

djc reviewed Aug 8, 2022

View reviewed changes

src/offset/local/unix.rs Outdated Show resolved Hide resolved

src/offset/local/unix.rs Outdated Show resolved Hide resolved

esheppa force-pushed the fix-no-set-timezone-panic branch 2 times, most recently from 81ad471 to 058241c Compare August 8, 2022 13:30

esheppa force-pushed the fix-no-set-timezone-panic branch 5 times, most recently from 2b7851a to 40cec2d Compare August 9, 2022 11:31

djc reviewed Aug 9, 2022

View reviewed changes

Cargo.toml Show resolved Hide resolved

src/offset/local/unix.rs Show resolved Hide resolved

src/offset/local/unix.rs Outdated Show resolved Hide resolved

src/offset/local/tz_info/timezone.rs Outdated Show resolved Hide resolved

src/offset/local/unix.rs Outdated Show resolved Hide resolved

msrv -> 1.38

9712fdb

esheppa force-pushed the fix-no-set-timezone-panic branch from 40cec2d to abe831b Compare August 9, 2022 13:21

default to UTC when iana-time-zone errors and /etc/localtime is missi…

669d555

…ng, support android fix function name

esheppa force-pushed the fix-no-set-timezone-panic branch from abe831b to 669d555 Compare August 9, 2022 13:24

djc approved these changes Aug 9, 2022

View reviewed changes

esheppa merged commit 557bcd5 into chronotope:main Aug 9, 2022

esheppa deleted the fix-no-set-timezone-panic branch August 9, 2022 13:43

This was referenced Aug 9, 2022

Panic with 0.4.20 on latest ArchLinux and Alpine #755

Closed

Consider bumping MSRV to 1.34.0 for TryFrom #757

Closed

madsmtm mentioned this pull request Aug 11, 2022

Fix core-foundation dependency cycle ryanmcgrath/cacao#44

Merged

djc mentioned this pull request Aug 22, 2022

chrono breaks on Android #788

Closed

pinkisemils mentioned this pull request Aug 25, 2022

Update chrono to 0.4.21 to fix Android mullvad/mullvadvpn-app#3863

Merged

dae mentioned this pull request Dec 23, 2022

Local timezones broken on Android #922

Closed

djc mentioned this pull request Apr 18, 2023

Remove iana-time-zone dependency for clock feature. #1018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default to UTC when /etc/localtime is missing #756

default to UTC when /etc/localtime is missing #756

esheppa commented Aug 5, 2022 •

edited

Loading

esheppa commented Aug 5, 2022 •

edited

Loading

djc commented Aug 5, 2022

djc commented Aug 5, 2022 •

edited

Loading

djc commented Aug 5, 2022 •

edited

Loading

esheppa commented Aug 7, 2022

djc commented Aug 7, 2022

djc commented Aug 8, 2022 •

edited

Loading

esheppa commented Aug 8, 2022 •

edited

Loading

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022 •

edited

Loading

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022

esheppa commented Aug 8, 2022

djc left a comment

djc left a comment

esheppa commented Aug 9, 2022

default to UTC when /etc/localtime is missing #756

default to UTC when /etc/localtime is missing #756

Conversation

esheppa commented Aug 5, 2022 • edited Loading

esheppa commented Aug 5, 2022 • edited Loading

djc commented Aug 5, 2022

djc commented Aug 5, 2022 • edited Loading

djc commented Aug 5, 2022 • edited Loading

esheppa commented Aug 7, 2022

djc commented Aug 7, 2022

djc commented Aug 8, 2022 • edited Loading

esheppa commented Aug 8, 2022 • edited Loading

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022 • edited Loading

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022

esheppa commented Aug 8, 2022

djc commented Aug 8, 2022

esheppa commented Aug 8, 2022

djc left a comment

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

esheppa commented Aug 9, 2022

esheppa commented Aug 5, 2022 •

edited

Loading

esheppa commented Aug 5, 2022 •

edited

Loading

djc commented Aug 5, 2022 •

edited

Loading

djc commented Aug 5, 2022 •

edited

Loading

djc commented Aug 8, 2022 •

edited

Loading

esheppa commented Aug 8, 2022 •

edited

Loading

djc commented Aug 8, 2022 •

edited

Loading