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.

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

Use precompiled time zone information from TZJData.jl #441

Merged
merged 29 commits into from
Aug 22, 2023

Conversation

omus
Copy link
Member

@omus omus commented Aug 7, 2023

Updates the TimeZones to use the TZJData package as the default source of compiled time zones information. This eliminates the need to build the time zone information at package initialization time and avoids download issues with communicating with the IANA servers eliminating quite a few of the problems we've been having.

Users may choose to upgrade or downgrade the TZJData package they use to modify the version of the tzdata they use with the TimeZones package. Alternately, users with special use cases can still compile tzdata rules into time zone information which is stored in a scratch directory. Most users won't require this but it's useful when updating to using the latest tzdata version or exploratory research.

Fixes: #359, #357, #399, #343, #331, #325, #300, #321, #302, #279

Future work: The Unicode CLDR artifacts are around ~50MB and we only require a small XML file located within them. We should probably take a similar approach to the TZJData.jl package and only host the data we require. (#373)

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2023

Codecov Report

Merging #441 (ff5b8ab) into master (9f7fd55) will decrease coverage by 2.91%.
The diff coverage is 53.65%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   95.57%   92.66%   -2.91%     
==========================================
  Files          38       40       +2     
  Lines        1762     1841      +79     
==========================================
+ Hits         1684     1706      +22     
- Misses         78      135      +57     
Files Changed Coverage Δ
src/build.jl 0.00% <0.00%> (-100.00%) ⬇️
src/tzdata/TZData.jl 80.00% <ø> (ø)
src/tzdata/compile.jl 96.05% <ø> (ø)
src/tzdata/deprecated.jl 0.00% <0.00%> (ø)
src/tzdata/archive.jl 63.63% <63.63%> (ø)
src/TimeZones.jl 92.85% <80.00%> (-7.15%) ⬇️
src/types/timezone.jl 97.05% <100.00%> (+3.11%) ⬆️
src/tzdata/build.jl 100.00% <100.00%> (+10.00%) ⬆️
src/tzdata/download.jl 100.00% <100.00%> (ø)
src/tzdata/version.jl 74.19% <100.00%> (ø)
... and 1 more

@omus
Copy link
Member Author

omus commented Aug 7, 2023

There's a bit more work to do here yet. I've got to:

I'm aiming to get this done by the end of the week

omus added 10 commits August 8, 2023 23:37
Julia 1.9.2 on Windows was failing to find 7z under
`joinpath(Sys.BINDIR, Base.LIBEXECDIR, "7z.exe")`. As Pkg.jl uses the
p7zip_jll using this JLL seemed like the best option. I considered using
the Tar_jll instead but that JLL isn't supported on Windows.
Avoids failure to compare against previous version. Can be removed once
the current version is released.
Works around issue with PkgBenchmarks
@@ -4,6 +4,7 @@ authors = ["Curtis Vogt <[email protected]>"]
version = "1.11.0"

[deps]
Artifacts = "56f22d72-fd6d-98f1-02f0-08ddc0907c33"
Copy link
Member Author

Choose a reason for hiding this comment

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

Note we can remove the dependency on LazyArtifacts now but doing so interferes with running PkgBenchmarks. I'll leave in this dependency for now and remove it in a follow up PR where benchmarks don't matter

Copy link
Member Author

Choose a reason for hiding this comment

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

@test TimeZone("Africa/Windhoek").cutoff == DateTime(2201, 4, 5)
end
finally
TimeZones._reload_cache(TimeZones._COMPILED_DIR[])
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be better if these tests could just compile the extended time zones in memory and avoid writing these to disk. Will try to do as a follow up

Copy link
Member Author

Choose a reason for hiding this comment

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

@omus omus marked this pull request as ready for review August 12, 2023 05:08
@omus
Copy link
Member Author

omus commented Aug 12, 2023

PR is only waiting on a 1.0 release of TZJData. Anyone wanting to review this PR should do so in the next couple of days.

@omus
Copy link
Member Author

omus commented Aug 22, 2023

Took a bit longer to get the TZJData v1.0.0 release merged into the registry than I anticipated. I'll let CI run and make a new TimeZones.jl release tonight and we'll finally be able to close all those issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Separate TZData module into separate package
2 participants