Add parser for tzdata on android #975
Conversation
There was a problem hiding this comment.
Thanks for working on this!
Here are some suggestions. I wonder if this should maybe remain a separate crate that chrono will depend on only on Android, would that make sense? That would give you more freedom on how to test and avoids the chrono maintainers having to take responsibility for a piece of core infrastructure that is hard to actually exercise in CI (or maybe it isn't?)
Alternatively, for testing we could subset the original file to have like one or two timezones only?
| @@ -24,6 +24,8 @@ pub(crate) enum Error { | |||
| LocalTimeType(&'static str), | |||
| /// Invalid slice for integer conversion | |||
| InvalidSlice(&'static str), | |||
| /// Invalid tzdata file | |||
| InvalidTzdataFile(&'static str), | |||
There was a problem hiding this comment.
Style nit: I think we'd want to capitalize Data here?
| @@ -52,6 +52,11 @@ impl TimeZone { | |||
| return Self::from_file(&mut file); | |||
| } | |||
|
|
|||
| #[cfg(target_os = "android")] | |||
| if let Ok(bytes) = super::super::tzdata::find_tz_data(tz_string) { | |||
There was a problem hiding this comment.
Style: not very fond of super::super, maybe use the path from crate:: instead?
| } | ||
|
|
||
| pub(super) fn find_tz_data(tz_name: &str) -> Result<Vec<u8>, Error> { | ||
| let mut file = find_file()?; |
There was a problem hiding this comment.
Since this is the only caller for find_file(), I think we could just inline it?
| [("ANDROID_DATA", "/misc/zoneinfo/"), ("ANDROID_ROOT", "/usr/share/zoneinfo/")] | ||
| { | ||
| if let Ok(env_value) = std::env::var(env_var) { | ||
| if let Ok(file) = File::open(env_value + dir + "tzdata") { |
There was a problem hiding this comment.
Not a great fan of concatenating strings this way, either. Can we just do a format!() call instead?
| Err(Error::Io(io::Error::from(io::ErrorKind::NotFound))) | ||
| } | ||
|
|
||
| fn find_tz_data_in_file(file: &mut File, tz_name: &str) -> Result<Vec<u8>, Error> { |
There was a problem hiding this comment.
I would probably also inline this?
| } | ||
|
|
||
| /// Panics if slice does not contain [TZ_INT_SIZE] bytes beginning at start. | ||
| fn parse_tz_int(slice: &[u8], start: usize) -> u32 { |
There was a problem hiding this comment.
I'd make this a newtype with a constructor, we can use the type for the Header fields.
| // The database uses 32-bit (4 byte) integers. | ||
| const TZ_INT_SIZE: usize = 4; | ||
| // The first 12 bytes contain a special version string. | ||
| const MAGIC_SIZE: usize = 12; | ||
| const HEADER_SIZE: usize = MAGIC_SIZE + 3 * TZ_INT_SIZE; | ||
| // The database reserves 40 bytes for each id. | ||
| const TZ_NAME_SIZE: usize = 40; | ||
| const INDEX_ENTRY_SIZE: usize = TZ_NAME_SIZE + 3 * TZ_INT_SIZE; |
There was a problem hiding this comment.
Would prefer to have the constants at the bottom of the module.
|
Thanks for the feedback! I'll come back to it in the upcoming week. |
|
To clarify the testing situation, the parsing can be tested easily in CI/on the local platform if a test tzdata file is bundled. Testing on a device/simulator would be a fair bit more complicated - you need to install the Android NDK, cross-compile, then spin up a sim or device to run the code, and it's unlikely to be worth the effort just to confirm the file exists in a standard location. |
Sure, the existence of the file and its path is one thing. But since this appears to more or less reverse engineer Android's format, it's not clear to me that we can depend on the stability of the format in the longer term. Unless I'm missing something here? |
|
There are no guarantees, but as far as I'm aware it's been the same format for a long time. Golang rolled their own version in 2016, and I don't see any changes to the format since then in their tree: https://github.com/golang/go/commits/31881f87873b84709a49ca17195bbe5b3f683acf/src/time/zoneinfo_android.go |
Closes #922.
The only way to add a meaningful unit test seems to require a tzdata file, but I don't want to check in a 500 KB binary (67 KB zipped). What do you think?
This commit already had some alpha testing as part of the @ankidroid project.