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

RFC: Add RelocPath type for userdefined relocatable paths #56053

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 71 additions & 3 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3228,14 +3228,22 @@ mutable struct CacheHeaderIncludes
const modpath::Vector{String} # seemingly not needed in Base, but used by Revise
end

function replace_depot_path(path::AbstractString, depots::Vector{String}=normalize_depots_for_relocation())
function replace_depot_path_impl(path::AbstractString, depots::Vector{String}=normalize_depots_for_relocation())
the_depot = nothing
reloc_path = path
for depot in depots
if startswith(path, string(depot, Filesystem.pathsep())) || path == depot
path = replace(path, depot => "@depot"; count=1)
reloc_path = replace(path, depot => "@depot"; count=1)
the_depot = depot
break
end
end
return path
return the_depot, reloc_path
end

function replace_depot_path(path::AbstractString, depots::Vector{String}=normalize_depots_for_relocation())
_, reloc_path = replace_depot_path_impl(path, depots)
return reloc_path
end

function normalize_depots_for_relocation()
Expand Down Expand Up @@ -3263,6 +3271,66 @@ function resolve_depot(inc::AbstractString)
return :no_depot_found
end

"""
RelocPath(path::AbstractString)
fatteneder marked this conversation as resolved.
Show resolved Hide resolved

A type to represent a relocatable path.

Requires `path` to be located within one of `DEPOT_PATH` upon construction.

An error is thrown if relocation fails.

# Example
```jldoctest
julia> path1 = joinpath(mktempdir(), "foo"); touch(path1); # set up a file called foo

julia> pushfirst!(DEPOT_PATH, dirname(path1));

julia> relocpath = RelocPath(path1);
fatteneder marked this conversation as resolved.
Show resolved Hide resolved

julia> String(relocpath) == path1
true

julia> path2 = joinpath(mktempdir(), "foo"); touch(path2); # set up another foo

julia> pushfirst!(DEPOT_PATH, dirname(path2));

julia> String(relocpath) == path2
true

julia> path1 != path2
true
```
"""
struct RelocPath
subpath::String
Comment on lines +3375 to +3376
Copy link
Member

@vtjnash vtjnash Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts about using OncePerProcess to make it so we only compute the result once:

Suggested change
struct RelocPath
subpath::String
struct RelocPath
getpath::OncePerProcess{String}
end
RelocPath(path::AbstractString) = let
depot, _ = replace_depot_path_impl(path)
if isnothing(depot)
error("Failed to locate $(path) in any of DEPOT_PATH.")
end
subpath = String(replace(path, depot => ""; count=1))
return RelocPath(OncePerProcess{String}() do
for d in DEPOT_PATH
if isdirpath(d)
d = dirname(d)
end
local path = string(d, subpath)
if ispath(path)
return path
end
end
error("Failed to relocate @depot$(subpath) in any of DEPOT_PATH.")
end)
end
String(r::RelocPath) = r.getpath() # dynamically dispatches since getpath isn 't concrete :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this. What if someone has modified Base.DEPOT_PATH during their Julia session?

function RelocPath(path::AbstractString)
depot, _ = replace_depot_path_impl(path)
if isnothing(depot)
@show DEPOT_PATH
fatteneder marked this conversation as resolved.
Show resolved Hide resolved
error("Failed to locate $(path) in any of DEPOT_PATH.")
end
subpath = replace(path, depot => ""; count=1)
return new(subpath)
end
end

function String(r::RelocPath)
for d in DEPOT_PATH
if isdirpath(d)
d = dirname(d)
end
path = string(d, r.subpath)
if ispath(path)
return path
end
end
error("Failed to relocate @depot$(r.subpath) in any of DEPOT_PATH.")
end

function show(io::IO, r::RelocPath)
print(io, string("RelocPath(\"@depot", r.subpath, "\")"))
end
Comment on lines +3400 to +3402
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty inaccurate, since it doesn't support @depot

Suggested change
function show(io::IO, r::RelocPath)
print(io, string("RelocPath(\"@depot", r.subpath, "\")"))
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we allow RelocPath to accept paths that are literally @depot/foo/bar?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, as far as the original show method goes, I think that the meaning of @depot/ is reasonably easy to understand here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty inaccurate, since it doesn't support @depot

But it does behave like a @depot. (it behaves more like include_dependency than include to be precise)

What is inaccurate is that show assumes that nobody tinkered with DEPOT_PATH since construction.


function _parse_cache_header(f::IO, cachefile::AbstractString)
flags = read(f, UInt8)
Expand Down
1 change: 1 addition & 0 deletions base/public.jl
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public
DL_LOAD_PATH,
load_path,
active_project,
RelocPath,

# Reflection and introspection
isambiguous,
Expand Down
2 changes: 2 additions & 0 deletions test/RelocationTestPkg1/src/RelocationTestPkg1.jl
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module RelocationTestPkg1

const relocpath = Base.RelocPath(@__DIR__)

greet() = print("Hello World!")

end # module RelocationTestPkg1
11 changes: 9 additions & 2 deletions test/relocatedepot.jl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function test_harness(@nospecialize(fn); empty_load_path=true, empty_depot_path=
end

# We test relocation with these dummy pkgs:
# - RelocationTestPkg1 - pkg with no include_dependency
# - RelocationTestPkg1 - pkg with no include_dependency, also contains a RelocPath()
# - RelocationTestPkg2 - pkg with include_dependency tracked by `mtime`
# - RelocationTestPkg3 - pkg with include_dependency tracked by content
# - RelocationTestPkg4 - pkg with no dependencies; will be compiled such that the pkgimage is
Expand Down Expand Up @@ -263,11 +263,18 @@ else
pkgname = "RelocationTestPkg1"
test_harness() do
push!(LOAD_PATH, joinpath(@__DIR__, "relocatedepot"))
push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot")) # required to find src files
push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot", "julia")) # contains cache file
push!(DEPOT_PATH, joinpath(@__DIR__, "relocatedepot")) # required to find src files
pkg = Base.identify_package(pkgname)
@test Base.isprecompiled(pkg) == true
@test Base.isrelocatable(pkg) == true
pkg = Base.require(pkg)
@test String(pkg.relocpath) == joinpath(pkgdir(pkg), "src")
pop!(DEPOT_PATH)
@test_throws(
ErrorException("Failed to relocate @depot/RelocationTestPkg1/src in any of DEPOT_PATH."),
String(pkg.relocpath)
)
end
end

Expand Down