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

(re-)allow include_dependency(directory) #49244

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

benlorenz
Copy link
Contributor

This would allow using a directory for include_dependency(path) to track whether any file or subdirectory is added.

This was working fine (for Oscar.jl) in Julia 1.6 and 1.8 but since #47184 (in master and 1.9) the cache file is rejected with the following error and a fresh precompilation is triggered every time Oscar is loaded:

┌ Debug: Rejecting stale cache file <...>/aJ3Pg_wOMvI.ji because file <...>/Oscar.jl/experimental does not exist

Here experimental is a subfolder where developers can add extra code that will then be picked up without adding all files explicitly to some list in the main Oscar sources.

I am aware that the docs for include_dependency only mention file, I understood file here in a more file-system oriented way and not strictly as regular file, similar to how it is used in the docs for stat.

I adjusted the docs for include_dependency accordingly and clarified that this uses the mtime of path.

cc: @lkastner
x-ref: oscar-system/Oscar.jl#2203

same for symlinks, adjust docs accordingly and clarify that it refers to the mtime
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 4, 2023

I think this is okay to change back, since it should adequately reflect any changes to readdir. The JLL packages should be doing this too, for correct execution. If we ever add some content-based hashing as well, we can state that the content is the set of filenames returned by readdir and that should be consistent with this mtime.

@vtjnash vtjnash added status:merge me PR is reviewed. Merge when all tests are passing backport 1.9 Change should be backported to release-1.9 labels Apr 4, 2023
@benlorenz
Copy link
Contributor Author

The test-failure on win32 looks unrelated, worker 5 died with std::bad_alloc during Tar.jl tests.

@vtjnash vtjnash merged commit 93df7e2 into JuliaLang:master Apr 4, 2023
@benlorenz benlorenz deleted the bl/incdep_dir branch April 4, 2023 17:23
KristofferC pushed a commit that referenced this pull request Apr 9, 2023
same for symlinks, adjust docs accordingly and clarify that it refers to the mtime

(cherry picked from commit 93df7e2)
@KristofferC KristofferC mentioned this pull request Apr 9, 2023
26 tasks
@giordano giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 11, 2023
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
same for symlinks, adjust docs accordingly and clarify that it refers to the mtime
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Apr 25, 2023
@benlorenz benlorenz mentioned this pull request Oct 4, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants