search

JuliaTime / TimeZones.jl

IANA time zone database access for the Julia programming language
Other
86 stars 51 forks source link

Use non-lazy platform specific artifacts for unicode-cldr #439

Closed omus closed 11 months ago

omus commented 11 months ago

As brought up in #438 we were previously using lazy artifacts for the multiple unicode-cldr artifacts which are only needed on Windows for translating the current OS time zone name into the IANA time zone name. This approach was problematic because PackageCompiler 1.0 would always download all of the lazy artifacts and PackageCompiler 2.0 would not download the lazy artifacts (unless you use include_lazy_artifacts=true) resulting in the used unicode-cldr artifact being downloaded at runtime. Additionally, we only require the latest unicode-cldr artifact and the only reason we included multiple of these artifacts was due to us failing to clean them up in the dev/generate_artifacts.jl script which generates this file.

This PR changes the dev/generate_artifacts.jl to update the Artifacts.toml to only include the latest unicode-cldr. We now specify the platform for this architecture so that the artifact is only used on Windows which also allows us to specify it as non-lazy. Finally, there was a newer unicode-cldr release so I've also updated to use release-43-1 here.

Supercedes #438

omus commented 11 months ago

There is an argument to be made for testing on Windows 32-bit to ensure this artifact is present and working. I'll temporarily enable this environment as part of this PR but I don't believe it's necessary to test against this environment for other PRs. The exception being additional changes to the unicode-cldr artifact generation code.

codecov-commenter commented 11 months ago

Codecov Report

Merging #439 (1550176) into master (57d4a99) will not change coverage. The diff coverage is n/a.

:exclamation: 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     #439   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files          38       38           
  Lines        1762     1762           
=======================================
  Hits         1684     1684           
  Misses         78       78
Files Changed Coverage Δ
src/winzone/WindowsTimeZoneIDs.jl 100.00% <ø> (ø)
DilumAluthge commented 11 months ago

The exception being additional changes to the unicode-cldr artifact generation code.

We could configure it so that the Windows 32-bit CI job runs on PRs that touch said code, but otherwise is automatically skipped.

omus commented 11 months ago

An additional future improvement that can be made would be to create a new artifact to replace our use of unicode-cldr such that we can use a tiny XML file as the artifact instead of the 50MB artifact we currently fetch.

omus commented 11 months ago

The exception being additional changes to the unicode-cldr artifact generation code.

We could configure it so that the Windows 32-bit CI job runs on PRs that touch said code, but otherwise is automatically skipped.

The path filtering with GHA workflows happens at a workflow level so we'd either need a separate workflow, generate our matrix, or introduce conditions into our steps.

If I end up making the reduced sized artifact I'll probably use a separate data pkg and remove the Artifacts.toml here so I'll punt on the CI changes for now.

omus commented 11 months ago

Validated that Windows 32-bit works in https://github.com/JuliaTime/TimeZones.jl/pull/439/commits/dee8df5c23fcdd903ab875717b66514fffed425c. Reverted the commit and will merge after CI completes