Use Scratch to hold downloads and compiled tzdata
#384
Conversation
We don't want to mutate the package directory; let's use a scratchspace instead, and adjust all `const` variables to do runtime lookups, for maximum relocatability. We shift around the data a bit here, to make it more clear that the package directory is not to be mutated. The ideal case here now is that, on a fresh install, nothing is downloaded for the default happy path, the package directory is never mutated, and the build step is minimal in cost.
Codecov Report
@@ Coverage Diff @@
## master #384 +/- ##
==========================================
+ Coverage 95.12% 95.24% +0.12%
==========================================
Files 35 36 +1
Lines 1743 1746 +3
==========================================
+ Hits 1658 1663 +5
+ Misses 85 83 -2
Continue to review full report at Codecov.
|
|
As a side note, I did experiment with eliminating the local serialization step completely; I used We could have a GitHub Actions job that automatically serializes all tzdata sets and uploads them to a GitHub release or similar, and opens a pull request if a new tzdata set is added without the paired serialized artifact. If someone wants to do this, I'd be happy to guide them through it, but I can't do it myself at the moment. |
staticfloat
force-pushed
the
sf/serialize_to_scratch
branch
from
f197cad
to
a240c06
Compare
|
Thanks for this @staticfloat. I'll give this a thorough review tomorrow but I'm already sure I'd like to pair this with defaulting to the TZJFile format introduced in #380. |
There was a problem hiding this comment.
Overall seems good to me. I've modified some of the names to be more consistent. e.g. mixing "compiled"/"serialized"/"serialized cache" or "tz_source"/"source". I'll mention at first I was thinking using Ref constants for the directories was the correct approach but the use of functions reduces the chance of mutations there so I think it's the right direction. I'll make a follow up PR to rename "custom_tzsource_regions" and I think we can merge this. I'll integrate defaulting to the TZJFile format before making a release though
|
I should call out that this could be a breaking change if people were referencing the now removed constants. I'm not too worried about this but I may use some |
With this in mind I think it makes sense to merge #385 first as that will eliminate us having to specify the Julia version number to ensure serialization compatibility. |
After sleeping on this there may be an advantage in the |
|
Alright in order to get things moving I'll merge this as it exists now and follow up on the issues I pointed out in a follow up PR. |
|
The changes in this PR remove the constants: All of these should only be used internally by TimeZones.jl so I won't bother with trying to gracefully deprecate external usages. In the future I'll try to be more diligent in this package to mark internal contants and functions with leading underscores. |
|
This PR is also running into #386. Looking into this before merging here |
Failure definitely appears unrelated. |
* Eagerly download the default TZData * Use `Scratch` to hold downloads and compiled tzdata We don't want to mutate the package directory; let's use a scratchspace instead, and adjust all `const` variables to do runtime lookups, for maximum relocatability. We shift around the data a bit here, to make it more clear that the package directory is not to be mutated. The ideal case here now is that, on a fresh install, nothing is downloaded for the default happy path, the package directory is never mutated, and the build step is minimal in cost. * Fix Windows as well * Rename `serialized_cache_dir` to `compiled_dir` * Rename `get_windows_xml_file_path` to `windows_xml_file_path` * Formatting changes * Improve backwards compatibility in `build` Co-authored-by: Curtis Vogt <[email protected]>
* Specify Scratch compat * Rename custom tzsource directory * Refactor tzdata copying logic * Restore region selection behaviour for compile
This should address many of the problems listed in #343. It still contains a
Pkg.build()step to do the serialization, but it stores things in a scratch space, rather than in the package directory, and it marks the default tzdata as an eager artifact, rather than a lazy one, so it should download along with the rest of the artifacts during package installation.The one thing I'm not sure about is whether the "version" that it checks against the IANA servers is properly cached; I see many different ways that the version can be queried, and I'm not sure if the "happy path" still makes a request against the IANA servers. I would prefer that it doesn't, and no network accesses outside of the normal Pkg flow occur, as that helps this package be used in restrictive environments where only an internal PkgServer is whitelisted.