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.

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

Revert "Extensions: make loading of extensions independent of what packages are in the sysimage (#52841) #56234

Merged
merged 3 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion base/Base.jl
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,6 @@ function __init__()
init_load_path()
init_active_project()
append!(empty!(_sysimage_modules), keys(loaded_modules))
empty!(explicit_loaded_modules)
empty!(loaded_precompiles) # If we load a packageimage when building the image this might not be empty
for (mod, key) in module_keys
push!(get!(Vector{Module}, loaded_precompiles, key), mod)
Expand Down
49 changes: 23 additions & 26 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -974,14 +974,14 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St
entry = entry::Dict{String, Any}
uuid = get(entry, "uuid", nothing)::Union{String, Nothing}
uuid === nothing && continue
# deps is either a list of names (deps = ["DepA", "DepB"]) or
# a table of entries (deps = {"DepA" = "6ea...", "DepB" = "55d..."}
deps = get(entry, "deps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing}
Copy link
Member

Choose a reason for hiding this comment

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

Is this deps lookup fix related? Does it need a test to match?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is partly related since this runs into #55939 due to how the manifest change with the revert. So it does in some sense already has a test.

if UUID(uuid) === where.uuid
found_where = true
# deps is either a list of names (deps = ["DepA", "DepB"]) or
# a table of entries (deps = {"DepA" = "6ea...", "DepB" = "55d..."}
deps = get(entry, "deps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing}
if deps isa Vector{String}
found_name = name in deps
break
found_name && @goto done
elseif deps isa Dict{String, Any}
deps = deps::Dict{String, Any}
for (dep, uuid) in deps
Expand All @@ -1000,30 +1000,33 @@ function explicit_manifest_deps_get(project_file::String, where::PkgId, name::St
return PkgId(UUID(uuid), name)
end
exts = extensions[where.name]::Union{String, Vector{String}}
weakdeps = get(entry, "weakdeps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing}
if (exts isa String && name == exts) || (exts isa Vector{String} && name in exts)
weakdeps = get(entry, "weakdeps", nothing)::Union{Vector{String}, Dict{String, Any}, Nothing}
if weakdeps !== nothing
if weakdeps isa Vector{String}
found_name = name in weakdeps
break
elseif weakdeps isa Dict{String, Any}
weakdeps = weakdeps::Dict{String, Any}
for (dep, uuid) in weakdeps
uuid::String
if dep === name
return PkgId(UUID(uuid), name)
for deps′ in [weakdeps, deps]
if deps′ !== nothing
if deps′ isa Vector{String}
found_name = name in deps′
found_name && @goto done
elseif deps′ isa Dict{String, Any}
deps′ = deps′::Dict{String, Any}
for (dep, uuid) in deps′
uuid::String
if dep === name
return PkgId(UUID(uuid), name)
end
end
end
end
end
end
end
# `name` is not an ext, do standard lookup as if this was the parent
return identify_package(PkgId(UUID(uuid), dep_name), name)
end
end
end
end
end
@label done
found_where || return nothing
found_name || return PkgId(name)
# Only reach here if deps was not a dict which mean we have a unique name for the dep
Expand Down Expand Up @@ -1554,7 +1557,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
# TODO: Better error message if this lookup fails?
uuid_trigger = UUID(totaldeps[trigger]::String)
trigger_id = PkgId(uuid_trigger, trigger)
if !haskey(explicit_loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id)
push!(trigger1, gid)
else
Expand Down Expand Up @@ -2430,9 +2433,8 @@ function __require_prelocked(uuidkey::PkgId, env=nothing)
insert_extension_triggers(uuidkey)
# After successfully loading, notify downstream consumers
run_package_callbacks(uuidkey)
elseif !haskey(explicit_loaded_modules, uuidkey)
explicit_loaded_modules[uuidkey] = m
run_package_callbacks(uuidkey)
else
newm = root_module(uuidkey)
end
return m
end
Expand All @@ -2445,7 +2447,6 @@ end
PkgOrigin() = PkgOrigin(nothing, nothing, nothing)
const pkgorigins = Dict{PkgId,PkgOrigin}()

const explicit_loaded_modules = Dict{PkgId,Module}() # Emptied on Julia start
const loaded_modules = Dict{PkgId,Module}() # available to be explicitly loaded
const loaded_precompiles = Dict{PkgId,Vector{Module}}() # extended (complete) list of modules, available to be loaded
const loaded_modules_order = Vector{Module}()
Expand Down Expand Up @@ -2485,7 +2486,6 @@ end
end
maybe_loaded_precompile(key, module_build_id(m)) === nothing && push!(loaded_modules_order, m)
loaded_modules[key] = m
explicit_loaded_modules[key] = m
module_keys[m] = key
end
nothing
Expand Down Expand Up @@ -2517,9 +2517,6 @@ loaded_modules_array() = @lock require_lock copy(loaded_modules_order)
# after unreference_module, a subsequent require call will try to load a new copy of it, if stale
# reload(m) = (unreference_module(m); require(m))
function unreference_module(key::PkgId)
if haskey(explicit_loaded_modules, key)
m = pop!(explicit_loaded_modules, key)
end
if haskey(loaded_modules, key)
m = pop!(loaded_modules, key)
# need to ensure all modules are GC rooted; will still be referenced
Expand Down Expand Up @@ -3125,7 +3122,7 @@ function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, in
# build up the list of modules that we want the precompile process to preserve
if keep_loaded_modules
concrete_deps = copy(_concrete_dependencies)
for (pkgreq, modreq) in loaded_modules # TODO: convert all relevant staleness heuristics to use explicit_loaded_modules instead
for (pkgreq, modreq) in loaded_modules
if !(pkgreq === Main || pkgreq === Core || pkgreq === Base)
push!(concrete_deps, pkgreq => module_build_id(modreq))
end
Expand Down
19 changes: 0 additions & 19 deletions test/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1129,25 +1129,6 @@ end
run(cmd_proj_ext)
end

# Sysimage extensions
# The test below requires that LinearAlgebra is in the sysimage and that it has not been loaded yet.
# if it gets moved out, this test will need to be updated.
# We run this test in a new process so we are not vulnerable to a previous test having loaded LinearAlgebra
sysimg_ext_test_code = """
uuid_key = Base.PkgId(Base.UUID("37e2e46d-f89d-539d-b4ee-838fcccc9c8e"), "LinearAlgebra")
Base.in_sysimage(uuid_key) || error("LinearAlgebra not in sysimage")
haskey(Base.explicit_loaded_modules, uuid_key) && error("LinearAlgebra already loaded")
using HasExtensions
Base.get_extension(HasExtensions, :LinearAlgebraExt) === nothing || error("unexpectedly got an extension")
using LinearAlgebra
haskey(Base.explicit_loaded_modules, uuid_key) || error("LinearAlgebra not loaded")
Base.get_extension(HasExtensions, :LinearAlgebraExt) isa Module || error("expected extension to load")
"""
cmd = `$(Base.julia_cmd()) --startup-file=no -e $sysimg_ext_test_code`
cmd = addenv(cmd, "JULIA_LOAD_PATH" => join([proj, "@stdlib"], sep))
run(cmd)


# Extensions in implicit environments
old_load_path = copy(LOAD_PATH)
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,12 @@ deps = ["ExtDep3"]
path = "../HasExtensions.jl"
uuid = "4d3288b3-3afc-4bb6-85f3-489fffe514c8"
version = "0.1.0"
weakdeps = ["ExtDep", "ExtDep2"]

[deps.HasExtensions.extensions]
Extension = "ExtDep"
ExtensionDep = "ExtDep3"
ExtensionFolder = ["ExtDep", "ExtDep2"]
LinearAlgebraExt = "LinearAlgebra"

[deps.HasExtensions.weakdeps]
ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c"
ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

[[deps.SomePackage]]
path = "../SomePackage"
Expand Down
2 changes: 0 additions & 2 deletions test/project/Extensions/HasExtensions.jl/Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@ ExtDep3 = "a5541f1e-a556-4fdc-af15-097880d743a1"
[weakdeps]
ExtDep = "fa069be4-f60b-4d4c-8b95-f8008775090c"
ExtDep2 = "55982ee5-2ad5-4c40-8cfe-5e9e1b01500d"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

[extensions]
Extension = "ExtDep"
ExtensionDep = "ExtDep3"
ExtensionFolder = ["ExtDep", "ExtDep2"]
LinearAlgebraExt = "LinearAlgebra"

This file was deleted.