Avoid repeated precompilation when loading from non-relocatable cache…

…files (#53905) Fixes #53859 (comment), which was actually fixed before in #52346, but #52750 undid that fix. This PR does not directly address #53859, because I can not reproduce it atm. --- The `@depot` resolution logic for `include()` files is adjusted as follows: 1. (new behavior) If the cache is not relocatable because of an absolute path, we ignore that path for the depot search. Recompilation will be triggered by `stale_cachefile()` if that absolute path does not exist. Previously this caused any `@depot` tags to be not resolved and so trigger recompilation. 2. (new behavior) If we can't find a depot for a relocatable path, we still replace it with the depot we found from other files. Recompilation will be triggered by `stale_cachefile()` because the resolved path does not exist. 3. (this behavior is kept) We require that relocatable paths all resolve to the same depot. 4. (new behavior) We no longer use the first matching depot for replacement, but instead we explicitly check that all resolve to the same depot. This has two reasons: - We want to scan all source files anyways in order to provide logs for 1. and 2. above, so the check is free. - It is possible that a depot might be missing source files. Assume that we have two depots on `DEPOT_PATH`, `depot_complete` and `depot_incomplete`. If `DEPOT_PATH=["depot_complete","depot_incomplete"]` then no recompilation shall happen, because `depot_complete` will be picked. If `DEPOT_PATH=["depot_incomplete","depot_complete"]` we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves to `depot_complete` or `depot_incomplete`. (cherry picked from commit d8d3842)
JuliaLang · Apr 9, 2024 · 967e2f0 · 967e2f0
1 parent b0d7ee4
commit 967e2f0
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 16 deletions.
diff --git a/base/loading.jl b/base/loading.jl
@@ -3160,31 +3160,71 @@ function parse_cache_header(f::IO, cachefile::AbstractString)
 
     includes_srcfiles = CacheHeaderIncludes[]
     includes_depfiles = CacheHeaderIncludes[]
-    for (i, inc) in enumerate(includes)
+    for inc in includes
         if inc.filename ∈ srcfiles
             push!(includes_srcfiles, inc)
         else
             push!(includes_depfiles, inc)
         end
     end
 
-    # determine depot for @depot replacement for include() files and include_dependency() files separately
-    srcfiles_depot = resolve_depot(first(srcfiles))
-    if srcfiles_depot === :no_depot_found
-        @debug("Unable to resolve @depot tag include() files from cache file $cachefile", srcfiles)
-    elseif srcfiles_depot === :not_relocatable
-        @debug("include() files from $cachefile are not relocatable", srcfiles)
-    else
+
+    # The @depot resolution logic for include() files:
+    # 1. If the cache is not relocatable because of an absolute path,
+    #    we ignore that path for the depot search.
+    #    Recompilation will be triggered by stale_cachefile() if that absolute path does not exist.
+    # 2. If we can't find a depot for a relocatable path,
+    #    we still replace it with the depot we found from other files.
+    #    Recompilation will be triggered by stale_cachefile() because the resolved path does not exist.
+    # 3. We require that relocatable paths all resolve to the same depot.
+    # 4. We explicitly check that all relocatable paths resolve to the same depot. This has two reasons:
+    #    - We want to scan all source files in order to provide logs for 1. and 2. above.
+    #    - It is possible that a depot might be missing source files.
+    #      Assume that we have two depots on DEPOT_PATH, depot_complete and depot_incomplete.
+    #      If DEPOT_PATH=["depot_complete","depot_incomplete"] then no recompilation shall happen,
+    #      because depot_complete will be picked.
+    #      If DEPOT_PATH=["depot_incomplete","depot_complete"] we trigger recompilation and
+    #      hopefully a meaningful error about missing files is thrown.
+    #      If we were to just select the first depot we find, then whether recompilation happens would
+    #      depend on whether the first relocatable file resolves to depot_complete or depot_incomplete.
+    srcdepot = nothing
+    any_not_relocatable = false
+    any_no_depot_found = false
+    multiple_depots_found = false
+    for src in srcfiles
+        depot = resolve_depot(src)
+        if depot === :not_relocatable
+            any_not_relocatable = true
+        elseif depot === :no_depot_found
+            any_no_depot_found = true
+        elseif isnothing(srcdepot)
+            srcdepot = depot
+        elseif depot != srcdepot
+            multiple_depots_found = true
+        end
+    end
+    if any_no_depot_found
+        @debug("Unable to resolve @depot tag for at least one include() file from cache file $cachefile", srcfiles, _group=:relocatable)
+    end
+    if any_not_relocatable
+        @debug("At least one include() file from $cachefile is not relocatable", srcfiles, _group=:relocatable)
+    end
+    if multiple_depots_found
+        @debug("Some include() files from $cachefile are distributed over multiple depots", srcfiles, _group=:relocatable)
+    elseif !isnothing(srcdepot)
         for inc in includes_srcfiles
-            inc.filename = restore_depot_path(inc.filename, srcfiles_depot)
+            inc.filename = restore_depot_path(inc.filename, srcdepot)
         end
     end
+
+    # unlike include() files, we allow each relocatable include_dependency() file to resolve
+    # to a separate depot, #52161
     for inc in includes_depfiles
         depot = resolve_depot(inc.filename)
         if depot === :no_depot_found
-            @debug("Unable to resolve @depot tag for include_dependency() file $(inc.filename) from cache file $cachefile", srcfiles)
+            @debug("Unable to resolve @depot tag for include_dependency() file $(inc.filename) from cache file $cachefile", _group=:relocatable)
         elseif depot === :not_relocatable
-            @debug("include_dependency() file $(inc.filename) from $cachefile is not relocatable", srcfiles)
+            @debug("include_dependency() file $(inc.filename) from $cachefile is not relocatable", _group=:relocatable)
         else
             inc.filename = restore_depot_path(inc.filename, depot)
         end

diff --git a/test/.gitignore b/test/.gitignore
@@ -4,3 +4,4 @@
 /libccalltest.*
 /relocatedepot
 /RelocationTestPkg2/src/foo.txt
+/RelocationTestPkg*/Manifest.toml
diff --git a/test/Makefile b/test/Makefile
@@ -43,6 +43,7 @@ relocatedepot:
 	@cp -R $(SRCDIR)/RelocationTestPkg1 $(SRCDIR)/relocatedepot
 	@cp -R $(SRCDIR)/RelocationTestPkg2 $(SRCDIR)/relocatedepot
 	@cp -R $(SRCDIR)/RelocationTestPkg3 $(SRCDIR)/relocatedepot
+	@cp -R $(SRCDIR)/RelocationTestPkg4 $(SRCDIR)/relocatedepot
 	@cd $(SRCDIR) && \
 	$(call PRINT_JULIA, $(call spawn,RELOCATEDEPOT="" $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl $@)
 
@@ -55,6 +56,7 @@ revise-relocatedepot: revise-% :
 	@cp -R $(SRCDIR)/RelocationTestPkg1 $(SRCDIR)/relocatedepot
 	@cp -R $(SRCDIR)/RelocationTestPkg2 $(SRCDIR)/relocatedepot
 	@cp -R $(SRCDIR)/RelocationTestPkg3 $(SRCDIR)/relocatedepot
+	@cp -R $(SRCDIR)/RelocationTestPkg4 $(SRCDIR)/relocatedepot
 	@cd $(SRCDIR) && \
 	$(call PRINT_JULIA, $(call spawn,RELOCATEDEPOT="" $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl --revise $*)
 

diff --git a/test/RelocationTestPkg4/Project.toml b/test/RelocationTestPkg4/Project.toml
@@ -0,0 +1,6 @@
+name = "RelocationTestPkg4"
+uuid = "d423d817-d7e9-49ac-b245-9d9d6db0b429"
+version = "0.1.0"
+
+[deps]
+RelocationTestPkg1 = "854e1adb-5a97-46bf-a391-1cfe05ac726d"
diff --git a/test/RelocationTestPkg4/src/RelocationTestPkg4.jl b/test/RelocationTestPkg4/src/RelocationTestPkg4.jl
@@ -0,0 +1,5 @@
+module RelocationTestPkg4
+
+greet() = print("Hello World!")
+
+end # module RelocationTestPkg4
diff --git a/test/relocatedepot.jl b/test/relocatedepot.jl
@@ -17,10 +17,12 @@ function test_harness(@nospecialize(fn); empty_load_path=true, empty_depot_path=
     end
 end
 
-# We test relocation with three dummy pkgs:
-# - RelocationTestPkg1 - no include_dependency
-# - RelocationTestPkg2 - with include_dependency tracked by `mtime`
-# - RelocationTestPkg3 - with include_dependency tracked by content
+# We test relocation with these dummy pkgs:
+# - RelocationTestPkg1 - pkg with no include_dependency
+# - RelocationTestPkg2 - pkg with include_dependency tracked by `mtime`
+# - RelocationTestPkg3 - pkg with include_dependency tracked by content
+# - RelocationTestPkg4 - pkg with no dependencies; will be compiled such that the pkgimage is
+#                        not relocatable, but no repeated recompilation happens upon loading
 
 if !test_relocated_depot
 
@@ -123,6 +125,27 @@ if !test_relocated_depot
         end
     end
 
+    @testset "precompile RelocationTestPkg4" begin
+        # test for #52346 and https://github.com/JuliaLang/julia/issues/53859#issuecomment-2027352004
+        # If a pkgimage is not relocatable, no repeated precompilation should occur.
+        pkgname = "RelocationTestPkg4"
+        test_harness(empty_depot_path=false) do
+            push!(LOAD_PATH, @__DIR__)
+            # skip this dir to make the pkgimage not relocatable
+            filter!(DEPOT_PATH) do depot
+                !startswith(@__DIR__, depot)
+            end
+            pkg = Base.identify_package(pkgname)
+            cachefiles = Base.find_all_in_cache_path(pkg)
+            rm.(cachefiles, force=true)
+            @test Base.isprecompiled(pkg) == false
+            @test Base.isrelocatable(pkg) == false # because not precompiled
+            Base.require(pkg)
+            @test Base.isprecompiled(pkg, ignore_loaded=true) == true
+            @test Base.isrelocatable(pkg) == false
+        end
+    end
+
     @testset "#52161" begin
         # Take the src files from two pkgs Example1 and Example2,
         # which are each located in depot1 and depot2, respectively, and
@@ -246,7 +269,7 @@ else
         pkgname = "RelocationTestPkg3"
         test_harness() do
             push!(LOAD_PATH, joinpath(@__DIR__, "relocatedepot"))
-            push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot"))
+            push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot")) # required to find src files
             push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot", "julia")) # contains cache file
             pkg = Base.identify_package(pkgname)
             @test Base.isprecompiled(pkg) == true
@@ -257,4 +280,17 @@ else
         end
     end
 
+    @testset "load RelocationTestPkg4 from test/relocatedepot" begin
+        pkgname = "RelocationTestPkg4"
+        test_harness() do
+            push!(LOAD_PATH, @__DIR__, "relocatedepot")
+            push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot")) # required to find src files
+            push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot", "julia")) # contains cache file
+            pkg = Base.identify_package(pkgname)
+            # precompiled but not relocatable
+            @test Base.isprecompiled(pkg) == true
+            @test Base.isrelocatable(pkg) == false
+        end
+    end
+
 end