precompilepkgs: respect loaded dependencies when precompiling for load #56901
+16
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
IanButterworth
added
packages
IanButterworth
force-pushed
the
ib/precompile_respect_loaded_loading
branch
from
56b416b to
2066d21
Compare
IanButterworth
commented
Dec 24, 2024
| @@ -1158,7 +1160,7 @@ function precompile_pkgs_maybe_cachefile_lock(f, io::IO, print_lock::ReentrantLo | |||
| # wait until the lock is available | |||
| @invokelatest Base.mkpidlock_hook(() -> begin | |||
| # double-check in case the other process crashed or the lock expired | |||
| if Base.isprecompiled(pkg; ignore_loaded=true, flags=cacheflags) # don't use caches for this as the env state will have changed | |||
| if Base.isprecompiled(pkg; ignore_loaded, flags=cacheflags) # don't use caches for this as the env state will have changed | |||
There was a problem hiding this comment.
I don't recall a reason for this to be forced false. I'm assuming it was just missed.
vtjnash
reviewed
Dec 27, 2024
| staledeps = stale_cachefile(sourcepath, path_to_try, ignore_loaded = true, requested_flags=flags) | ||
| staledeps = stale_cachefile(sourcepath, path_to_try; ignore_loaded, requested_flags=flags) |
There was a problem hiding this comment.
IIUC, this was to allow the user to compute the best pkg to load for compilecache_path, even if it is currently loaded at a different version?
There was a problem hiding this comment.
Or maybe it was just missed originally? c24136d
There was a problem hiding this comment.
I think it could've been. It's not mentioned in the PR #50218
vtjnash
approved these changes
Dec 27, 2024
|
I've expanded the explanation at Pkg time to explain why further precompilation may be hit at load time.
|
IanButterworth
commented
Dec 29, 2024
IanButterworth
force-pushed
the
ib/precompile_respect_loaded_loading
branch
from
65009dc to
9a10434
Compare
IanButterworth
added
the
merge me
stevengj
pushed a commit
that referenced
this pull request
Jan 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this
Pkg.precompilestill precompiles for a clean session with the existing warning about different loaded versions, but now load-time precompilation precompiles for the current session respecting the loaded versions of deps.So the old style precompilation is avoided at load-time now.
However, there's still no warning at load time about the wrong dep version being used, but at least it's clearer at install-time now.