Skip to content

Commit

Permalink
[backports-release-1.11] Allow ext → ext dependency if triggers are a…
Browse files Browse the repository at this point in the history 
… strict superset (#56368)

This is an effort at a proper workaround (feature?) to resolve
#56204.

For example:
```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.

We'll also need to land #49891, so that we guarantee these load in the
right order.
  • Loading branch information
@topolarity
topolarity authored Oct 31, 2024
1 parent 23cb128 commit bf5675b
Show file tree
Hide file tree
Showing 22 changed files with 297 additions and 50 deletions.
4 changes: 4 additions & 0 deletions NEWS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 triggers of any
extension they wish to depend upon (+ at least one other trigger). Ext-to-ext dependencies
that don't meet this requirement are now blocked from using `Base.get_extension` during pre-
compilation, to prevent extension cycles [#55557].

Compiler/Runtime improvements
-----------------------------
Expand Down
75 changes: 39 additions & 36 deletions base/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -733,8 +733,9 @@ function manifest_uuid_path(env::String, pkg::PkgId)::Union{Nothing,String,Missi
proj = implicit_manifest_uuid_path(env, pkg)
proj === nothing || return proj
# if not found
parentid = get(EXT_PRIMED, pkg, nothing)
if parentid !== nothing
triggers = get(EXT_PRIMED, pkg, nothing)
if triggers !== nothing
parentid = triggers[1]
_, parent_project_file = entry_point_and_project_file(env, parentid.name)
if parent_project_file !== nothing
parentproj = project_file_name_uuid(parent_project_file, parentid.name)
Expand Down Expand Up @@ -1387,9 +1388,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
Expand Down Expand Up @@ -1418,7 +1417,7 @@ mutable struct ExtensionId
ntriggers::Int # how many more packages must be defined until this is loaded
end

const EXT_PRIMED = Dict{PkgId, PkgId}() # Extension -> Parent
const EXT_PRIMED = Dict{PkgId,Vector{PkgId}}() # Extension -> Parent + Triggers (parent is always first)
const EXT_DORMITORY = Dict{PkgId,Vector{ExtensionId}}() # Trigger -> Extensions that can be triggered by it
const EXT_DORMITORY_FAILED = ExtensionId[]

Expand Down Expand Up @@ -1509,14 +1508,15 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
if haskey(EXT_PRIMED, id) || haskey(Base.loaded_modules, id)
continue # extension is already primed or loaded, don't add it again
end
EXT_PRIMED[id] = parent
EXT_PRIMED[id] = trigger_ids = PkgId[parent]
gid = ExtensionId(id, parent, 1 + length(triggers), 1 + length(triggers))
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, parent)
push!(trigger1, gid)
for trigger in triggers
# TODO: Better error message if this lookup fails?
uuid_trigger = UUID(totaldeps[trigger]::String)
trigger_id = PkgId(uuid_trigger, trigger)
push!(trigger_ids, trigger_id)
if !haskey(explicit_loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id)
push!(trigger1, gid)
Expand All @@ -1528,6 +1528,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)
Expand Down Expand Up @@ -1558,7 +1559,7 @@ function run_extension_callbacks(pkgid::PkgId)
for extid in extids
@assert extid.ntriggers > 0
extid.ntriggers -= 1
if extid.ntriggers == 0
if extid.ntriggers == 0 && (loadable_extensions === nothing || extid.id in loadable_extensions)
push!(extids_to_load, extid)
end
end
Expand Down Expand Up @@ -2542,7 +2543,17 @@ function _require(pkg::PkgId, env=nothing)
# double-check the search now that we have lock
m = _require_search_from_serialized(pkg, path, UInt128(0), true)
m isa Module && return m
return compilecache(pkg, path; reasons)
triggers = get(EXT_PRIMED, pkg, nothing)
loadable_exts = nothing
if triggers !== nothing # extension
loadable_exts = PkgId[]
for (ext′, triggers′) in EXT_PRIMED
if triggers′ ⊊ triggers
push!(loadable_exts, ext′)
end
end
end
return compilecache(pkg, path; reasons, loadable_exts)
end
loaded isa Module && return loaded
if isnothing(loaded) # maybe_cachefile_lock returns nothing if it had to wait for another process
Expand Down Expand Up @@ -2865,19 +2876,26 @@ function check_package_module_loaded(pkg::PkgId)
return nothing
end

# protects against PkgId and UUID being imported and losing Base prefix
_pkg_str(_pkg::PkgId) = (_pkg.uuid === nothing) ? "Base.PkgId($(repr(_pkg.name)))" : "Base.PkgId(Base.UUID(\"$(_pkg.uuid)\"), $(repr(_pkg.name)))"
_pkg_str(_pkg::Vector) = sprint(show, eltype(_pkg); context = :module=>nothing) * "[" * join(map(_pkg_str, _pkg), ",") * "]"
_pkg_str(_pkg::Pair{PkgId}) = _pkg_str(_pkg.first) * " => " * repr(_pkg.second)
_pkg_str(_pkg::Nothing) = "nothing"

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)
depot_path = map(abspath, DEPOT_PATH)
dl_load_path = map(abspath, DL_LOAD_PATH)
load_path = map(abspath, Base.load_path())
# if pkg is a stdlib, append its parent Project.toml to the load path
parentid = get(EXT_PRIMED, pkg, nothing)
if parentid !== nothing
triggers = get(EXT_PRIMED, pkg, nothing)
if triggers !== nothing
parentid = triggers[1]
for env in load_path
project_file = env_project_file(env)
if project_file === true
Expand All @@ -2895,22 +2913,6 @@ function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::
any(path -> path_sep in path, load_path) &&
error("LOAD_PATH entries cannot contain $(repr(path_sep))")

deps_strs = String[]
# protects against PkgId and UUID being imported and losing Base prefix
function pkg_str(_pkg::PkgId)
if _pkg.uuid === nothing
"Base.PkgId($(repr(_pkg.name)))"
else
"Base.PkgId(Base.UUID(\"$(_pkg.uuid)\"), $(repr(_pkg.name)))"
end
end
for (pkg, build_id) in concrete_deps
push!(deps_strs, "$(pkg_str(pkg)) => $(repr(build_id))")
end
deps_eltype = sprint(show, eltype(concrete_deps); context = :module=>nothing)
deps = deps_eltype * "[" * join(deps_strs, ",") * "]"
precomp_stack = "Base.PkgId[$(join(map(pkg_str, vcat(Base.precompilation_stack, pkg)), ", "))]"

if output_o === nothing
# remove options that make no difference given the other cache options
cacheflags = CacheFlags(cacheflags, opt_level=0)
Expand Down Expand Up @@ -2941,10 +2943,11 @@ function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::
# write data over stdin to avoid the (unlikely) case of exceeding max command line size
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.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))))
Base.track_nested_precomp($(_pkg_str(vcat(Base.precompilation_stack, pkg))))
Base.loadable_extensions = $(_pkg_str(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)), $(_pkg_str(concrete_deps)), $(repr(source_path(nothing))))
""")
close(io.in)
return io
Expand Down Expand Up @@ -2999,18 +3002,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
Expand Down Expand Up @@ -3050,7 +3053,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
Expand Down
31 changes: 22 additions & 9 deletions base/precompilation.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
Expand All @@ -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),
Expand Down Expand Up @@ -817,7 +825,12 @@ 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
# for extensions, any extension in our direct dependencies is one we have a right to load
# for packages, we may load any extension (all possible triggers are accounted for above)
loadable_exts = haskey(exts, pkg) ? filter((dep)->haskey(exts, dep), depsmap[pkg]) : nothing
Base.compilecache(pkg, sourcepath, std_pipe, std_pipe, keep_loaded_modules;
flags, cacheflags, loadable_exts)
end
end
if ret isa Base.PrecompilableError
Expand Down
68 changes: 68 additions & 0 deletions test/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1158,6 +1158,74 @@ end
cmd = addenv(cmd, "JULIA_LOAD_PATH" => proj)
@test occursin("Hello Cycles!", String(read(cmd)))

# Extension-to-extension dependencies

mktempdir() do depot # Parallel pre-compilation
code = """
Base.disable_parallel_precompile = false
using ExtToExtDependency
Base.get_extension(ExtToExtDependency, :ExtA) isa Module || error("expected extension to load")
Base.get_extension(ExtToExtDependency, :ExtAB) isa Module || error("expected extension to load")
ExtToExtDependency.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "ExtToExtDependency")
cmd = `$(Base.julia_cmd()) --startup-file=no -e $code`
cmd = addenv(cmd,
"JULIA_LOAD_PATH" => proj,
"JULIA_DEPOT_PATH" => depot * Base.Filesystem.pathsep(),
)
@test occursin("Hello ext-to-ext!", String(read(cmd)))
end
mktempdir() do depot # Serial pre-compilation
code = """
Base.disable_parallel_precompile = true
using ExtToExtDependency
Base.get_extension(ExtToExtDependency, :ExtA) isa Module || error("expected extension to load")
Base.get_extension(ExtToExtDependency, :ExtAB) isa Module || error("expected extension to load")
ExtToExtDependency.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "ExtToExtDependency")
cmd = `$(Base.julia_cmd()) --startup-file=no -e $code`
cmd = addenv(cmd,
"JULIA_LOAD_PATH" => proj,
"JULIA_DEPOT_PATH" => depot * Base.Filesystem.pathsep(),
)
@test occursin("Hello ext-to-ext!", String(read(cmd)))
end

mktempdir() do depot # Parallel pre-compilation
code = """
Base.disable_parallel_precompile = false
using CrossPackageExtToExtDependency
Base.get_extension(CrossPackageExtToExtDependency.CyclicExtensions, :ExtA) isa Module || error("expected extension to load")
Base.get_extension(CrossPackageExtToExtDependency, :ExtAB) isa Module || error("expected extension to load")
CrossPackageExtToExtDependency.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "CrossPackageExtToExtDependency")
cmd = `$(Base.julia_cmd()) --startup-file=no -e $code`
cmd = addenv(cmd,
"JULIA_LOAD_PATH" => proj,
"JULIA_DEPOT_PATH" => depot * Base.Filesystem.pathsep(),
)
@test occursin("Hello x-package ext-to-ext!", String(read(cmd)))
end
mktempdir() do depot # Serial pre-compilation
code = """
Base.disable_parallel_precompile = true
using CrossPackageExtToExtDependency
Base.get_extension(CrossPackageExtToExtDependency.CyclicExtensions, :ExtA) isa Module || error("expected extension to load")
Base.get_extension(CrossPackageExtToExtDependency, :ExtAB) isa Module || error("expected extension to load")
CrossPackageExtToExtDependency.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "CrossPackageExtToExtDependency")
cmd = `$(Base.julia_cmd()) --startup-file=no -e $code`
cmd = addenv(cmd,
"JULIA_LOAD_PATH" => proj,
"JULIA_DEPOT_PATH" => depot * Base.Filesystem.pathsep(),
)
@test occursin("Hello x-package ext-to-ext!", String(read(cmd)))
end

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

julia_version = "1.11.1"
manifest_format = "2.0"
project_hash = "dc35c2cf8c6b82fb5b9624c9713c2df34ca30499"

[[deps.CyclicExtensions]]
deps = ["ExtDep"]
path = "../CyclicExtensions"
uuid = "17d4f0df-b55c-4714-ac4b-55fa23f7355c"
version = "0.1.0"
weakdeps = ["SomePackage"]

[deps.CyclicExtensions.extensions]
ExtA = ["SomePackage"]
ExtB = ["SomePackage"]

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

[[deps.SomeOtherPackage]]
path = "../SomeOtherPackage"
uuid = "178f68a2-4498-45ee-a775-452b36359b63"
version = "0.1.0"

[[deps.SomePackage]]
path = "../SomePackage"
uuid = "678608ae-7bb3-42c7-98b1-82102067a3d8"
version = "0.1.0"
12 changes: 12 additions & 0 deletions test/project/Extensions/CrossPackageExtToExtDependency/Project.toml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name = "CrossPackageExtToExtDependency"
uuid = "30f07f2e-c47e-40db-93a2-cbc4d1b301cc"
version = "0.1.0"

[deps]
CyclicExtensions = "17d4f0df-b55c-4714-ac4b-55fa23f7355c"

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

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

using CrossPackageExtToExtDependency
using SomePackage
using CyclicExtensions

const ExtA = Base.get_extension(CyclicExtensions, :ExtA)
if !(ExtA isa Module)
error("expected extension to load")
end

end
Loading

2 comments on commit bf5675b

@DilumAluthge
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runtests(["PackageCompiler"])

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package evaluation job you requested has completed - possible issues were detected.
The full report is available.

Please sign in to comment.