Raise an error when using include_dependency with non-existent file #52105
Conversation
Since depending on a non-existent file or directory is strange, raise an error instead of silently triggering precompilation. Fixes JuliaLang#52063
| @@ -1677,6 +1677,7 @@ function _include_dependency(mod::Module, _path::AbstractString; track_content=t | |||
| push!(_require_dependencies, | |||
| (mod, path, filesize(path), open(_crc32c, path, "r"), -1.0)) | |||
| else | |||
| @assert ispath(path) "'$path': No such file or directory" | |||
There was a problem hiding this comment.
Sounds good but assert is the wrong choice here. Just check and throw a meaningful error type
| @@ -1677,6 +1677,7 @@ function _include_dependency(mod::Module, _path::AbstractString; track_content=t | |||
| push!(_require_dependencies, | |||
| (mod, path, filesize(path), open(_crc32c, path, "r"), -1.0)) | |||
| else | |||
| @assert ispath(path) "'$path': No such file or directory" | |||
There was a problem hiding this comment.
| @assert ispath(path) "'$path': No such file or directory" | |
| (isfile(path) && uperm(path) & 0x04 == 0x04) || | |
| throw(ArgumentError("$(repr(path)) is not a readable file")) |
(It has to be a readable file — not just a path, e.g. not a directory — for include_dependency, because Julia will compute a CRC hash of the contents IIRC. And repr is the right way to convert the path to a quoted string.)
There was a problem hiding this comment.
The perm check should be moved to the other branch, because incluce_dependencys are only tracked by mtime, or does that also require read permissions?
There was a problem hiding this comment.
mtime doesn't require read permissions, sorry. But I guess #51798 will affect this?
There was a problem hiding this comment.
#51798 is concerned with the other branch. If the file does not exist there, then the error is thrown through open().
|
You should also add a test to @test_throws ArgumentError include_dependency("i_do_not_exist")
@test_throws ArgumentError include_dependency(@__DIR__) |
… or directory (#53286) Replaces #52105 Fixes #52063 There is a question about whether the `ispath & uperm` check should be moved inside the `_track_dependencies[]` check (like it was in #52105), which would make it such that any errors are thrown only during precompilation. --------- Co-authored-by: Qian Long <longqian95@gmail.com> Co-authored-by: Jameson Nash <vtjnash@gmail.com>
… or directory (#53286) Replaces #52105 Fixes #52063 There is a question about whether the `ispath & uperm` check should be moved inside the `_track_dependencies[]` check (like it was in #52105), which would make it such that any errors are thrown only during precompilation. --------- Co-authored-by: Qian Long <longqian95@gmail.com> Co-authored-by: Jameson Nash <vtjnash@gmail.com> (cherry picked from commit 71fa11f)
Since depending on a non-existent file or directory is strange, raise an error instead of silently triggering precompilation.
Fixes #52063