Allow include() and include_dependency() files to resolve to different depots
#52750
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
fatteneder
added
compiler:precompilation
staticfloat
approved these changes
Jan 5, 2024
There was a problem hiding this comment.
LGTM! Also, can confirm this fixes the constant precompilation issue referenced in #52161 (comment)
|
Looks like some tests might need some adjustment though: https://buildkite.com/julialang/julia-master/builds/31836#018cd6aa-caa1-4e75-aa48-3eea5e857605/802-1575 |
fatteneder
force-pushed
the
fa/incldeps-depot-search
branch
2 times, most recently
from
eba00a2
to
860596a
Compare
fatteneder
force-pushed
the
fa/incldeps-depot-search
branch
from
860596a
to
07cae69
Compare
|
x86_64-apple-darwin failure should be fixed by #52782, but I don't understand why it only appears on that platform. |
Allow include() and include_dependency() files to resolve to different depots. We use one and the same depot for all include() files, but include_dependency() files can each resolve to a different depot. Co-authored-by: staticfloat [email protected]
fatteneder
force-pushed
the
fa/incldeps-depot-search
branch
from
07cae69
to
edb66b8
Compare
|
Windows failures look real. |
|
I could not spot an obvious problem with the test. From worker 9: ┌ Warning: RelocationTestPkg1 [854e1adb-5a97-46bf-a391-1cfe05ac726d] is already loaded from a different path:
From worker 9: │ loaded: v0.1.0 "C:\\buildkite-agent\\builds\\win2k22-amdci6-5\\julialang\\julia-master\\julia-edb66b8b14\\share\\julia\\test\\RelocationTestPkg1\\src\\RelocationTestPkg1.jl"so I updated the test to work with temporarily generated example pkgs to avoid that. |
loading of already loaded pkg fixup version tag; use open() instead of write()
fatteneder
force-pushed
the
fa/incldeps-depot-search
branch
from
2d32501
to
d29b365
Compare
giordano
reviewed
Jan 18, 2024
Comment on lines
+155
to
+156
| @show srcfile1 | ||
| @show srcfile2 |
There was a problem hiding this comment.
Are these @show's necessary? They're spamming all tests.
There was a problem hiding this comment.
Are these @show's necessary?
No.
KristofferC
pushed a commit
that referenced
this pull request
Jan 20, 2024
fatteneder
added a commit
that referenced
this pull request
Apr 2, 2024
…files (#53905) Fixes #53859 (comment), which was actually fixed before in #52346, but #52750 undid that fix. This PR does not directly address #53859, because I can not reproduce it atm. --- The `@depot` resolution logic for `include()` files is adjusted as follows: 1. (new behavior) If the cache is not relocatable because of an absolute path, we ignore that path for the depot search. Recompilation will be triggered by `stale_cachefile()` if that absolute path does not exist. Previously this caused any `@depot` tags to be not resolved and so trigger recompilation. 2. (new behavior) If we can't find a depot for a relocatable path, we still replace it with the depot we found from other files. Recompilation will be triggered by `stale_cachefile()` because the resolved path does not exist. 3. (this behavior is kept) We require that relocatable paths all resolve to the same depot. 4. (new behavior) We no longer use the first matching depot for replacement, but instead we explicitly check that all resolve to the same depot. This has two reasons: - We want to scan all source files anyways in order to provide logs for 1. and 2. above, so the check is free. - It is possible that a depot might be missing source files. Assume that we have two depots on `DEPOT_PATH`, `depot_complete` and `depot_incomplete`. If `DEPOT_PATH=["depot_complete","depot_incomplete"]` then no recompilation shall happen, because `depot_complete` will be picked. If `DEPOT_PATH=["depot_incomplete","depot_complete"]` we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves to `depot_complete` or `depot_incomplete`.
KristofferC
pushed a commit
that referenced
this pull request
Apr 9, 2024
…files (#53905) Fixes #53859 (comment), which was actually fixed before in #52346, but #52750 undid that fix. This PR does not directly address #53859, because I can not reproduce it atm. --- The `@depot` resolution logic for `include()` files is adjusted as follows: 1. (new behavior) If the cache is not relocatable because of an absolute path, we ignore that path for the depot search. Recompilation will be triggered by `stale_cachefile()` if that absolute path does not exist. Previously this caused any `@depot` tags to be not resolved and so trigger recompilation. 2. (new behavior) If we can't find a depot for a relocatable path, we still replace it with the depot we found from other files. Recompilation will be triggered by `stale_cachefile()` because the resolved path does not exist. 3. (this behavior is kept) We require that relocatable paths all resolve to the same depot. 4. (new behavior) We no longer use the first matching depot for replacement, but instead we explicitly check that all resolve to the same depot. This has two reasons: - We want to scan all source files anyways in order to provide logs for 1. and 2. above, so the check is free. - It is possible that a depot might be missing source files. Assume that we have two depots on `DEPOT_PATH`, `depot_complete` and `depot_incomplete`. If `DEPOT_PATH=["depot_complete","depot_incomplete"]` then no recompilation shall happen, because `depot_complete` will be picked. If `DEPOT_PATH=["depot_incomplete","depot_complete"]` we trigger recompilation and hopefully a meaningful error about missing files is thrown. If we were to just select the first depot we find, then whether recompilation happens would depend on whether the first relocatable file resolves to `depot_complete` or `depot_incomplete`. (cherry picked from commit d8d3842)
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.
Fixes #52161
#52161 is an awkward use of the relocation feature in the sense that it attempts to load the
include()andinclude_dependencyfiles of a pkg from two separate depots.The problem there is that the value with which we replace the
@depottag for bothinclude()andinclude_dependency()files is determined by trying to relocate only theinclude()files. We then end up not finding theinclude_dependency()files.Solution:
@staticfloat noted that the pkg slugs in depot paths like
@depot/packages/Foo/1a2b3c/src/Foo.jlare already enough to (weakly) content-address a depot.This means that we should be able to load any
include()file of a pkg from anydepotthat contains a precompile cache, provided the hashes match.The same logic can be extended to
include_dependency()files, which this PR does.Note that we continue to use only one file from the
include()files to determine the depot which we use to relocate allinclude()files. [this behavior is kept from master]But for
include_dependency()files we allow each file to resolve to a different depot. This way the MWE given in #52161 should be extendable to two README files being located in two different pkgs that lie in two different depots.Side note: #49866 started with explicitly verifying that all
include()files come from the same depot. In #52346 this was already relaxed to pick the first depot for which anyinclude()file can be resolved to. This works, because if any other file might be missing from that depot then this is caught instalecache().