Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TimeZones.jl relocatable #479

Merged
merged 21 commits into from
Dec 20, 2024
Merged

Make TimeZones.jl relocatable #479

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
56865ff
Set _COMPILED_DIR during __init__
lcontento Oct 29, 2024
05d4c9e
Add fallback for older versions of TZJData
lcontento Nov 4, 2024
ca1ba7b
Backwards compatibility with hardcoded artifact hashes
lcontento Nov 4, 2024
8879f7e
Merge branch 'master' into lc/relocatable
lcontento Nov 4, 2024
0402582
remove the docs manifest
lcontento Nov 4, 2024
c7612a0
Turn off coverage for the setting of _COMPILED_DIR
lcontento Dec 16, 2024
9af280c
Determine tree hash directly from Artifacts.toml
omus Dec 19, 2024
f448082
Merge branch 'master' into lc/relocatable
omus Dec 20, 2024
3fbda1d
Remove CodeCov exclusion comments
omus Dec 20, 2024
5aa01e6
Use `locate_package`
omus Dec 20, 2024
7e3a3cb
Test for sysimage
omus Dec 20, 2024
63cbb29
fixup! Test for sysimage
omus Dec 20, 2024
c303339
fixup! Test for sysimage
omus Dec 20, 2024
99f9e7e
fixup! Test for sysimage
omus Dec 20, 2024
451d241
fixup! Test for sysimage
omus Dec 20, 2024
3aa1b39
fixup! Test for sysimage
omus Dec 20, 2024
c64babd
fixup! Test for sysimage
omus Dec 20, 2024
b70192c
Validate test failure
omus Dec 20, 2024
73da68a
fixup! Test for sysimage
omus Dec 20, 2024
549fdfd
Back to working
omus Dec 20, 2024
8d430e1
Make job name unique
omus Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions src/TimeZones.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module TimeZones

using Artifacts
using Artifacts: Artifacts
using Dates
using Printf
using Scratch: @get_scratch!
Expand Down Expand Up @@ -42,23 +42,13 @@ abstract type Local <: TimeZone end

