JuliaTime / TimeZones.jl

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

Relocatability no longer given due to link to TZJData.ARTIFACT_DIR and storing path in const string reference #467

Open divbyzerofordummies opened 2 months ago

divbyzerofordummies commented 2 months ago

We have encountered a relocatability issue when creating a multi-stage docker container where we copy an app created by PackageCompiler to a clean image after having built it.

It now looks for the artifact in the old directory which was valid at build time and is no longer available in the final container.

The hash of the artifact is 520ce3f83be7fbb002cca87993a1b1f71fe10912 and I have verified that this is the hash of the artifact created when imporing TZJData.

Looking for this problem I have found a post with a similar problem with a solution in the following commit.

The problem seems to be the definition of a constant in TZJData.jl:

const ARTIFACT_DIR = artifact"tzjdata" 

which is referenced in TimeZones.jl (again, with the const keyword, but this time on the reference, so the value is very much mutable):

const _COMPILED_DIR = Ref{String}(TZJData.ARTIFACT_DIR)

According to the comment in the post mentioned above, "Is it because the const makes the path fixed at compilation, whereas we need it to load and find the artifact each time?", this seems to be a bad idea when trying to keep a package portable.

The solution seems to make both const definitions functions instead, i.e., in TZJData.jl:

function artifact_dir()
    return artifact"tzjdata" 
end

(not sure about the necessity to put this into a function in the first place), and in TimeZones.jl:

function compiled_dir()
    return TZJData.artifact_dir()
end

BUT I am not really sure how this works when building with a different tzdata version: In TimeZones.jl, it says:

function _build()
    desired_version = TZData.tzdata_version()
    if desired_version != TZJData.TZDATA_VERSION
        _COMPILED_DIR[] = TZData.build(desired_version, _scratch_dir())
    end

    return nothing
end

I have absolutely no clue how/whether this changed content of the referenced string ends up in my compiled app... and if it does (which I assume) how portability could still be ensured, because now it is definitely an incorrect path on my final docker image.

But I guess for the vanilla user not requesting a special version of tzdata, my solution would work, but I would need an extra string reference to the new compile dir if the user chooses a different version. That would be quite ugly and still not portable for special tzdata requirements...

const _CUSTOM_TZDATA_DIR = Ref{String}("")
function compiled_dir()
    return if isempty(_CUSTOM_TZDATA_DIR[])
        TZJData.artifact_dir()
    else
        _CUSTOM_TZDATA_DIR[]
    end
end

I assume / hope more experienced Julia programmers have better ideas?

omus commented 4 days ago

Thanks for bringing this up. An alternate solution could be to utilize __init__ functions to populate Ref constants. The benefit of this approach is that it lets us control when we perform the artifact check.

Something we want to avoid is having the artifacts downloaded at runtime within a Docker container. Ideally, precompilation and artifacts are installed during the PackageCompiler process so we aren't suddenly trying to install artifacts during the container runtime. As you mentioned the artifact hash remains consistent in your two different container environments so maybe an alternate solution would be to update TZJData.ARTIFACT_DIR to be a path relative to the Base.DEPOT_PATH. This should allow for good package loading performance and improve portability.

If you want to create PRs with such approaches you should be able to validate they work in your specific setup (use pkg> add TimeZones#branch-name) or alternatively if you could provide a MWE of your setup I can work on the issue.

omus commented 4 days ago

We probably do want to calculate the artifact location at runtime so that overrides are applied. I'll need to think about this more yet.