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.

Sign up for GitHub

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

Jump to bottom

relocation: account for trailing path separator in depot paths #55355

Merged
merged 7 commits into from
Sep 30, 2024
Merged

relocation: account for trailing path separator in depot paths #55355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
01d11e9
account for path separator when inserting @depot tag
fatteneder Aug 2, 2024
cba4a7e
unit test for issue 55340
fatteneder Aug 2, 2024
8dcf3d6
document assumptions on replace_depot_path
fatteneder Aug 9, 2024
d03ff12
refactor some tests
fatteneder Aug 11, 2024
8c6d7db
add license header
fatteneder Sep 18, 2024
0174e43
normalize depot paths just before inserting @depot tag
fatteneder Sep 18, 2024
b600a62
use abspath in depot normalization
fatteneder Sep 19, 2024
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
26 changes: 16 additions & 10 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3176,23 +3176,29 @@ mutable struct CacheHeaderIncludes
const modpath::Vector{String} # seemingly not needed in Base, but used by Revise
end

function replace_depot_path(path::AbstractString)
for depot in DEPOT_PATH
!isdir(depot) && continue

# Strip extraneous pathseps through normalization.
if isdirpath(depot)
depot = dirname(depot)
end

if startswith(path, depot)
function replace_depot_path(path::AbstractString, depots::Vector{String}=normalize_depots_for_relocation())
for depot in depots
if startswith(path, string(depot, Filesystem.pathsep())) || path == depot
fatteneder marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

In many cases, Windows accepts / as a path separator - is there any risk of that appearing 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.

oh, I did not know that.
I pushed a fix with an accompanying test.

Copy link
Member

@topolarity topolarity Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! The replace isn't safe for UNC paths (it destroys double slashes \\), but maybe we don't support those?

https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#example-ways-to-refer-to-the-same-file shows some exotic ways to build paths on Windows, in case we do. The Base path functionality uses splitdrive to get the UNC, etc. prefixes out of the way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I now settled to just use abspath to normalize both.
That should also increase the accuracy in matching depots in cases where one is an absolute and the other a relative one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I traced back the use of replace_depot_path and found that these issues should already be taken care of.
In particular,

  • the path argument will be absolute and normalized when it arrives, which happens at base/loading.jl:_include_dependency,
  • the elements of DEPOT_PATH also went through abspath before they are handed over to precompilation, see base/loading.jl:create_expr_cache.

So I will revert changes related to this and make the tests pass again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right before spawning the precompilation process, here

julia/base/loading.jl

Line 2812 in f2f76d8

depot_path = String[abspath(x) for x in DEPOT_PATH]

Shouldn't --output-ji hit that too?

Copy link
Member

Choose a reason for hiding this comment

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

That's something that spawns a process with --output-ji not the other way around

Copy link
Member Author

Choose a reason for hiding this comment

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

I am sorry, I don't understand.
Where would be the entry point for what you describe?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my point is that it's legal to spawn Julia with --output-ji outside of the spawning that precompilation does, and in that case the DEPOT_PATH may not be normalized

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I see. Indeed, that's problematic.

path = replace(path, depot => "@depot"; count=1)
break
end
end
return path
end

function normalize_depots_for_relocation()
depots = String[]
sizehint!(depots, length(DEPOT_PATH))
for d in DEPOT_PATH
isdir(d) || continue
if isdirpath(d)
d = dirname(d)
end
push!(depots, abspath(d))
end
return depots
end

