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

Prevent pre-compilation package from triggering its own extensions #56666

Merged
merged 2 commits into from
Nov 24, 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
5 changes: 4 additions & 1 deletion base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1433,6 +1433,7 @@ function run_module_init(mod::Module, i::Int=1)
end

function run_package_callbacks(modkey::PkgId)
@assert modkey != precompilation_target
run_extension_callbacks(modkey)
assert_havelock(require_lock)
unlock(require_lock)
Expand Down Expand Up @@ -1562,7 +1563,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
uuid_trigger = UUID(totaldeps[trigger]::String)
trigger_id = PkgId(uuid_trigger, trigger)
push!(trigger_ids, trigger_id)
if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id)
if !haskey(Base.loaded_modules, trigger_id) || haskey(package_locks, trigger_id) || (trigger_id == precompilation_target)
Copy link
Member Author

Choose a reason for hiding this comment

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

@KristofferC mentioned that the package_locks check used to do this job because the pre-compiling package would have been effectively marked as an "in-progress" load

That sounds like a broken invariant to me..

Copy link
Member Author

Choose a reason for hiding this comment

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

@vtjnash any idea whether this is an issue? Is it possible we might violate the package locks for the pre-compiling package?

Copy link
Member

Choose a reason for hiding this comment

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

Since this appears to prevent the trigger from firing (if it was the explicitly required package for precompile), I'd say that still follows the locking scheme

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I don't mean this fix specifically

I'm asking whether the fact that the explicitly required package is missing from package_locks could be an issue elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

oh, yeah, that would probably cause confusion in the code elsewhere

trigger1 = get!(Vector{ExtensionId}, EXT_DORMITORY, trigger_id)
push!(trigger1, gid)
else
Expand All @@ -1575,6 +1576,7 @@ end
loading_extension::Bool = false
loadable_extensions::Union{Nothing,Vector{PkgId}} = nothing
precompiling_extension::Bool = false
precompilation_target::Union{Nothing,PkgId} = nothing
function run_extension_callbacks(extid::ExtensionId)
assert_havelock(require_lock)
succeeded = try
Expand Down Expand Up @@ -3081,6 +3083,7 @@ function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::
Base.track_nested_precomp($(_pkg_str(vcat(Base.precompilation_stack, pkg))))
Base.loadable_extensions = $(_pkg_str(loadable_exts))
Base.precompiling_extension = $(loading_extension)
Base.precompilation_target = $(_pkg_str(pkg))
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))))
""")
Expand Down
34 changes: 34 additions & 0 deletions test/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,40 @@ end
@test occursin("Hello x-package ext-to-ext!", String(read(cmd)))
end

# Extensions for "parent" dependencies
# (i.e. an `ExtAB` where A depends on / loads B, but B provides the extension)

mktempdir() do depot # Parallel pre-compilation
code = """
Base.disable_parallel_precompile = false
using Parent
Base.get_extension(getfield(Parent, :DepWithParentExt), :ParentExt) isa Module || error("expected extension to load")
Parent.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "Parent.jl")
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 parent!", String(read(cmd)))
end
mktempdir() do depot # Serial pre-compilation
code = """
Base.disable_parallel_precompile = true
using Parent
Base.get_extension(getfield(Parent, :DepWithParentExt), :ParentExt) isa Module || error("expected extension to load")
Parent.greet()
"""
proj = joinpath(@__DIR__, "project", "Extensions", "Parent.jl")
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 parent!", String(read(cmd)))
end

finally
try
rm(depot_path, force=true, recursive=true)
Expand Down
9 changes: 9 additions & 0 deletions test/project/Extensions/DepWithParentExt.jl/Project.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name = "DepWithParentExt"
uuid = "8a35c396-5ffc-40d2-b7ec-e8ed2248da32"
version = "0.1.0"

[weakdeps]
Parent = "58cecb9c-f68a-426e-b92a-89d456ae7acc"

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

using Parent
using DepWithParentExt

end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module DepWithParentExt

greet() = print("Hello dep w/ ext for parent dep!")

end # module DepWithParentExt
20 changes: 20 additions & 0 deletions test/project/Extensions/Parent.jl/Manifest.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# This file is machine-generated - editing it directly is not advised

julia_version = "1.12.0-DEV"
manifest_format = "2.0"
project_hash = "b6ac643184d62cc94427c9aa665ff1fb63d66038"

[[deps.DepWithParentExt]]
path = "../DepWithParentExt.jl"
uuid = "8a35c396-5ffc-40d2-b7ec-e8ed2248da32"
version = "0.1.0"
weakdeps = ["Parent"]

[deps.DepWithParentExt.extensions]
ParentExt = "Parent"

[[deps.Parent]]
deps = ["DepWithParentExt"]
path = "."
uuid = "58cecb9c-f68a-426e-b92a-89d456ae7acc"
version = "0.1.0"
7 changes: 7 additions & 0 deletions test/project/Extensions/Parent.jl/Project.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
name = "Parent"
uuid = "58cecb9c-f68a-426e-b92a-89d456ae7acc"
version = "0.1.0"
authors = ["Cody Tapscott <[email protected]>"]

[deps]
DepWithParentExt = "8a35c396-5ffc-40d2-b7ec-e8ed2248da32"
7 changes: 7 additions & 0 deletions test/project/Extensions/Parent.jl/src/Parent.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module Parent

using DepWithParentExt

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

end # module Parent
Loading