Make TimeZones.jl relocatable

[lcontento]

Fixes #467

Half of a possible solution to #467. The other half is at JuliaTime/TZJData.jl#32.

As discussed in the issue, there are many possible ways to fix this. I have tested that this approach works when including TimeZones.jl in a system image and relocating the depot. I think it should also be 100% equivalent to the old code in a normal setup. I cannot exclude that there are other relocatability issues hidden away, but even if this will not be the long term solution to the problem I think it is a decent fix for most people's use-cases.

[omus]

Would be good to re-run and post the benchmarks from #457 around importing to see if this has an impact on load time.

[omus]

An option for backwards compat:

Suggested change

	_COMPILED_DIR[] = TZJData.artifact_dir()
	# Backwards compatibility with TZJData v1.3.0 and below. Using older versions means TimeZones.jl is not relocatable
	_COMPILED_DIR[] = isdefined(TZJData, :artifact_dir) ? TZJData.artifact_dir() : TZJData.ARTIFACT_DIR

[lcontento]

If we want to ensure backwards compatibility then relocatability can only be enforced by adding a lower bound to the TZJData version. I would see adding a lower bound to the version of TimeZones as a more natural way to do it, since that's the package I expect people usually add to their Project.toml.
Unless there is sometimes the need for keeping old versions of TZJData pinned, in which case backwards compatibility becomes really necessary. I don't know the package well enough to say if that's the case.

[DilumAluthge]

Yeah, I think most downstream users are depending on TimeZones.jl, not TZJData.jl.

Is there a pressing need for TimeZones.jl to support old versions of TZJData.jl? If so, could we just backport JuliaTime/TZJData.jl#32 to older versions of TZJData.jl?

[omus]

Part of the reason for the TimeZones.jl and TZJData.jl package separation is to allow for changing tzdata independently from the TimeZones.jl code. Setting a lower bound to TZJData.jl means we can't use those old versions. That's not the end of the world but the suggestion I had allows for relocatability and compatibility.

Backporting JuliaTime/TZJData.jl#32 is possible but we would still need to update the compat in TimeZones.jl to allow for each of those patch releases.

[lcontento]

If older versions of TZJData are needed then I guess we should go for backward compatibility. If relocatability is backported to the older versions of TZJData we can later remove the fallback. Similarly, if at any point the non-relocatable versions of TZJData become so old that no one would be expected to use them in new code, then a [compat] bound can be added to exclude them so that the fallback can be removed.

In the meanwhile, I think we could check whether TZJData.ARTIFACT_DIR actually exists and if not print an error message suggesting adding a [compat] bound for TZJData to ensure relocatability.

[lcontento]

I have added a fallback for older versions by hardcoding the artifact hashes and querying them using TZDATA_VERSION. This is not very elegant but 1) there is no 100% foolproof solution to retrieve the hash AFAIK 2) there are not too many versions. In this way we can ensure relocatability even with the old TZJDatas.

[omus]

Do you care about relocatability for old TZJData versions? I'm fine with using ARTIFACT_DIR as the fallback. I'd rather break relocatability for old versions than break compat support unnecessarily (if that is a problem).

[lcontento]

I think the current fallback works with all old versions of TZJData, so there is no need to change the [compat] bounds.

[lcontento]

I have tested it for the last released version of TZJData on a system image and it works, even without the relocatability PR for TZJData.

[lcontento]

Would be good to re-run and post the benchmarks from #457 around importing to see if this has an impact on load time.

I tried the benchmarks. The first time I run them after precompiling the packages they are quite slower, but on subsequent runs (after restarting the REPL) they are faster and consistent. This is for all three cases reported below, not sure why (maybe the artifact is redownloaded or rechecked every time the packages is recompiled?). I have reported only the timings for the runs >= 2nd (if you want to see the ones for the 1st run let me know): they all seem comparable and within the error I observe by running multiple times.

Before this PR:

julia> @time_imports import TimeZones
      0.5 ms  Scratch
      3.7 ms  InlineStrings
      0.2 ms  TZJData
      0.4 ms  Compat
      0.2 ms  Compat → CompatLinearAlgebraExt
      0.3 ms  ExprTools
      1.1 ms  Mocking
               ┌ 0.7 ms TimeZones.TZData.__init__() 
               ├ 0.0 ms TimeZones.__init__() 
     31.1 ms  TimeZones 36.28% compilation time

julia> using BenchmarkTools, TimeZones

julia> @btime istimezone("Europe/Warsaw");
  72.906 ns (1 allocation: 48 bytes)

This PR (with vanilla TZJData.jl)

julia> @time_imports import TimeZones
      0.4 ms  Scratch
      3.7 ms  InlineStrings
      0.3 ms  TZJData
      0.4 ms  Compat
      0.2 ms  Compat → CompatLinearAlgebraExt
      0.3 ms  ExprTools
      1.0 ms  Mocking
               ┌ 0.5 ms TimeZones.TZData.__init__() 
               ├ 0.1 ms TimeZones.__init__() 
     30.3 ms  TimeZones 33.97% compilation time

julia> using BenchmarkTools, TimeZones

julia> @btime istimezone("Europe/Warsaw");

  77.583 ns (1 allocation: 48 bytes)

This PR (with relocatable TZJData.jl)

julia> @time_imports import TimeZones
      0.4 ms  Scratch
      3.8 ms  InlineStrings
      1.0 ms  TZJData
      0.5 ms  Compat
      0.2 ms  Compat → CompatLinearAlgebraExt
      0.3 ms  ExprTools
      1.1 ms  Mocking
               ┌ 0.7 ms TimeZones.TZData.__init__() 
               ├ 0.1 ms TimeZones.__init__() 
     30.5 ms  TimeZones 36.08% compilation time

julia> using BenchmarkTools, TimeZones

julia> @btime istimezone("Europe/Warsaw");
  74.769 ns (1 allocation: 48 bytes)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.57%. Comparing base (2bc8f50) to head (c7612a0).
Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage   92.79%   92.57%   -0.23%     
==========================================
  Files          39       38       -1     
  Lines        1818     1831      +13     
==========================================
+ Hits         1687     1695       +8     
- Misses        131      136       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[omus]

Need to update the docs/Manifest.toml

[lcontento]

Do you really need a Manifest there? I will try to remove it just to see if it works.

Otherwise I am a bit unsure how to edit it since the entry for TimeZones is at a previous version, so if I added the Artifacts dependency there it would be weird. I am not very familiar with the process for building the documentation in Julia, so I am not sure where exactly the error occurs.

[lcontento]

It works without a Manifest there. It seems reasonable to me but again I am not familiar with Documenter.

[lcontento]

I tried to avoid the code coverage errors by excluding the new code, but the project level check still fails. The errors on Julia nightly were not there last time I looked at this, but they do not seem related to this PR.
@omus Is there anything else you would like to see improved here?

[omus]

I'll try to get this PR moving today