Skip to content

Commit

Permalink
prevent loading other extensions when precompiling an extension (#55589)
Browse files Browse the repository at this point in the history
The current way of loading extensions when precompiling an extension
very easily leads to cycles. For example, if you have more than one
extension and you happen to transitively depend on the triggers of one
of your extensions you will immediately hit a cycle where the extensions
will try to load each other indefinitely. This is an issue because you
cannot directly influence your transitive dependency graph so from this
p.o.v the current system of loading extension is "unsound".

The test added here checks this scenario and we can now precompile and
load it without any warnings or issues.

Would have made #55517 a non
issue.

Fixes #55557

---------

Co-authored-by: KristofferC <kristoffer.carlsson@juliacomputing.com>
  • Loading branch information
KristofferC and KristofferC authored Sep 30, 2024
1 parent 17445fe commit 4da0671
Show file tree
Hide file tree
Showing 8 changed files with 76 additions and 53 deletions.
16 changes: 9 additions & 7 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1387,7 +1387,9 @@ function run_module_init(mod::Module, i::Int=1)
end

function run_package_callbacks(modkey::PkgId)
run_extension_callbacks(modkey)
if !precompiling_extension
run_extension_callbacks(modkey)
end
assert_havelock(require_lock)
unlock(require_lock)
try
Expand Down Expand Up @@ -2843,7 +2845,7 @@ end

const PRECOMPILE_TRACE_COMPILE = Ref{String}()
function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::Union{Nothing, String},
concrete_deps::typeof(_concrete_dependencies), flags::Cmd=``, internal_stderr::IO = stderr, internal_stdout::IO = stdout)
concrete_deps::typeof(_concrete_dependencies), flags::Cmd=``, internal_stderr::IO = stderr, internal_stdout::IO = stdout, isext::Bool=false)
@nospecialize internal_stderr internal_stdout
rm(output, force=true) # Remove file if it exists
output_o === nothing || rm(output_o, force=true)
Expand Down Expand Up @@ -2912,7 +2914,7 @@ function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::
write(io.in, """
empty!(Base.EXT_DORMITORY) # If we have a custom sysimage with `EXT_DORMITORY` prepopulated
Base.track_nested_precomp($precomp_stack)
Base.precompiling_extension = $(loading_extension)
Base.precompiling_extension = $(loading_extension | isext)
Base.precompiling_package = true
Base.include_package_for_output($(pkg_str(pkg)), $(repr(abspath(input))), $(repr(depot_path)), $(repr(dl_load_path)),
$(repr(load_path)), $deps, $(repr(source_path(nothing))))
Expand Down Expand Up @@ -2970,18 +2972,18 @@ This can be used to reduce package load times. Cache files are stored in
`DEPOT_PATH[1]/compiled`. See [Module initialization and precompilation](@ref)
for important notes.
"""
function compilecache(pkg::PkgId, internal_stderr::IO = stderr, internal_stdout::IO = stdout; flags::Cmd=``, reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}())
function compilecache(pkg::PkgId, internal_stderr::IO = stderr, internal_stdout::IO = stdout; flags::Cmd=``, reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}(), isext::Bool=false)
@nospecialize internal_stderr internal_stdout
path = locate_package(pkg)
path === nothing && throw(ArgumentError("$(repr("text/plain", pkg)) not found during precompilation"))
return compilecache(pkg, path, internal_stderr, internal_stdout; flags, reasons)
return compilecache(pkg, path, internal_stderr, internal_stdout; flags, reasons, isext)
end

const MAX_NUM_PRECOMPILE_FILES = Ref(10)

function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, internal_stdout::IO = stdout,
keep_loaded_modules::Bool = true; flags::Cmd=``, cacheflags::CacheFlags=CacheFlags(),
reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}())
reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}(), isext::Bool=false)

@nospecialize internal_stderr internal_stdout
# decide where to put the resulting cache file
Expand Down Expand Up @@ -3021,7 +3023,7 @@ function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, in
close(tmpio_o)
close(tmpio_so)
end
p = create_expr_cache(pkg, path, tmppath, tmppath_o, concrete_deps, flags, internal_stderr, internal_stdout)
p = create_expr_cache(pkg, path, tmppath, tmppath_o, concrete_deps, flags, internal_stderr, internal_stdout, isext)