function __init__()
# COV_EXCL_START
# Must be set at runtime to ensure relocatability
_COMPILED_DIR[] = if isdefined(TZJData, :artifact_dir)
# Recent versions of TZJData are relocatable
# Set at runtime to ensure relocatability
_COMPILED_DIR[] = @static if isdefined(TZJData, :artifact_dir)
TZJData.artifact_dir()
else
# Backwards compatibility with TZJData v1.3.0 and below
hash = if TZJData.TZDATA_VERSION == "2024b"
Base.SHA1("7fdea2a12522469ca39925546d1fd93c10748180")
elseif TZJData.TZDATA_VERSION == "2024a"
Base.SHA1("520ce3f83be7fbb002cca87993a1b1f71fe10912")
elseif TZJData.TZDATA_VERSION == "2023d"
Base.SHA1("b071cffdb310f5d3ca640c09cfa3dc3f23d450ad")
elseif TZJData.TZDATA_VERSION == "2023c"
Base.SHA1("52e48e96c4df04eeebc6ece0d9f1c3b545f0544c")
else
error("TZJData.jl with TZDATA_VERSION = $(TZJData.TZDATA_VERSION) is supposed to be relocatable!")
end
# Backwards compatibility for TZJData versions below v1.3.1
artifact_dict = Artifacts.parse_toml(joinpath(pkgdir(TZJData), "Artifacts.toml"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work with a system image because pkgdir(TZJData) is the path of the TZJData package on the machine that created the system image.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could try to take the last part of the path and look into all depots until one finds the new package path. However, I am not 100% sure that having the source of the package in the depot is a requirement; I think you can probably run the system image without downloading the source. Hence, my original, but defintely not elegant, approach of hard-coding the hashes...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? That seems like a bug as we're calling pkgdir at package initialization time so this should be looked up on the system which is running the sysimage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's a situation similar to @__DIR__ which is also non-relocatable.

Copy link
Member

@omus omus Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to be working as intended (future reader note. You need to use TZJData 1.3.0):

❯ JULIA_DEPOT_PATH=/tmp/depot julia -E 'using Pkg; Pkg.add(PackageSpec(name="TimeZones", version="1.19.0")); using TimeZones; TimeZones._COMPILED_DIR[]'
  Installing known registries into `/tmp/depot`
       Added `General` registry to /tmp/depot/registries
    Updating registry at `/tmp/depot/registries/General.toml`
   Resolving package versions...
   Installed Mocking ─────── v0.8.1
   Installed InlineStrings ─ v1.4.2
   Installed Compat ──────── v4.16.0
   Installed TZJData ─────── v1.3.0+2024b
   Installed ExprTools ───── v0.1.10
   Installed Scratch ─────── v1.2.1
   Installed TimeZones ───── v1.19.0
  Downloaded artifact: tzjdata
    Updating `/private/tmp/depot/environments/v1.11/Project.toml`
  [f269a46b] + TimeZones v1.19.0
    Updating `/private/tmp/depot/environments/v1.11/Manifest.toml`
  [34da2185] + Compat v4.16.0
  [e2ba6199] + ExprTools v0.1.10
  [842dd82b] + InlineStrings v1.4.2
  [78c3b35d] + Mocking v0.8.1
  [6c6a2e73] + Scratch v1.2.1
  [dc5dba14] + TZJData v1.3.0+2024b
  [f269a46b] + TimeZones v1.19.0
  [0dad84c5] + ArgTools v1.1.2
  [56f22d72] + Artifacts v1.11.0
  [ade2ca70] + Dates v1.11.0
  [f43a241f] + Downloads v1.6.0
  [7b1f6079] + FileWatching v1.11.0
  [b27032c2] + LibCURL v0.6.4
  [8f399da3] + Libdl v1.11.0
  [ca575930] + NetworkOptions v1.2.0
  [de0858da] + Printf v1.11.0
  [9a3f8284] + Random v1.11.0
  [ea8e919c] + SHA v0.7.0
  [fa267f1f] + TOML v1.0.3
  [cf7118a7] + UUIDs v1.11.0
  [4ec0a83e] + Unicode v1.11.0
  [deac9b47] + LibCURL_jll v8.6.0+0
  [29816b5a] + LibSSH2_jll v1.11.0+1
  [c8ffd9c3] + MbedTLS_jll v2.28.6+0
  [14a3606d] + MozillaCACerts_jll v2023.12.12
  [83775a58] + Zlib_jll v1.2.13+1
  [8e850ede] + nghttp2_jll v1.59.0+0
  [3f19e933] + p7zip_jll v17.4.0+2
Precompiling project...
  9 dependencies successfully precompiled in 7 seconds. 14 already precompiled.
"/tmp/depot/artifacts/7fdea2a12522469ca39925546d1fd93c10748180"

❯ mv /tmp/depot /tmp/depot2

❯ JULIA_DEPOT_PATH=/tmp/depot2 julia -E 'using TimeZones; TimeZones._COMPILED_DIR[]'
"/tmp/depot/artifacts/7fdea2a12522469ca39925546d1fd93c10748180"

❯ rm -rf /tmp/depot2

❯ JULIA_DEPOT_PATH=/tmp/depot julia -E 'using Pkg; Pkg.add(PackageSpec(url="https://github.com/lcontento/TimeZones.jl", rev="lc/relocatable")); using TimeZones; TimeZones._COMPILED_DIR[]'
  Installing known registries into `/tmp/depot`
       Added `General` registry to /tmp/depot/registries
     Cloning git-repo `https://github.com/lcontento/TimeZones.jl`
    Updating git-repo `https://github.com/lcontento/TimeZones.jl`
    Updating registry at `/tmp/depot/registries/General.toml`
   Resolving package versions...
   Installed Scratch ─────── v1.2.1
   Installed Compat ──────── v4.16.0
   Installed InlineStrings ─ v1.4.2
   Installed TZJData ─────── v1.3.0+2024b
   Installed Mocking ─────── v0.8.1
   Installed ExprTools ───── v0.1.10
  Downloaded artifact: tzjdata
    Updating `/private/tmp/depot/environments/v1.11/Project.toml`
  [f269a46b] + TimeZones v1.19.0 `https://github.com/lcontento/TimeZones.jl#lc/relocatable`
    Updating `/private/tmp/depot/environments/v1.11/Manifest.toml`
  [34da2185] + Compat v4.16.0
  [e2ba6199] + ExprTools v0.1.10
  [842dd82b] + InlineStrings v1.4.2
  [78c3b35d] + Mocking v0.8.1
  [6c6a2e73] + Scratch v1.2.1
  [dc5dba14] + TZJData v1.3.0+2024b
  [f269a46b] + TimeZones v1.19.0 `https://github.com/lcontento/TimeZones.jl#lc/relocatable`
  [0dad84c5] + ArgTools v1.1.2
  [56f22d72] + Artifacts v1.11.0
  [ade2ca70] + Dates v1.11.0
  [f43a241f] + Downloads v1.6.0
  [7b1f6079] + FileWatching v1.11.0
  [b27032c2] + LibCURL v0.6.4
  [8f399da3] + Libdl v1.11.0
  [ca575930] + NetworkOptions v1.2.0
  [de0858da] + Printf v1.11.0
  [9a3f8284] + Random v1.11.0
  [ea8e919c] + SHA v0.7.0
  [fa267f1f] + TOML v1.0.3
  [cf7118a7] + UUIDs v1.11.0
  [4ec0a83e] + Unicode v1.11.0
  [deac9b47] + LibCURL_jll v8.6.0+0
  [29816b5a] + LibSSH2_jll v1.11.0+1
  [c8ffd9c3] + MbedTLS_jll v2.28.6+0
  [14a3606d] + MozillaCACerts_jll v2023.12.12
  [83775a58] + Zlib_jll v1.2.13+1
  [8e850ede] + nghttp2_jll v1.59.0+0
  [3f19e933] + p7zip_jll v17.4.0+2
Precompiling project...
  9 dependencies successfully precompiled in 6 seconds. 14 already precompiled.
"/tmp/depot/artifacts/7fdea2a12522469ca39925546d1fd93c10748180"

❯ mv /tmp/depot /tmp/depot2

❯ JULIA_DEPOT_PATH=/tmp/depot2 julia -E 'using TimeZones; TimeZones._COMPILED_DIR[]'
"/tmp/depot2/artifacts/7fdea2a12522469ca39925546d1fd93c10748180"

Maybe there is something I'm forgetting about sysimages. Can you verify this works for your use case?

Copy link
Member

@omus omus Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was also working on an example:

docker run -it --entrypoint=/bin/bash julia:1.11.2

apt-get update -qq
apt-get install -qq build-essential

cat >demo.jl <<EOF
using TimeZones
TimeZone("America/Winnipeg")
EOF

julia -e '
    using Pkg
    Pkg.add([
        PackageSpec(name="PackageCompiler", version="1"),
        PackageSpec(url="https://github.com/lcontento/TimeZones.jl", rev="lc/relocatable"), 
        PackageSpec(name="TZJData", version="1.3.0"),
    ])
    '

julia -e '
    using PackageCompiler
    PackageCompiler.create_sysimage(["TimeZones"]; sysimage_path="demo-sysimage.so", precompile_execution_file="demo.jl")
    '

echo "With sysimage"
julia -Jdemo-sysimage.so --trace-compile=stderr demo.jl

rm -rf /root/.julia/packages /root/.julia/compiled
echo "With sysimage after delete"
julia -Jdemo-sysimage.so --trace-compile=stderr demo.jl

The system image no longer requires the package contents or the .ji to be present in the depot. However, the artifact must still exist. The solution I have in place with pkgdir does allow pkgdir to point to the location where the Artifact.toml would be but will fail when trying to load the file.

Update: I just realized I was using an old version of PackageCompiler.jl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect I'll roll back to using the hardcoded hashes but I'll do a little more experimentation first

Copy link
Member

@omus omus Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came up with an improved MWE:

rm -rf build_depot build_env sysimage_depot sysimage_env
mkdir build_depot build_env sysimage_env
JULIA_DEPOT_PATH=build_depot julia --project=sysimage_env -e '
    using Pkg
    Pkg.add([
        PackageSpec(name="TZJData", version=v"1.3.0"),
        PackageSpec(url="https://github.com/lcontento/TimeZones.jl", rev="lc/relocatable"),
    ])
    Pkg.precompile()'
JULIA_DEPOT_PATH=build_depot julia --project=build_env -e '
    using Pkg
    Pkg.add(PackageSpec(name="PackageCompiler", version="2"))
    Pkg.precompile()
    using PackageCompiler
    create_sysimage(; project="sysimage_env", sysimage_path="sysimage.so")'
JULIA_DEPOT_PATH=build_depot julia -Jsysimage.so --project=sysimage_env -E 'using TimeZones; TimeZones._COMPILED_DIR[]'
mv build_depot sysimage_depot
JULIA_DEPOT_PATH=sysimage_depot julia -Jsysimage.so --project=sysimage_env -E 'using TimeZones; TimeZones._COMPILED_DIR[]'

It looks like we can use Base.identify_package and Base.locate_package to get the package location with the activated sysimage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long does it take to build a sysimage that contains TimeZones.jl? If it just takes a few minutes, could we maybe add a sysimage test to CI, so that we can catch any relocatability regressions in the future?

We could check for ENV["CI"], so that people who run Pkg.test("TimeZones") locally don't have to run the test if they don't want to.

Copy link
Member

@omus omus Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up writing these tests directly in GHA. If I find this to be problematic I'll pull them out into the TimeZones tests directly in the future

hash = Base.SHA1(artifact_dict["tzjdata"]["git-tree-sha1"])
Artifacts.artifact_path(hash)
end
# COV_EXCL_STOP
Expand Down
Loading