Prevent pre-compilation package from triggering its own extensions#56666
Prevent pre-compilation package from triggering its own extensions#56666topolarity merged 2 commits intoJuliaLang:masterfrom
Conversation
topolarity
added
backport 1.10
These are the main correctness fix from JuliaLang#55910, so it's important that we have test coverage for it.
It is possible for an extension `ExtAB` to be loadable by one of its triggers, e.g. if A loads B. However this loading is only supposed to happen after loading for A is finished, so it shouldn't be included as part of pre-compiling A. Getting this wrong means disagreeing with the scheduled pre-compile jobs (A is not scheduled to depend on or generate a cache file for ExtAB but accidentally does both) and leads to confusing errors about missing cache files. To avoid trying to use / generate a cache file for ExtAB while still pre-compiling A, this change tracks the package being currently pre- compiled so that its extension triggers can be ignored.
topolarity
force-pushed
the
ct/no-toplevel-ext
branch
from
5c3aaf5 to
06f8519
Compare
|
I am not talented enough to decode our I'm going to merge this for now and improving the test can be a follow-up. In any case, this is enough coverage to get an error to show up in the CI logs |
…56666) It is possible for an extension `ExtAB` to be loadable by one of its triggers, e.g. if `A` loads `B`. However, this loading is not supposed to happen during pre-compilation of `A`. Getting this wrong means disagreeing with the scheduled pre-compile jobs (`A` is not scheduled to depend on or generate a cache file for `ExtAB` but accidentally attempts both) and leads to confusing errors about missing cache files. We used to cover up this bad behavior w/ an erroneous cycle warning (fixed by #55910), but now we need to be sure this works.
|
The 1.10 backport of this (e6c7c92) went smoothly, but testing it exposed a new bug on 1.10: $ ./julia --project=test/project/Extensions/Parent.jl -q
(Parent) pkg> precompile
Precompiling project...
✓ DepWithParentExt
✓ Parent
2 dependencies successfully precompiled in 2 seconds
julia> using Parent
[ Info: Precompiling ParentExt [3290166e-fa45-5082-a21d-a36e4f051b06]
This is probably why @christiangnrd reported in JuliaConcurrent/Atomix.jl#44 (comment) that the bug fixed by this PR wasn't present on 1.10 - I only hit the loading bug here on 1.10 because I was using a new (1.11) Manifest which actually precompiles everything |
topolarity
removed
the
backport 1.10
|
…uliaLang#56666) It is possible for an extension `ExtAB` to be loadable by one of its triggers, e.g. if `A` loads `B`. However, this loading is not supposed to happen during pre-compilation of `A`. Getting this wrong means disagreeing with the scheduled pre-compile jobs (`A` is not scheduled to depend on or generate a cache file for `ExtAB` but accidentally attempts both) and leads to confusing errors about missing cache files. We used to cover up this bad behavior w/ an erroneous cycle warning (fixed by JuliaLang#55910), but now we need to be sure this works.
…n extensions (#56666) (#56676) It is possible for an extension `ExtAB` to be loadable by one of its triggers, e.g. if `A` loads `B`. However, this loading is not supposed to happen during pre-compilation of `A`. Getting this wrong means disagreeing with the scheduled pre-compile jobs (`A` is not scheduled to depend on or generate a cache file for `ExtAB` but accidentally attempts both) and leads to confusing errors about missing cache files. We used to cover up this bad behavior w/ an erroneous cycle warning (fixed by #55910), but now we need to be sure this works.
| 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) |
There was a problem hiding this comment.
@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..
There was a problem hiding this comment.
@vtjnash any idea whether this is an issue? Is it possible we might violate the package locks for the pre-compiling package?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
oh, yeah, that would probably cause confusion in the code elsewhere
…56666) It is possible for an extension `ExtAB` to be loadable by one of its triggers, e.g. if `A` loads `B`. However, this loading is not supposed to happen during pre-compilation of `A`. Getting this wrong means disagreeing with the scheduled pre-compile jobs (`A` is not scheduled to depend on or generate a cache file for `ExtAB` but accidentally attempts both) and leads to confusing errors about missing cache files. We used to cover up this bad behavior w/ an erroneous cycle warning (fixed by #55910), but now we need to be sure this works.
4 participants
It is possible for an extension
ExtABto be loadable by one of its triggers, e.g. ifAloadsB. However, this loading is not supposed to happen during pre-compilation ofA.Getting this wrong means disagreeing with the scheduled pre-compile jobs (
Ais not scheduled to depend on or generate a cache file forExtABbut accidentally attempts both) and leads to confusing errors about missing cache files.We used to cover up this bad behavior w/ an erroneous cycle warning (fixed by #55910), but now we need to be sure this works.