From 80f0764556f5b00c50abecb8aa7838a609aaaf3b Mon Sep 17 00:00:00 2001 From: KristofferC Date: Mon, 26 Aug 2024 13:27:35 +0200 Subject: [PATCH 1/2] prevent loading other extensions when precompiling an extension 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 immidiately hit a cycle. The test added here checks this scenario and we can now precompile and load it without any warnings or issues. --- base/loading.jl | 16 +++++++------- base/precompilation.jl | 7 +++++-- test/loading.jl | 13 ++++++++++++ .../Extensions/CyclicExtensions/Manifest.toml | 21 +++++++++++++++++++ .../Extensions/CyclicExtensions/Project.toml | 13 ++++++++++++ .../Extensions/CyclicExtensions/ext/ExtA.jl | 6 ++++++ .../Extensions/CyclicExtensions/ext/ExtB.jl | 6 ++++++ .../CyclicExtensions/src/CyclicExtensions.jl | 7 +++++++ 8 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 test/project/Extensions/CyclicExtensions/Manifest.toml create mode 100644 test/project/Extensions/CyclicExtensions/Project.toml create mode 100644 test/project/Extensions/CyclicExtensions/ext/ExtA.jl create mode 100644 test/project/Extensions/CyclicExtensions/ext/ExtB.jl create mode 100644 test/project/Extensions/CyclicExtensions/src/CyclicExtensions.jl diff --git a/base/loading.jl b/base/loading.jl index 4dc735f0099d8..b73f61838f3de 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -1332,7 +1332,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 @@ -2805,7 +2807,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) @@ -2874,7 +2876,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.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)))) """) @@ -2931,18 +2933,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 @@ -2982,7 +2984,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 diff --git a/base/precompilation.jl b/base/precompilation.jl index 6997ce12c8d01..01865fceba9cd 100644 --- a/base/precompilation.jl +++ b/base/precompilation.jl @@ -471,7 +471,10 @@ function precompilepkgs(pkgs::Vector{String}=String[]; 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) + # extensions do not depend on each other + if !haskey(exts, extension_2) + push!(depsmap[extension_1], extension_2) + end end end end @@ -849,7 +852,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 diff --git a/test/loading.jl b/test/loading.jl index 51e0c45d2faf1..d9c60ee890324 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -1147,6 +1147,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) diff --git a/test/project/Extensions/CyclicExtensions/Manifest.toml b/test/project/Extensions/CyclicExtensions/Manifest.toml new file mode 100644 index 0000000000000..a506825cf7995 --- /dev/null +++ b/test/project/Extensions/CyclicExtensions/Manifest.toml @@ -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" diff --git a/test/project/Extensions/CyclicExtensions/Project.toml b/test/project/Extensions/CyclicExtensions/Project.toml new file mode 100644 index 0000000000000..08d539dcc40ae --- /dev/null +++ b/test/project/Extensions/CyclicExtensions/Project.toml @@ -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"] diff --git a/test/project/Extensions/CyclicExtensions/ext/ExtA.jl b/test/project/Extensions/CyclicExtensions/ext/ExtA.jl new file mode 100644 index 0000000000000..fa0c0961633cb --- /dev/null +++ b/test/project/Extensions/CyclicExtensions/ext/ExtA.jl @@ -0,0 +1,6 @@ +module ExtA + +using CyclicExtensions +using SomePackage + +end diff --git a/test/project/Extensions/CyclicExtensions/ext/ExtB.jl b/test/project/Extensions/CyclicExtensions/ext/ExtB.jl new file mode 100644 index 0000000000000..8f6da556d39b8 --- /dev/null +++ b/test/project/Extensions/CyclicExtensions/ext/ExtB.jl @@ -0,0 +1,6 @@ +module ExtB + +using CyclicExtensions +using SomePackage + +end diff --git a/test/project/Extensions/CyclicExtensions/src/CyclicExtensions.jl b/test/project/Extensions/CyclicExtensions/src/CyclicExtensions.jl new file mode 100644 index 0000000000000..f1c2ec2077562 --- /dev/null +++ b/test/project/Extensions/CyclicExtensions/src/CyclicExtensions.jl @@ -0,0 +1,7 @@ +module CyclicExtensions + +using ExtDep + +greet() = print("Hello Cycles!") + +end # module CyclicExtensions From a84c385b18c13b68d77f093fd68bbff4f6ea9ebe Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Tue, 24 Sep 2024 16:36:23 +0200 Subject: [PATCH 2/2] remove dead code --- base/precompilation.jl | 48 ------------------------------------------ 1 file changed, 48 deletions(-) diff --git a/base/precompilation.jl b/base/precompilation.jl index 01865fceba9cd..4899c1739c326 100644 --- a/base/precompilation.jl +++ b/base/precompilation.jl @@ -433,54 +433,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) - # extensions do not depend on each other - if !haskey(exts, extension_2) - push!(depsmap[extension_1], extension_2) - end - 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