Use Scratch for cached timezone data.
#347
Conversation
Fixes JuliaTime#338. 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 JuliaPackaging/Scratch.jl#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.
src/TimeZones.jl
Outdated
| if isempty(readdir(DEPS_DIR)) | ||
| cd(DEPS_DIR) do | ||
| for (path, contents) in DEPS_CONTENTS | ||
| mkpath(dirname(path)) | ||
| write(path, contents) | ||
| end | ||
| end | ||
| build() | ||
| end |
There was a problem hiding this comment.
I suspect it would be cleaner to just have the build process write the scratch directory?
There was a problem hiding this comment.
Yes, I'll move it into build() instead.
src/TimeZones.jl
Outdated
| function __init__() | ||
| global DEPS_DIR = @get_scratch!("timezones-$VERSION-$pkg_version") |
There was a problem hiding this comment.
May be able to use the tzdata version
There was a problem hiding this comment.
Can do, is the cache folder structure likely to remain the same between package versions? If not then using just tzdata_version as the scratch key may cause issues, perhaps.
There was a problem hiding this comment.
It's probably safest to include Julia version, package version, and tzdata version. It also just occurred to me that users are allowed to re-compile the time zones to support dates in the far future. I'll need to think more about this.
|
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: #340 |
|
I'd like to retain LTS support as well, hence JuliaPackaging/Scratch.jl#24 should be completed before finishing off this one, so that we can just use a version of |
|
@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 Report
@@ 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
Continue to review full report at Codecov.
|
src/tzdata/TZData.jl
Outdated
| global ARCHIVE_DIR = joinpath(TimeZones.DEPS_DIR, "tzarchive") | ||
| global TZ_SOURCE_DIR = joinpath(TimeZones.DEPS_DIR, "tzsource") | ||
| global COMPILED_DIR = joinpath(TimeZones.DEPS_DIR, "compiled", string(VERSION)) | ||
| global ACTIVE_VERSION_FILE = joinpath(TimeZones.DEPS_DIR, "active_version") | ||
| global LATEST_FILE = joinpath(TimeZones.DEPS_DIR, "latest") |
There was a problem hiding this comment.
I think I'd prefer if these were all global constants and we used Ref{String}()
There was a problem hiding this comment.
Are any of these constants regarded as public? It'll of course be a breaking change if that's the case unfortunately.
| if tzdata_hash === nothing | ||
| error("Latest tzdata is $latest_version which is not present in the Artifacts.toml") | ||
| end | ||
| @artifact_str "tzdata$latest_version" |
There was a problem hiding this comment.
It should have the same effect of throwing an error when the particular artifact does not exist, but by using @artifact_str the Artifact.toml contents gets embedded rather than looked up at runtime in the file, which may not exist if it's been relocated.
deps/build.jl
Outdated
| using TimeZones: build | ||
|
|
||
| build() | ||
| using TimeZones |
There was a problem hiding this comment.
Hmmm, if someone manually runs ] build we should probably re-build the scratch directory.
Not too sure why the cache isn't being found when running the benchmarks, but manually rebuilding seems to help locally. It's not particularly ideal, and is probably being caused by some other underlying issue with this change.
|
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. |
|
Closing in favour of #348. |
Fixes #338, based on #343 (comment) by KristofferC.
At module init runs the current
buildto generatethe 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_initfor submodules) to allow forPackageCompilersupport. Scratchspace is linked to the Julia andpackage version so should rebuild when those change for users.
The
deps/build.jlnow just imports the package which triggersa build of the cache if needed so this shouldn't really impact
using TimeZonesmuch as far as I could tell.A required PR for
Scratchto expand it's version support is atJuliaPackaging/Scratch.jl#24. I've enabled
support going back to 1.0 for that package to match what
TimeZonessays it supports, but it turns out
EzXMLis bounded at1.3soI couldn't test down to 1.0-1.2.