Allow ext → ext dependency if triggers are a strict superset

topolarity · topolarity · commit f01c6e0aad1e · 2024-10-29T08:59:17.000-04:00
This allows for one extension to depend on another if its triggers are a strict superset of the other's. For example, in a Project.toml: ```toml [extensions] PlottingExt = "Plots" StatisticsPlottingExt = ["Plots", "Statistics"] ``` Here `StatisticsPlottingExt` is allowed to depend on `PlottingExt` This provides a way to declare `ext → ext` dependencies while still avoiding any extension cycles. The same trick can also be used to make an extension in one package depend on an extension provided in another. Also requires something like JuliaLang#49891, so that we guarantee these load in the right order.
diff --git a/NEWS.md b/NEWS.md
@@ -37,6 +37,10 @@ Language changes
   omit the default user depot ([#51448]).
 * Precompilation cache files are now relocatable and their validity is now verified through
   a content hash of their source files instead of their `mtime` ([#49866]).
+* Extensions may now depend on other extensions, if their triggers include all of the triggers
+  of any extension they wish to depend upon (+ at least one other trigger). In contrast to prior
+  versions, ext-to-ext dependencies that don't meet this requirement are now blocked during pre-
+  compilation to prevent extension cycles [#55557].
 
 Compiler/Runtime improvements
 -----------------------------
diff --git a/base/loading.jl b/base/loading.jl
@@ -1387,9 +1387,7 @@ function run_module_init(mod::Module, i::Int=1)
 end
 
 function run_package_callbacks(modkey::PkgId)
-    if !precompiling_extension
-        run_extension_callbacks(modkey)
-    end
+    run_extension_callbacks(modkey)
     assert_havelock(require_lock)
     unlock(require_lock)
     try
@@ -1527,6 +1525,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
 end
 
 loading_extension::Bool = false
+loadable_extensions::Union{Nothing,Vector{PkgId}} = nothing
 precompiling_extension::Bool = false
 function run_extension_callbacks(extid::ExtensionId)
     assert_havelock(require_lock)
@@ -1565,7 +1564,7 @@ function run_extension_callbacks(pkgid::PkgId)
         else
             succeeded = true
         end
-        if extid.ntriggers == 0
+        if extid.ntriggers == 0 && (loadable_extensions === nothing || extid.id in loadable_extensions)
             # actually load extid, now that all dependencies are met,
             # and record the result
             succeeded = succeeded && run_extension_callbacks(extid)
@@ -2870,7 +2869,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=``, cacheflags::CacheFlags=CacheFlags(),
-                           internal_stderr::IO = stderr, internal_stdout::IO = stdout, isext::Bool=false)
+                           internal_stderr::IO = stderr, internal_stdout::IO = stdout, loadable_exts::Union{Vector{PkgId},Nothing}=nothing)
     @nospecialize internal_stderr internal_stdout
     rm(output, force=true)   # Remove file if it exists
     output_o === nothing || rm(output_o, force=true)
@@ -2944,7 +2943,8 @@ 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 | isext)
+        Base.loadable_extensions = $(loadable_exts)
+        Base.precompiling_extension = $(loading_extension)
         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))))
         """)
@@ -3001,18 +3001,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}(), isext::Bool=false)
+function compilecache(pkg::PkgId, internal_stderr::IO = stderr, internal_stdout::IO = stdout; flags::Cmd=``, reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}(), loadable_exts::Union{Vector{PkgId},Nothing}=nothing)
     @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, isext)
+    return compilecache(pkg, path, internal_stderr, internal_stdout; flags, reasons, loadable_exts)
 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}(), isext::Bool=false)
+                      reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}(), loadable_exts::Union{Vector{PkgId},Nothing}=nothing)
 
     @nospecialize internal_stderr internal_stdout
     # decide where to put the resulting cache file
@@ -3052,7 +3052,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, cacheflags, internal_stderr, internal_stdout, isext)
+        p = create_expr_cache(pkg, path, tmppath, tmppath_o, concrete_deps, flags, cacheflags, internal_stderr, internal_stdout, loadable_exts)
 
         if success(p)
             if cache_objects
diff --git a/base/precompilation.jl b/base/precompilation.jl
@@ -419,6 +419,7 @@ function _precompilepkgs(pkgs::Vector{String},
         return name
     end
 
+    triggers = Dict{Base.PkgId,Vector{Base.PkgId}}()
     for (dep, deps) in env.deps
         pkg = Base.PkgId(dep, env.names[dep])
         Base.in_sysimage(pkg) && continue
@@ -427,25 +428,22 @@ function _precompilepkgs(pkgs::Vector{String},
         # add any extensions
         pkg_exts = Dict{Base.PkgId, Vector{Base.PkgId}}()
         for (ext_name, extdep_uuids) in env.extensions[dep]
-            ext_deps = Base.PkgId[]
-            push!(ext_deps, pkg) # depends on parent package
+            ext_uuid = Base.uuid5(pkg.uuid, ext_name)
+            ext = Base.PkgId(ext_uuid, ext_name)
+            triggers[ext] = Base.PkgId[pkg] # depends on parent package
             all_extdeps_available = true
             for extdep_uuid in extdep_uuids
                 extdep_name = env.names[extdep_uuid]
                 if extdep_uuid in keys(env.deps)
-                    push!(ext_deps, Base.PkgId(extdep_uuid, extdep_name))
+                    push!(triggers[ext], Base.PkgId(extdep_uuid, extdep_name))
                 else
                     all_extdeps_available = false
                     break
                 end
             end
             all_extdeps_available || continue
-            ext_uuid = Base.uuid5(pkg.uuid, ext_name)
-            ext = Base.PkgId(ext_uuid, ext_name)
-            filter!(!Base.in_sysimage, ext_deps)
-            depsmap[ext] = ext_deps
             exts[ext] = pkg.name
-            pkg_exts[ext] = ext_deps
+            pkg_exts[ext] = depsmap[ext] = filter(!Base.in_sysimage, triggers[ext])
         end
         if !isempty(pkg_exts)
             pkg_exts_map[pkg] = collect(keys(pkg_exts))
@@ -461,6 +459,16 @@ function _precompilepkgs(pkgs::Vector{String},
     append!(direct_deps, keys(filter(d->last(d) in keys(env.project_deps), exts)))
 
     @debug "precompile: deps collected"
+
+    # An extension effectively depends on another extension if it has a strict superset of its triggers
+    for ext_a in keys(exts)
+        for ext_b in keys(exts)
+            if triggers[ext_a] ⊋ triggers[ext_b]
+                push!(depsmap[ext_a], ext_b)
+            end
+        end
+    end
+
     # this loop must be run after the full depsmap has been populated
     for (pkg, pkg_exts) in pkg_exts_map
         # find any packages that depend on the extension(s)'s deps and replace those deps in their deps list with the extension(s),
@@ -817,7 +825,16 @@ function _precompilepkgs(pkgs::Vector{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, isext = haskey(exts, pkg))
+                                    keep_loaded_modules = false
+                                    if haskey(exts, pkg)
+                                        # any extension in our direct dependencies is one we have a right to load
+                                        loadable_exts = filter((dep)->haskey(exts, dep), depsmap[pkg])
+                                        Base.compilecache(pkg, sourcepath, std_pipe, std_pipe, keep_loaded_modules;
+                                                          flags, cacheflags, loadable_exts)
+                                    else
+                                        Base.compilecache(pkg, sourcepath, std_pipe, std_pipe, keep_loaded_modules;
+                                                          flags, cacheflags)
+                                    end
                                 end
                             end
                             if ret isa Base.PrecompilableError
