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

Strip trailing slashes in JULIA_DEPOT_PATH when embedding @depot #51892

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

staticfloat
Copy link
Sponsor Member

The new relocatable cache file work uses simple text substitution when stripping out the depot from a cache file's paths, and when substituting it in again over the @depot marker. However, if a user starts julia with JULIA_DEPOT_PATH=/opt/foo/, the embedded path for Foo.jl's includes list will look like @depotpackages/Foo/XYZ/src/Foo.jl, and if the user then uses JULIA_DEPOT_PATH=/opt/foo (which should be equivalent) the cache file will fail to load with the message:

Failed to determine depot from srctext files

This commit standardizes the serialization format to always contain a trailing pathsep(), so that textual substitution is more likely to work regardless of slightly-inconsistent JULIA_DEPOT_PATH settings.

base/loading.jl Outdated Show resolved Hide resolved
@fatteneder
Copy link
Member

Thanks for spotting. Changes look good!

I checked the replace logic again and I think there are at least two more edges cases where we could mess up (but for another reason):

  1. When inserting @depot:

    path = replace(path, depot => "@depot")
    This can yield double substitution when JULIA_DEPOT_PATH=foo/bar and path = "/foo/bar/foo/bar" which would result in @depot/@depot.

  2. When replacing @depot: I wanted to be smart and inserted a ^ into the regex to limit substitution to first occurence

    isfile(replace(inc, r"^@depot" => depot))
    However, this can also give a double substitution in case we have serialized a path like @depot/foo/bar/\n@depot/mypkg/src/file.jl (the second @depot must not necessarily come from case 1. above) and the \n trips up the ^ in the regex.

Both cases should be fixable by adding a count=1 keyword to the relevant replace calls.
You could add this here too, or shall we open another PR?

The new relocatable cache file work uses simple text substitution when
stripping out the depot from a cache file's paths, and when substituting
it in again over the `@depot` marker.  However, if a user starts julia
with `JULIA_DEPOT_PATH=/opt/foo/`, the embedded path for `Foo.jl`'s
includes list will look like `@depotpackages/Foo/XYZ/src/Foo.jl`, and if
the user then uses `JULIA_DEPOT_PATH=/opt/foo` (which should be
equivalent) the cache file will fail to load with the message:

```
Failed to determine depot from srctext files
```

This commit standardizes the serialization format to always contain a
trailing `pathsep()`, so that textual substitution is more likely to
work regardless of slightly-inconsistent `JULIA_DEPOT_PATH` settings.
It also attempts to dodge a few other edge cases by only replacing the
first instance of `@depot` found.
@staticfloat
Copy link
Sponsor Member Author

Great comments both of you; I've integrated the suggestions here. I didn't create any test cases for those other edge cases, @fatteneder if you have some spare cycles, would you mind working up a test case for those?

@staticfloat staticfloat merged commit 199cac7 into master Oct 27, 2023
7 checks passed
@staticfloat staticfloat deleted the sf/depots_with_a_slash branch October 27, 2023 17:38
fatteneder added a commit to fatteneder/julia that referenced this pull request Oct 28, 2023
giordano pushed a commit that referenced this pull request Oct 31, 2023
#51892 caused depots to be skipped when inserting the `@depot` tags,
because it assumed that `isdirpath(path) == false` means that `path` is
not a directory.
This is fixed here while preserving the path normalization introduced
there.

Furthermore, this adds tests as requested here
#51892 (comment)
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

3 participants