function restore_depot_path(path::AbstractString, depot::AbstractString)
replace(path, r"^@depot" => depot; count=1)
end
Expand Down
17 changes: 13 additions & 4 deletions src/precompile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,17 @@ void write_srctext(ios_t *f, jl_array_t *udeps, int64_t srctextpos) {
static jl_value_t *replace_depot_func = NULL;
if (!replace_depot_func)
replace_depot_func = jl_get_global(jl_base_module, jl_symbol("replace_depot_path"));
static jl_value_t *normalize_depots_func = NULL;
if (!normalize_depots_func)
normalize_depots_func = jl_get_global(jl_base_module, jl_symbol("normalize_depots_for_relocation"));
ios_t srctext;
jl_value_t *deptuple = NULL;
JL_GC_PUSH2(&deptuple, &udeps);
jl_value_t *deptuple = NULL, *depots = NULL;
JL_GC_PUSH3(&deptuple, &udeps, &depots);
jl_task_t *ct = jl_current_task;
size_t last_age = ct->world_age;
ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
depots = jl_apply(&normalize_depots_func, 1);
ct->world_age = last_age;
for (size_t i = 0; i < len; i++) {
deptuple = jl_array_ptr_ref(udeps, i);
jl_value_t *depmod = jl_fieldref(deptuple, 0); // module
Expand All @@ -60,13 +68,14 @@ void write_srctext(ios_t *f, jl_array_t *udeps, int64_t srctextpos) {
}

jl_value_t **replace_depot_args;
JL_GC_PUSHARGS(replace_depot_args, 2);
JL_GC_PUSHARGS(replace_depot_args, 3);
replace_depot_args[0] = replace_depot_func;
replace_depot_args[1] = abspath;
replace_depot_args[2] = depots;
jl_task_t *ct = jl_current_task;
size_t last_age = ct->world_age;
ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
jl_value_t *depalias = (jl_value_t*)jl_apply(replace_depot_args, 2);
jl_value_t *depalias = (jl_value_t*)jl_apply(replace_depot_args, 3);
ct->world_age = last_age;
JL_GC_POP();

Expand Down
20 changes: 14 additions & 6 deletions src/staticdata_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,16 @@ static int64_t write_dependency_list(ios_t *s, jl_array_t* worklist, jl_array_t
static jl_value_t *replace_depot_func = NULL;
if (!replace_depot_func)
replace_depot_func = jl_get_global(jl_base_module, jl_symbol("replace_depot_path"));
static jl_value_t *normalize_depots_func = NULL;
if (!normalize_depots_func)
normalize_depots_func = jl_get_global(jl_base_module, jl_symbol("normalize_depots_for_relocation"));

jl_value_t *depots = NULL, *prefs_hash = NULL, *prefs_list = NULL;
JL_GC_PUSH2(&depots, &prefs_list);
last_age = ct->world_age;
ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
depots = jl_apply(&normalize_depots_func, 1);
ct->world_age = last_age;

// write a placeholder for total size so that we can quickly seek past all of the
// dependencies if we don't need them
Expand All @@ -764,13 +774,14 @@ static int64_t write_dependency_list(ios_t *s, jl_array_t* worklist, jl_array_t

if (replace_depot_func) {
jl_value_t **replace_depot_args;
JL_GC_PUSHARGS(replace_depot_args, 2);
JL_GC_PUSHARGS(replace_depot_args, 3);
replace_depot_args[0] = replace_depot_func;
replace_depot_args[1] = deppath;
replace_depot_args[2] = depots;
ct = jl_current_task;
size_t last_age = ct->world_age;
ct->world_age = jl_atomic_load_acquire(&jl_world_counter);
deppath = (jl_value_t*)jl_apply(replace_depot_args, 2);
deppath = (jl_value_t*)jl_apply(replace_depot_args, 3);
ct->world_age = last_age;
JL_GC_POP();
}
Expand Down Expand Up @@ -803,9 +814,6 @@ static int64_t write_dependency_list(ios_t *s, jl_array_t* worklist, jl_array_t
write_int32(s, 0); // terminator, for ease of reading

// Calculate Preferences hash for current package.
jl_value_t *prefs_hash = NULL;
jl_value_t *prefs_list = NULL;
JL_GC_PUSH1(&prefs_list);
if (jl_base_module) {
// Toplevel module is the module we're currently compiling, use it to get our preferences hash
jl_value_t * toplevel = (jl_value_t*)jl_get_global(jl_base_module, jl_symbol("__toplevel__"));
Expand Down Expand Up @@ -852,7 +860,7 @@ static int64_t write_dependency_list(ios_t *s, jl_array_t* worklist, jl_array_t
write_int32(s, 0);
write_uint64(s, 0);
}
JL_GC_POP(); // for prefs_list
JL_GC_POP(); // for depots, prefs_list

// write a dummy file position to indicate the beginning of the source-text
pos = ios_pos(s);
Expand Down
34 changes: 29 additions & 5 deletions test/relocatedepot.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

using Test


Expand Down Expand Up @@ -26,26 +28,48 @@ end

if !test_relocated_depot

@testset "insert @depot tag in path" begin
@testset "edge cases when inserting @depot tag in path" begin

# insert @depot only once for first match
test_harness() do
mktempdir() do dir
pushfirst!(DEPOT_PATH, dir)
path = dir*dir
@test Base.replace_depot_path(path) == "@depot"*dir
if Sys.iswindows()
# dirs start with a drive letter instead of a path separator
path = dir*Base.Filesystem.pathsep()*dir
@test Base.replace_depot_path(path) == "@depot"*Base.Filesystem.pathsep()*dir
else
path = dir*dir
@test Base.replace_depot_path(path) == "@depot"*dir
end
end

# 55340
empty!(DEPOT_PATH)
mktempdir() do dir
jlrc = joinpath(dir, "julia-rc2")
jl = joinpath(dir, "julia")
mkdir(jl)
push!(DEPOT_PATH, jl)
@test Base.replace_depot_path(jl) == "@depot"
@test Base.replace_depot_path(string(jl,Base.Filesystem.pathsep())) ==
string("@depot",Base.Filesystem.pathsep())
@test Base.replace_depot_path(jlrc) != "@depot-rc2"
@test Base.replace_depot_path(jlrc) == jlrc
end
end

# deal with and without trailing path separators
test_harness() do
mktempdir() do dir
pushfirst!(DEPOT_PATH, dir)
path = joinpath(dir, "foo")
if isdirpath(DEPOT_PATH[1])
DEPOT_PATH[1] = dirname(DEPOT_PATH[1]) # strip trailing pathsep
end
tag = joinpath("@depot", "") # append a pathsep
tag = string("@depot", Base.Filesystem.pathsep())
@test startswith(Base.replace_depot_path(path), tag)
DEPOT_PATH[1] = joinpath(DEPOT_PATH[1], "") # append a pathsep
DEPOT_PATH[1] = string(DEPOT_PATH[1], Base.Filesystem.pathsep())
@test startswith(Base.replace_depot_path(path), tag)
popfirst!(DEPOT_PATH)
@test !startswith(Base.replace_depot_path(path), tag)
Expand Down