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.

Sign up for GitHub

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

Jump to bottom

Support Android's system timezone database #13666

Merged
merged 5 commits into from
Jul 25, 2023
Merged

Support Android's system timezone database #13666

merged 5 commits into from
Jul 25, 2023

Conversation

HertzDevil
Copy link
Contributor

Resolves #13651.

straight-shoota
straight-shoota reviewed Jul 17, 2023
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering about the name android_tzdata. Is that a coined term?

src/crystal/system/unix/time.cr Outdated Show resolved Hide resolved
straight-shoota
straight-shoota reviewed Jul 17, 2023
View reviewed changes
src/time/location/loader.cr
else
File.open(path) do |file|
read_android_tzdata(file, false) do |location_name, location|
@@location_cache[location_name] = {time: mtime, location: location}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the modification time cached per location entry? They're all loaded from the same file, so shouldn't it be a global property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it is easier to reuse the same cache mechanism as the individual zoneinfo files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Let's do it that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking abou this, we might want to refactor the cache anyway.

@HertzDevil
Copy link
Contributor Author

I don't think android_tzdata is ever coined outside Android, or more precisely Bionic C

@straight-shoota straight-shoota added this to the 1.10.0 milestone Jul 24, 2023
@straight-shoota straight-shoota merged commit 0efe3c1 into crystal-lang:master Jul 25, 2023
53 checks passed
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jul 26, 2023 •
edited
Loading

Alpine is failing on CircleCI only: https://app.circleci.com/pipelines/github/crystal-lang/crystal/12717/workflows/a946e1d2-c96a-4932-a576-797b901df585/jobs/77669

EDIT: Another one https://github.com/crystal-lang/crystal/actions/runs/5684659479/job/15407860869?pr=13710

@HertzDevil HertzDevil deleted the feature/android-tzdata branch August 1, 2023 17:07
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
@HertzDevil @Blacksmoke16
Add support for Android's system timezone database (crystal-lang#13666)
ddca989
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sija Sija Sija left review comments

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:feature platform:android topic:stdlib:time
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Timezone database support on Android
3 participants
@HertzDevil @Sija @straight-shoota