if success(p)
if cache_objects
Expand Down
47 changes: 1 addition & 46 deletions base/precompilation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -435,51 +435,6 @@ function precompilepkgs(pkgs::Vector{String}=String[];
# consider exts of direct deps to be direct deps so that errors are reported
append!(direct_deps, keys(filter(d->last(d) in keys(env.project_deps), exts)))

# An extension effectively depends on another extension if it has all the the
# dependencies of that other extension
function expand_dependencies(depsmap)
function visit!(visited, node, all_deps)
if node in visited
return
end
push!(visited, node)
for dep in get(Set{Base.PkgId}, depsmap, node)
if !(dep in all_deps)
push!(all_deps, dep)
visit!(visited, dep, all_deps)
end
end
end

depsmap_transitive = Dict{Base.PkgId, Set{Base.PkgId}}()
for package in keys(depsmap)
# Initialize a set to keep track of all dependencies for 'package'
all_deps = Set{Base.PkgId}()
visited = Set{Base.PkgId}()
visit!(visited, package, all_deps)
# Update depsmap with the complete set of dependencies for 'package'
depsmap_transitive[package] = all_deps
end
return depsmap_transitive
end

depsmap_transitive = expand_dependencies(depsmap)

for (_, extensions_1) in pkg_exts_map
for extension_1 in extensions_1
deps_ext_1 = depsmap_transitive[extension_1]
for (_, extensions_2) in pkg_exts_map
for extension_2 in extensions_2
extension_1 == extension_2 && continue
deps_ext_2 = depsmap_transitive[extension_2]
if issubset(deps_ext_2, deps_ext_1)
push!(depsmap[extension_1], extension_2)
end
end
end
end
end

@debug "precompile: deps collected"
# this loop must be run after the full depsmap has been populated
for (pkg, pkg_exts) in pkg_exts_map
Expand Down Expand Up @@ -852,7 +807,7 @@ function precompilepkgs(pkgs::Vector{String}=String[];
t = @elapsed ret = precompile_pkgs_maybe_cachefile_lock(io, print_lock, fancyprint, pkg_config, pkgspidlocked, hascolor) do
Base.with_logger(Base.NullLogger()) do
# The false here means we ignore loaded modules, so precompile for a fresh session
Base.compilecache(pkg, sourcepath, std_pipe, std_pipe, false; flags, cacheflags)
Base.compilecache(pkg, sourcepath, std_pipe, std_pipe, false; flags, cacheflags, isext = haskey(exts, pkg))
end
end
if ret isa Base.PrecompilableError
Expand Down
13 changes: 13 additions & 0 deletions test/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,19 @@ end
finally
copy!(LOAD_PATH, old_load_path)
end

# Extension with cycles in dependencies
code = """
using CyclicExtensions
Base.get_extension(CyclicExtensions, :ExtA) isa Module || error("expected extension to load")
Base.get_extension(CyclicExtensions, :ExtB) isa Module || error("expected extension to load")
CyclicExtensions.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "CyclicExtensions")
cmd = `$(Base.julia_cmd()) --startup-file=no -e $code`
cmd = addenv(cmd, "JULIA_LOAD_PATH" => proj)
@test occursin("Hello Cycles!", String(read(cmd)))

finally
try
rm(depot_path, force=true, recursive=true)
Expand Down
21 changes: 21 additions & 0 deletions test/project/Extensions/CyclicExtensions/Manifest.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# This file is machine-generated - editing it directly is not advised

julia_version = "1.10.4"
manifest_format = "2.0"
project_hash = "ec25ff8df3a5e2212a173c3de2c7d716cc47cd36"

[[deps.ExtDep]]
deps = ["SomePackage"]
path = "../ExtDep.jl"
uuid = "fa069be4-f60b-4d4c-8b95-f8008775090c"
version = "0.1.0"

[[deps.ExtDep2]]
path = "../ExtDep2"
uuid = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d"
version = "0.1.0"

[[deps.SomePackage]]
path = "../SomePackage"
uuid = "678608ae-7bb3-42c7-98b1-82102067a3d8"
version = "0.1.0"
13 changes: 13 additions & 0 deletions test/project/Extensions/CyclicExtensions/Project.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
name = "CyclicExtensions"
uuid = "17d4f0df-b55c-4714-ac4b-55fa23f7355c"
version = "0.1.0"

[deps]
ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c"

[weakdeps]
SomePackage = "678608ae-7bb3-42c7-98b1-82102067a3d8"

[extensions]
ExtA = ["SomePackage"]
ExtB = ["SomePackage"]
6 changes: 6 additions & 0 deletions test/project/Extensions/CyclicExtensions/ext/ExtA.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module ExtA

using CyclicExtensions
using SomePackage

end
6 changes: 6 additions & 0 deletions test/project/Extensions/CyclicExtensions/ext/ExtB.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module ExtB

using CyclicExtensions
using SomePackage

end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module CyclicExtensions

using ExtDep

greet() = print("Hello Cycles!")

end # module CyclicExtensions

0 comments on commit 4da0671

Please sign in to comment.