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

Fix joinpath on Windows and improve joinpath on linux #33477

Merged
merged 4 commits into from
Oct 18, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion LICENSE.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ Julia includes code from the following projects, which have their own licenses:
- [MUSL](https://git.musl-libc.org/cgit/musl/tree/COPYRIGHT) (for getopt implementation on Windows) [MIT]
- [MINGW](https://sourceforge.net/p/mingw/mingw-org-wsl/ci/legacy/tree/mingwrt/mingwex/dirname.c) (for dirname implementation on Windows) [MIT]
- [NetBSD](https://www.netbsd.org/about/redistribution.html) (for setjmp, longjmp, and strptime implementations on Windows) [BSD-3]
- [Python](https://docs.python.org/2/license.html) (for strtod implementation on Windows) [BSD-3, effectively]
- [Python](https://docs.python.org/2/license.html) (for strtod and joinpath implementation on Windows) [BSD-3, effectively]

The following components included in Julia `Base` have their own separate licenses:

Expand Down
86 changes: 63 additions & 23 deletions base/path.jl
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,6 @@ function splitext(path::String)
a*m.captures[1], String(m.captures[2])
end

function pathsep(paths::AbstractString...)
for path in paths
m = match(path_separator_re, String(path))
m !== nothing && return m.match[1:1]
end
return path_separator
end

"""
splitpath(path::AbstractString) -> Vector{String}

Expand Down Expand Up @@ -246,8 +238,69 @@ function splitpath(p::String)
return out
end

joinpath(path::AbstractString)::String = path

if Sys.iswindows()

function joinpath(path::AbstractString, paths::AbstractString...)::String
result_drive, result_path = splitdrive(path)

local p_drive, p_path
for p in paths
p_drive, p_path = splitdrive(p)

if startswith(p_path, ('\\', '/'))
# second path is absolute
if !isempty(p_drive) || !isempty(result_drive)
result_drive = p_drive
end
result_path = p_path
continue
elseif !isempty(p_drive) && p_drive != result_drive
if lowercase(p_drive) != lowercase(result_drive)
# different drives, ignore the first path entirely
result_drive = p_drive
result_path = p_path
continue
end
result_drive = p_drive # same drive in different case
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result_drive = p_drive # same drive in different case

Just delete this line, I think.

end

# second path is relative to the first
if !isempty(result_path) && result_path[end] ∉ ('\\', '/')
StefanKarpinski marked this conversation as resolved.
Show resolved Hide resolved
result_path *= "\\"
end

result_path = result_path * p_path
end

# add separator between UNC and non-absolute path
if !isempty(p_path) && result_path[1] ∉ ('\\', '/') && !isempty(result_drive) && result_drive[end] != ':'
return result_drive * "\\" * result_path
end

return result_drive * result_path
end

else

function joinpath(path::AbstractString, paths::AbstractString...)::String
for p in paths
if isabspath(p)
path = p
elseif isempty(path) || path[end] == '/'
path *= p
else
path *= "/" * p
end
end
return path
end

end # os-test

"""
joinpath(parts...) -> AbstractString
joinpath(parts::AbstractString...) -> String

Join path components into a full path. If some argument is an absolute path or
(on Windows) has a drive specification that doesn't match the drive computed for
Expand All @@ -259,20 +312,7 @@ julia> joinpath("/home/myuser", "example.jl")
"/home/myuser/example.jl"
```
"""
joinpath(a::AbstractString, b::AbstractString, c::AbstractString...) = joinpath(joinpath(a,b), c...)
joinpath(a::AbstractString) = a

function joinpath(a::String, b::String)
StefanKarpinski marked this conversation as resolved.
Show resolved Hide resolved
isabspath(b) && return b
A, a = splitdrive(a)
B, b = splitdrive(b)
!isempty(B) && A != B && return string(B,b)
C = isempty(B) ? A : B
isempty(a) ? string(C,b) :
occursin(path_separator_re, a[end:end]) ? string(C,a,b) :
string(C,a,pathsep(a,b),b)
end
joinpath(a::AbstractString, b::AbstractString) = joinpath(String(a), String(b))
joinpath

"""
normpath(path::AbstractString) -> AbstractString
Expand Down
18 changes: 18 additions & 0 deletions test/path.jl
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,33 @@
@test isdirpath(S(".."))
end
@testset "joinpath" begin
@test joinpath(S("")) == ""
@test joinpath(S("foo")) == "foo"
@test joinpath(S("foo"), S("bar")) == "foo$(sep)bar"
@test joinpath(S("foo"), S("bar"), S("baz")) == "foo$(sep)bar$(sep)baz"
@test joinpath(S("foo"), S(""), S("baz")) == "foo$(sep)baz"
@test joinpath(S("foo"), S(""), S("")) == "foo$(sep)"
@test joinpath(S("foo"), S(""), S(""), S("bar")) == "foo$(sep)bar"

@test joinpath(S("foo"), S(homedir())) == homedir()
@test joinpath(S(abspath("foo")), S(homedir())) == homedir()

if Sys.iswindows()
@test joinpath(S("foo"),S("bar:baz")) == "bar:baz"
@test joinpath(S("C:"),S("foo"),S("D:"),S("bar")) == "D:bar"
@test joinpath(S("C:"),S("foo"),S("D:bar"),S("baz")) == "D:bar$(sep)baz"

# relative folders and case-insensitive drive letters
@test joinpath(S("C:\\a\\b"), S("c:c\\e")) == "c:\\a\\b\\c\\e"
Copy link
Member

Choose a reason for hiding this comment

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

Should the earlier or later capitalization of the drive name be kept?

Copy link
Contributor Author

@musm musm Oct 10, 2019

Choose a reason for hiding this comment

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

The preference is to keep the later capitalization. This follows the general rule to prefer p2 in joinpath(p1,p2), or if p2 is absolute to override p1 completely.

Copy link
Member

Choose a reason for hiding this comment

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

What does Python do here? I would think that in the case where a is absolute and b is relative, if they are joinable (i.e. the drives are compatible) one would want joinpath(a, b) to start with a and end with b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does Python do here? I

Python keeps the later capitalization. But it doesn't really matter, since it is case-insentive.

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 interesting. I was thinking that this might affect the definition of relpath but that doesn't seem to depend on joinpath at all in its logic, although I suspect it probably does have a bug in the case of drive letters with case mismatches.

Copy link
Contributor Author

@musm musm Oct 10, 2019

Choose a reason for hiding this comment

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

I see, honestly I haven't had a close look at relpath to tell how this affects relpath. My general thinking is that it would be a good idea to audit all of our FileSystem API, to check how we do with UNC paths and relative paths with drive specifications. We also don't support extended paths, which is another good extension in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@test joinpath(S("C:\\a\\b"), S("c:c\\e")) == "c:\\a\\b\\c\\e"
@test joinpath(S("C:\\a\\b"), S("c:c\\e")) == "C:\\a\\b\\c\\e"

And change this test.


# UNC paths
@test joinpath(S("\\\\server"), S("share")) == "\\\\server\\share"
@test joinpath(S("\\\\server"), S("share"), S("a")) == "\\\\server\\share\\a"
@test joinpath(S("\\\\server\\"), S("share"), S("a")) == "\\\\server\\share\\a"
@test joinpath(S("\\\\server"), S("share"), S("a"), S("b")) == "\\\\server\\share\\a\\b"
@test joinpath(S("\\\\server\\share"),S("a")) == "\\\\server\\share\\a"
@test joinpath(S("\\\\server\\share\\"), S("a")) == "\\\\server\\share\\a"

elseif Sys.isunix()
@test joinpath(S("foo"),S("bar:baz")) == "foo$(sep)bar:baz"
@test joinpath(S("C:"),S("foo"),S("D:"),S("bar")) == "C:$(sep)foo$(sep)D:$(sep)bar"
Expand Down