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

relpath broken for windows #23646

Closed
StephenVavasis opened this issue Sep 9, 2017 · 15 comments · Fixed by #38259
Closed

relpath broken for windows #23646

StephenVavasis opened this issue Sep 9, 2017 · 15 comments · Fixed by #38259
Labels
bug Indicates an unexpected problem or unintended behavior filesystem Underlying file system and functions that use it help wanted Indicates that a maintainer wants help on an issue or pull request system:windows Affects only Windows

Comments

@StephenVavasis
Copy link
Contributor

The relpath function does not seem to be working properly for Windows. The first two bad cases below are caused by a failure to be case-insensitive. The third bad case is caused by lack of recognition of drive letters.

julia> relpath("c:/Users/vavasis/Documents", "c:/users/vavasis/Documents")
"..\\..\\..\\Users\\vavasis\\Documents"

julia> relpath("c:/Users/vavasis/Documents", "C:/users/vavasis/Documents")
"..\\..\\..\\..\\c:\\Users\\vavasis\\Documents"

julia> relpath("c:/Users/vavasis/Documents", "E:/")
"..\\c:\\Users\\vavasis\\Documents"
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 9, 2017

The first two bad cases below are caused by a failure to be case-insensitive

I think it's worth noting that the first result is entirely correct. While the default filesystem configuration is case-insensitive, the paths on that file system are still case-sensitive (and case-preserving).

I agree the the results of the second two are malformed however.

@StephenVavasis
Copy link
Contributor Author

One other point to make about this issue: I found it because I was testing out a few changes I made in base by running all the julia tests, and one of them failed: the test set libdl.jl. At first I thought it failed because of the changes I made to base, but in fact the test set libdl.jl fails even in Julia-0.6.0 release due to this bug in relpath. I'm wondering how 0.6.0 was released even though libdl.jl fails. Does the release procedure including running tests in Windows?

@ararslan ararslan added filesystem Underlying file system and functions that use it system:windows Affects only Windows labels Sep 9, 2017
@nalimilan nalimilan added the bug Indicates an unexpected problem or unintended behavior label Sep 9, 2017
@nalimilan
Copy link
Member

Does the release procedure including running tests in Windows?

Yes, every PR needs to pass all tests on Windows via AppVeyor. Can you give more details about the failure you see?

@StephenVavasis
Copy link
Contributor Author

Sure, below is the test run (Julia 0.6.0, Windows 10):

julia> include("c:/Users/testj6/AppData/Local/Julia-0.6.0/share/julia/test/libdl.jl")
Test Failed
  Expression: dl != C_NULL
   Evaluated: Ptr{Void} @0x0000000000000000 != Ptr{Void} @0x0000000000000000
ERROR: LoadError: There was an error during testing
while loading c:\Users\testj6\AppData\Local\Julia-0.6.0\share\julia\test\libdl.jl, in expression starting on line 23

@nalimilan
Copy link
Member

Are you sure that failure is related to relpath? Have you tried adding a few show statements to see what path is used?

@StephenVavasis
Copy link
Contributor Author

Yes, here are some println statements I added to libdl.jl:

C:\Users\testj6>fc c:\Users\Public\Documents\libdl.jl c:\Users\testj6\AppData\Local\Julia-0.6.0\share\julia\test\libdl.jl
Comparing files C:\USERS\PUBLIC\DOCUMENTS\libdl.jl and C:\USERS\TESTJ6\APPDATA\LOCAL\JULIA-0.6.0\SHARE\JULIA\TEST\LIBDL.JL
***** C:\USERS\PUBLIC\DOCUMENTS\libdl.jl
    try
        println("relpath = ", relpath(joinpath(private_libdir, "libccalltest")))
        dl = Libdl.dlopen_e(relpath(joinpath(private_libdir, "libccalltest")))
***** C:\USERS\TESTJ6\APPDATA\LOCAL\JULIA-0.6.0\SHARE\JULIA\TEST\LIBDL.JL
    try
        dl = Libdl.dlopen_e(relpath(joinpath(private_libdir, "libccalltest")))
*****

***** C:\USERS\PUBLIC\DOCUMENTS\libdl.jl
        @test dl != C_NULL
        println("success A")
    finally
***** C:\USERS\TESTJ6\APPDATA\LOCAL\JULIA-0.6.0\SHARE\JULIA\TEST\LIBDL.JL
        @test dl != C_NULL
    finally
