MichaelHatherly commented 3 years ago

Fixes #338, based on https://github.com/JuliaTime/TimeZones.jl/issues/343#issuecomment-848823981 by KristofferC.

At module init runs the current build to generate the cached data, and stores it in a scratchspace instead of the deps folder. Associated globals that were defined at top-level are now set in __init__ (and _init for submodules) to allow for PackageCompiler support. Scratchspace is linked to the Julia and package version so should rebuild when those change for users.

The deps/build.jl now just imports the package which triggers a build of the cache if needed so this shouldn't really impact using TimeZones much as far as I could tell.

A required PR for Scratch to expand it's version support is at https://github.com/JuliaPackaging/Scratch.jl/pull/24. I've enabled support going back to 1.0 for that package to match what TimeZones says it supports, but it turns out EzXML is bounded at 1.3 so I couldn't test down to 1.0-1.2.

omus commented 3 years ago

This change will require us to drop support for Julia versions below 1.5. That's not a deal breaker but annoying as I want to continue to support the 1.0 LTS while it's the official LTS. The changes for dropping 1.0 support can be done in another PR. We'll need to at least disable the 1.0 CI jobs though so we can validate the changes. Ref: https://github.com/JuliaTime/TimeZones.jl/issues/340

MichaelHatherly commented 3 years ago

I'd like to retain LTS support as well, hence https://github.com/JuliaPackaging/Scratch.jl/pull/24 should be completed before finishing off this one, so that we can just use a version of Scratch that supports Julia 1+.

MichaelHatherly commented 3 years ago

@omus the CI failure is a bit of a mystery to me.. I've adjusted some paths and checks so maybe it'll get by this time.

codecov-commenter commented 3 years ago

Codecov Report

Merging #347 (fb1824b) into master (71178fe) will decrease coverage by 0.41%. The diff coverage is 82.60%.

:exclamation: Current head fb1824b differs from pull request most recent head b2c8022. Consider uploading reports for the commit b2c8022 to get more accurate results

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
- Coverage   93.60%   93.18%   -0.42%     
==========================================
  Files          32       33       +1     
  Lines        1532     1555      +23     
==========================================
+ Hits         1434     1449      +15     
- Misses         98      106       +8

Impacted Files	Coverage Δ
src/tzdata/build.jl	`66.66% <0.00%> (+3.03%)`	:arrow_up:
src/tzdata/download.jl	`96.22% <ø> (+0.22%)`	:arrow_up:
src/tzdata/version.jl	`91.42% <ø> (ø)`
src/TimeZones.jl	`84.61% <71.42%> (-15.39%)`	:arrow_down:
src/tzdata/TZData.jl	`87.50% <87.50%> (ø)`
src/winzone/WindowsTimeZoneIDs.jl	`83.87% <100.00%> (+4.70%)`	:arrow_up:
src/tzdata/compile.jl	`93.07% <0.00%> (-2.43%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 71178fe...b2c8022. Read the comment docs.

andreasnoack commented 3 years ago

Are there any unaddressed comments here or is this good to go?

MichaelHatherly commented 3 years ago

Are there any unaddressed comments here or is this good to go?

The failure in benchmarks is the current blocker here, still trying to pin it down.

MichaelHatherly commented 3 years ago

Closing in favour of #348.

JuliaTime / TimeZones.jl

Use `Scratch` for cached timezone data. #347

Codecov Report