-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make precompile files relocatable #49866
Conversation
I guess I did miss something, because the first test is already failing with |
ab2fa3b
to
25ae086
Compare
This needs more work because it breaks this MWE # from test/compiler/contextual.jl
load_path = mktempdir()
depot_path = mktempdir()
prev_DEPOT_PATH = deepcopy(DEPOT_PATH)
prev_LOAD_PATH = deepcopy(LOAD_PATH)
pushfirst!(LOAD_PATH, load_path)
pushfirst!(DEPOT_PATH, depot_path)
write(joinpath(load_path, "Foo.jl"),
"""
module Foo
end
""")
try
Foo = Base.require(Main, :Foo)
finally
copy!(DEPOT_PATH, prev_DEPOT_PATH)
copy!(LOAD_PATH, prev_LOAD_PATH)
end |
be58fca
to
0a5ab9e
Compare
This is very exciting. Thanks for working on this! |
Regarding builds & tests: I see random (?) build failures (e.g. There are some tests failing too where I don't know what is causing that.
|
d081c65
to
cdd5ab9
Compare
Some CI tests / machines are flaky, and those seem like they may be some of them. I think this is simple enough that I say we just go for it? We might have other issues still (like stacktraces and Strings in the source code that accidentally capture Maybe @KristofferC can help make sure this gets merged? |
cdd5ab9
to
3ede61c
Compare
I hacked together a MWE for a unit test. But I think it is not yet doing the right thing ... |
d0e93a0
to
88448ca
Compare
Regarding test failures:
|
88448ca
to
aac1ec8
Compare
aac1ec8
to
dd3c5ce
Compare
I could identify and fix one more problem related to |
dd3c5ce
to
ec16ce2
Compare
I’ve not reviewed this yet but I just wanted to say thanks. It would be great to have this. |
ec16ce2
to
22e2f69
Compare
Looks like this was hitting repeated precompilation on windows. This Pkg test was failing https://github.com/JuliaLang/Pkg.jl/blob/master/test/api.jl#L339 If you want to investigate why the caches are being rejected you can set JULIA_DEBUG=loading |
Let's do one more PkgEval and then merge this! |
The package evaluation job you requested has completed - possible new issues were detected. |
🎉🎉🎉 |
This is really exciting. Thank you so much @fatteneder! And thanks to @vchuravy, @staticfloat, @IanButterworth, @KristofferC, and anyone else that I missed that helped with reviewing this PR. This PR crosses off two things (#45215 and #45541) off of my Julia wishlist. |
@fatteneder @vchuravy Could we add this to NEWS? |
Thanks everyone for the assistance! |
This broke Revise, which is unfortunate. There's a PR open to fix it, but it'd have been nice to merge that first, since Revise is pretty essential for debugging for those of us that live on master. |
This patch restores the filtering of non-src files that was lost in #767. After JuliaLang/julia#49866, `Base.parse_cache_header` returns both the non-filtered and filtered list instead of doing filtering internally based on the `srcfiles_only::Bool` keyword argument. There is a test for this failure mode that fails on Revise master branch and passes with this patch (see https://github.com/timholy/Revise.jl/blob/1059181bed06387e9fbcea137dce28a80c5c45d9/test/runtests.jl#L3070-L3080).
* Restore filtering of non-src files This patch restores the filtering of non-src files that was lost in #767. After JuliaLang/julia#49866, `Base.parse_cache_header` returns both the non-filtered and filtered list instead of doing filtering internally based on the `srcfiles_only::Bool` keyword argument. There is a test for this failure mode that fails on Revise master branch and passes with this patch (see https://github.com/timholy/Revise.jl/blob/1059181bed06387e9fbcea137dce28a80c5c45d9/test/runtests.jl#L3070-L3080). * Set version to 3.5.9.
parse_cache_header now takes a second string argument after an IO JuliaLang/julia#49866
…erent depots (#52750) Fixes #52161 #52161 is an awkward use of the relocation feature in the sense that it attempts to load the `include()` and `include_dependency` files of a pkg from two separate depots. The problem there is that the value with which we replace the `@depot` tag for both `include()` and `include_dependency()` files is determined by trying to relocate only the `include()` files. We then end up not finding the `include_dependency()` files. Solution: @staticfloat noted that the pkg slugs in depot paths like `@depot/packages/Foo/1a2b3c/src/Foo.jl` are already enough to (weakly) content-address a depot. This means that we should be able to load any `include()` file of a pkg from any `depot` that 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 all `include()` 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 any `include()` file can be resolved to. This works, because if any other file might be missing from that depot then this is caught in `stalecache()`.
…51798) Continuation of #49866. Fixes #52462 So far any `include_dependency` was tracked by `mtime`. A package using `include_dependency` can't be relocated without recompilation if the dependency also needs to be relocated. With `include_dependency(path, track_content=true)` the tracking works like for `include`, i.e. recompilation is only triggered when the file's content changes. In case `path` is a directory we use the string `join(readdir(path))` as content. --------- Co-authored-by: Valentin Churavy <[email protected]>
Fixes #45215
Fixes #47943
Fixes #45541
Fixes #50918
Updated description
Hi.
I tried my luck with #47943 and I implemented what was suggested by OP.
String replacement with
@depot
when serializing out happens with any paths that are located inside aDEPOT_PATH
(first match wins). If no match, then we emit the absolute file path as before. Right now we only emit one token@depot
.String replacement of
@depot
when loading happens now on a.ji
file basis and only if all the listed include dependencies can be resolved to files located in one and the same depot onDEPOT_PATH
(again, first match wins). If we can't resolve, then the cache is invalided withstale_cachefile
.This PR does not explicitly address any of #4630 or #33065 some suggested would be nice to fix in one swipe with #47943. It remains to check if this was accidentally solved.
I also did not understand what was meant in #47943 (comment) with the
deps/build.jl
andartifacts
issue and how that should be addressed here.Status of this PR
The following works for me locally,
<git-root>/usr/share/julia/
) somewhere else, setJULIA_DEPOT_PATH
accordingly, e.g.Open questions
From docs:
Does... are treated as read-only ...
guarantee that Julia will never write anything to a path other thanDEPOT_PATH[1]
?[Need to test this myself] If we load packages from any of the read-only paths, their precompiles also end up inDEPOT_PATH[1]/compiled
?If both questions can be answered with yes, then I guess its save to replace@depot
always withDEPOT_PATH[1]
when loading. The path for@stdlib
is replaced withSys.STDLIB
, because that seems to be tied to where the julia binary sits.I think I could work around all of that by following more closely what was proposed in #47943 (comment):
See top of this post on how it is implemented now.
TODO
I would appreciate any feedback!