*****

and here is the result of running the modified file:

julia> include("c:/Users/Public/Documents/libdl.jl")
relpath = ..\..\..\..\C:\Users\testj6\AppData\Local\Julia-0.6.0\bin\libccalltest
Test Failed
  Expression: dl != C_NULL
   Evaluated: Ptr{Void} @0x0000000000000000 != Ptr{Void} @0x0000000000000000
ERROR: LoadError: There was an error during testing
while loading c:\Users\Public\Documents\libdl.jl, in expression starting on line 23

@robmosys
Copy link

robmosys commented Aug 11, 2019

Still seeing this on julia 1.02. Saw it when passing the build.jl of CxxWrap is passed an absolute path on a different drive.
Easy to replicate:

julia> relpath("c:/blah","d:/blah/bb")
"..\\..\\..\\c:\\blah"

Update: just looked on master, and this is still a problem.

@ixzh
Copy link

ixzh commented Aug 13, 2020

still an issue in V1. 5.

@StefanKarpinski
Copy link
Sponsor Member

Someone with a Windows system should take a crack at this.

@StefanKarpinski StefanKarpinski added the help wanted Indicates that a maintainer wants help on an issue or pull request label Aug 13, 2020
belamenso added a commit to belamenso/julia that referenced this issue Nov 1, 2020
@musm
Copy link
Contributor

musm commented Nov 17, 2020

I think it's worth noting that the first result is entirely correct. While the default filesystem configuration is case-insensitive, the paths on that file system are still case-sensitive (and case-preserving).

According to the MSDN docs for comparisons, they should be case-insensitive:
https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats#case-and-the-windows-file-system

However, directory and file name comparisons are case-insensitive. If you search for a file named "test.txt", .NET file system APIs ignore case in the comparison. "Test.txt", "TEST.TXT", "test.TXT", and any other combination of uppercase and lowercase letters will match "test.txt".

I think we should respect this in relpath in Julia

@musm
Copy link
Contributor

musm commented Nov 17, 2020

Here are some test cases that also are bugs

relpath("D","d")  returns  "..\\D" but   should return `"."`     (this is the case-insensitivity issue mentioned above) 

relpath("d:x","d:a/b/c")  returns "..\\..\\..\\d:x" but should return  "..\\..\\..\\x"      ("d:a/b/c" is a relative path in the current directory in the d drive)
(also test case sensitivity of the drive letters above)

I think we should probably first normalize the paths before doing path comparisons to fix these

@musm
Copy link
Contributor

musm commented Nov 17, 2020

I also think we should just throw an error for examples like

relpath("c:/Users/vavasis/Documents", "E:/")

path is on mount c:, but start is on mount E:/, surely this is not desired and an exception should be thrown.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 17, 2020

According to the MSDN docs for comparisons

That documentation seems actively misleading, as it's not (or at least should not) be in the ".NET file system APIs", though it was once part of the local NTFS configuration. It's also entirely wrong now, since it's not even been part of the NTFS configuration for over 2 years (https://www.windowscentral.com/how-enable-ntfs-treat-folders-case-sensitive-windows-10). There's now a "fork this on github" button at the top if you're interested in fixing this confusing wording for future readers. I've been using that button a couple times lately to fix various documentations issues, to be able rely upon the functions not doing what they claimed.

@musm
Copy link
Contributor

musm commented Nov 17, 2020

There's also this more detailed resource: https://devblogs.microsoft.com/commandline/per-directory-case-sensitivity-and-wsl/

@musm
Copy link
Contributor

musm commented Nov 17, 2020

In any case, let's keep case sensitivity as I agree that this is the safest from a user and dev perspective.

However, we should still fix

relpath("d:x","d:a/b/c")  returns "..\\..\\..\\d:x" but should return  "..\\..\\..\\x"      ("d:a/b/c" is a relative path in the current directory in the d drive)

and we should not allow cases like the following (i.e. we should throw an exception), although keeping the current behavior for now until agreed upon in a follow up after the bugs in this issue are fixed is a better option.

relpath("c:/Users/vavasis/Documents", "E:/")

KristofferC pushed a commit that referenced this issue Feb 1, 2021
staticfloat pushed a commit that referenced this issue Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior filesystem Underlying file system and functions that use it help wanted Indicates that a maintainer wants help on an issue or pull request system:windows Affects only Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants