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

Jump to bottom

Allow include() and include_dependency() files to resolve to different depots #52750

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Allow include() and include_dependency() files to resolve to different depots #52750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74301e4
remove test case that was made obsolote by #51989
fatteneder Jan 4, 2024
b7266a8
refactor @depot tag resolution logic
fatteneder Jan 4, 2024
84a4772
test: non-existent include_dependency() files are not relocatable
fatteneder Jan 6, 2024
edb66b8
add test for #52161
fatteneder Jan 6, 2024
7e0e7d4
update test to use temporarily generated Example pkgs to avoid
fatteneder Jan 7, 2024
d29b365
relocation test: tune load_path/depot_path setup to better exercise f…
fatteneder Jan 7, 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
60 changes: 32 additions & 28 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2778,22 +2778,10 @@ function restore_depot_path(path::AbstractString, depot::AbstractString)
replace(path, r"^@depot" => depot; count=1)
end

# Find depot in DEPOT_PATH for which all @depot tags from the `includes`
# can be replaced so that they point to a file on disk each.
function resolve_depot(includes::Union{AbstractVector,AbstractSet})
# `all` because it's possible to have a mixture of includes inside and outside of the depot
if all(includes) do inc
!startswith(inc, "@depot")
end
return :fully_outside_depot
end
function resolve_depot(inc::AbstractString)
startswith(inc, "@depot") || return :not_relocatable
for depot in DEPOT_PATH
# `any` because it's possible to have a mixture of includes inside and outside of the depot
if any(includes) do inc
isfile(restore_depot_path(inc, depot))
end
return depot
end
isfile(restore_depot_path(inc, depot)) && return depot
end
return :no_depot_found
end
Expand Down Expand Up @@ -2881,25 +2869,41 @@ function parse_cache_header(f::IO, cachefile::AbstractString)
l = read(f, Int32)
clone_targets = read(f, l)

# determine path for @depot replacement from srctext files only, e.g. ignore any include_dependency files
srcfiles = srctext_files(f, srctextpos, includes)
depot = resolve_depot(srcfiles)
keepidx = Int[]
for (i, chi) in enumerate(includes)
chi.filename ∈ srcfiles && push!(keepidx, i)
end
if depot === :no_depot_found
@debug("Unable to resolve @depot tag in cache file $cachefile", srcfiles)
elseif depot === :fully_outside_depot
@debug("All include dependencies in cache file $cachefile are outside of a depot.", srcfiles)

includes_srcfiles = CacheHeaderIncludes[]
includes_depfiles = CacheHeaderIncludes[]
for (i, inc) in enumerate(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
for inc in includes
for inc in includes_srcfiles
inc.filename = restore_depot_path(inc.filename, srcfiles_depot)
end
end
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)
elseif depot === :not_relocatable
@debug("include_dependency() file $(inc.filename) from $cachefile is not relocatable", srcfiles)
else
inc.filename = restore_depot_path(inc.filename, depot)
end
end
includes_srcfiles_only = includes[keepidx]

return modules, (includes, includes_srcfiles_only, requires), required_modules, srctextpos, prefs, prefs_hash, clone_targets, flags
return modules, (includes, includes_srcfiles, requires), required_modules, srctextpos, prefs, prefs_hash, clone_targets, flags
end

function parse_cache_header(cachefile::String)
Expand Down
4 changes: 2 additions & 2 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ relocatedepot:
@cp -R $(SRCDIR)/RelocationTestPkg1 $(SRCDIR)/relocatedepot
@cp -R $(SRCDIR)/RelocationTestPkg2 $(SRCDIR)/relocatedepot
@cd $(SRCDIR) && \
$(call PRINT_JULIA, $(call spawn,JULIA_DEBUG=loading RELOCATEDEPOT="" JULIA_DEPOT_PATH=$(SRCDIR)/relocatedepot/julia $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl $@)
$(call PRINT_JULIA, $(call spawn,JULIA_DEBUG=loading RELOCATEDEPOT="" $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl $@)

revise-relocatedepot: revise-% :
@rm -rf $(SRCDIR)/relocatedepot
Expand All @@ -54,7 +54,7 @@ revise-relocatedepot: revise-% :
@cp -R $(SRCDIR)/RelocationTestPkg1 $(SRCDIR)/relocatedepot
@cp -R $(SRCDIR)/RelocationTestPkg2 $(SRCDIR)/relocatedepot
@cd $(SRCDIR) && \
$(call PRINT_JULIA, $(call spawn,JULIA_DEBUG=loading RELOCATEDEPOT="" JULIA_DEPOT_PATH=$(SRCDIR)/relocatedepot/julia $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl --revise $*)
$(call PRINT_JULIA, $(call spawn,JULIA_DEBUG=loading RELOCATEDEPOT="" $(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no --depwarn=error ./runtests.jl --revise $*)

embedding:
@$(MAKE) -C $(SRCDIR)/$@ check $(EMBEDDING_ARGS)
Expand Down
3 changes: 2 additions & 1 deletion test/precompile.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,8 @@ precompile_test_harness(false) do dir
modules, (deps, _, requires), required_modules, _... = Base.parse_cache_header(cachefile)
discard_module = mod_fl_mt -> mod_fl_mt.filename
@test modules == [ Base.PkgId(Foo) => Base.module_build_id(Foo) % UInt64 ]
@test map(x -> x.filename, deps) == [ Foo_file, joinpath(dir, "foo.jl"), joinpath(dir, "bar.jl") ]
# foo.jl and bar.jl are never written to disk, so they are not relocatable
@test map(x -> x.filename, deps) == [ Foo_file, joinpath("@depot", "foo.jl"), joinpath("@depot", "bar.jl") ]
@test requires == [ Base.PkgId(Foo) => Base.PkgId(string(FooBase_module)),
Base.PkgId(Foo) => Base.PkgId(Foo2),
Base.PkgId(Foo) => Base.PkgId(Test),
Expand Down
113 changes: 92 additions & 21 deletions test/relocatedepot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ using Logging
include("testenv.jl")


function test_harness(@nospecialize(fn))
function test_harness(@nospecialize(fn); empty_load_path=true, empty_depot_path=true)
load_path = copy(LOAD_PATH)
depot_path = copy(DEPOT_PATH)
try
empty_load_path && empty!(LOAD_PATH)
empty_depot_path && empty!(DEPOT_PATH)
fn()
finally
copy!(LOAD_PATH, load_path)
Expand Down Expand Up @@ -66,9 +68,9 @@ if !test_relocated_depot

@testset "precompile RelocationTestPkg1" begin
pkgname = "RelocationTestPkg1"
test_harness() do
test_harness(empty_depot_path=false) do
push!(LOAD_PATH, @__DIR__)
push!(DEPOT_PATH, @__DIR__)
push!(DEPOT_PATH, @__DIR__) # required to make relocatable, but cache is written to DEPOT_PATH[1]
pkg = Base.identify_package(pkgname)
cachefiles = Base.find_all_in_cache_path(pkg)
rm.(cachefiles, force=true)
Expand All @@ -80,9 +82,9 @@ if !test_relocated_depot

@testset "precompile RelocationTestPkg2 (contains include_dependency)" begin
pkgname = "RelocationTestPkg2"
test_harness() do
test_harness(empty_depot_path=false) do
push!(LOAD_PATH, @__DIR__)
push!(DEPOT_PATH, string(@__DIR__, "/"))
push!(DEPOT_PATH, @__DIR__) # required to make relocatable, but cache is written to DEPOT_PATH[1]
pkg = Base.identify_package(pkgname)
cachefiles = Base.find_all_in_cache_path(pkg)
rm.(cachefiles, force=true)
Expand All @@ -93,27 +95,94 @@ if !test_relocated_depot
end
end

else

# must come before any of the load tests, because the will recompile and generate new cache files
@testset "attempt loading precompiled pkgs when depot is missing" begin
@testset "#52161" begin
# Take the src files from two pkgs Example1 and Example2,
# which are each located in depot1 and depot2, respectively, and
# add them as include_dependency()s to a new pkg Foo, which will be precompiled into depot3.
# After loading the include_dependency()s of Foo should refer to depot1 depot2 each.
test_harness() do
empty!(LOAD_PATH)
push!(LOAD_PATH, joinpath(@__DIR__, "relocatedepot"))
for pkgname in ("RelocationTestPkg1", "RelocationTestPkg2")
pkg = Base.identify_package(pkgname)
cachefile = only(Base.find_all_in_cache_path(pkg))
@test_throws ArgumentError("""
Failed to determine depot from srctext files in cache file $cachefile.
- Make sure you have adjusted DEPOT_PATH in case you relocated depots.""") Base.isprecompiled(pkg)
mktempdir() do depot1
# precompile Example in depot1
example1_root = joinpath(depot1, "Example1")
mkpath(joinpath(example1_root, "src"))
open(joinpath(example1_root, "src", "Example1.jl"); write=true) do io
println(io, """
module Example1
greet() = println("Hello from Example1!")
end
""")
end
open(joinpath(example1_root, "Project.toml"); write=true) do io
println(io, """
name = "Example1"
uuid = "00000000-0000-0000-0000-000000000001"
version = "1.0.0"
""")
end
pushfirst!(LOAD_PATH, depot1); pushfirst!(DEPOT_PATH, depot1)
pkg = Base.identify_package("Example1"); Base.require(pkg)
mktempdir() do depot2
# precompile Example in depot2
example2_root = joinpath(depot2, "Example2")
mkpath(joinpath(example2_root, "src"))
open(joinpath(example2_root, "src", "Example2.jl"); write=true) do io
println(io, """
module Example2
greet() = println("Hello from Example2!")
end
""")
end
open(joinpath(example2_root, "Project.toml"); write=true) do io
println(io, """
name = "Example2"
uuid = "00000000-0000-0000-0000-000000000002"
version = "1.0.0"
""")
end
pushfirst!(LOAD_PATH, depot2)
pushfirst!(DEPOT_PATH, depot2)
pkg = Base.identify_package("Example2")
Base.require(pkg)
mktempdir() do depot3
# precompile Foo in depot3
open(joinpath(depot3, "Foo.jl"), write=true) do io
println(io, """
module Foo
using Example1
using Example2
srcfile1 = joinpath(pkgdir(Example1), "src", "Example1.jl")
srcfile2 = joinpath(pkgdir(Example2), "src", "Example2.jl")
@show srcfile1
@show srcfile2
Comment on lines +155 to +156
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these @show's necessary? They're spamming all tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are these @show's necessary?

No.

include_dependency(srcfile1)
include_dependency(srcfile2)
end
""")
end
pushfirst!(LOAD_PATH, depot3)
pushfirst!(DEPOT_PATH, depot3)
pkg = Base.identify_package("Foo")
Base.require(pkg)
cachefile = joinpath(depot3, "compiled",
"v$(VERSION.major).$(VERSION.minor)", "Foo.ji")
_, (deps, _, _), _... = Base.parse_cache_header(cachefile)
@test map(x -> x.filename, deps) ==
[ joinpath(depot3, "Foo.jl"),
joinpath(depot1, "Example1", "src", "Example1.jl"),
joinpath(depot2, "Example2", "src", "Example2.jl") ]
end
end
end
end
end


else

@testset "load stdlib from test/relocatedepot" begin
test_harness() do
push!(LOAD_PATH, joinpath(@__DIR__, "relocatedepot"))
push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot"))
push!(LOAD_PATH, "@stdlib")
push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot", "julia"))
# stdlib should be already precompiled
pkg = Base.identify_package("DelimitedFiles")
@test Base.isprecompiled(pkg) == true
Expand All @@ -124,7 +193,8 @@ else
pkgname = "RelocationTestPkg1"
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
Base.require(pkg) # re-precompile
Expand All @@ -136,7 +206,8 @@ else
pkgname = "RelocationTestPkg2"
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) == false # moving depot changes mtime of include_dependency
Base.require(pkg) # re-precompile
Expand Down