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

Revise Windows implementation of splitdrive #42204

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

GunnarFarneback
Copy link
Contributor

This PR makes three improvements to the Windows splitdrive implementation:

  1. The matched regex is split into pieces and annotated.
  2. Forward and backward slashes are considered equivalent. This fixes splitdrive doesn't respect UNC sharepoints when the path is constructed with forward slashes  #38492.
  3. The patterns in the regex are reordered so that long UNC paths
    and long drive letters are once more recognized. This has been
    broken since splitdrive under windows #19695 (Julia 0.5).

Some things to discuss:

  1. Do we want to support the long forms? They have been broken for a
    long time without causing much of a stir.
  2. Why do we allow more than one drive letter? Should we do that?

@GunnarFarneback
Copy link
Contributor Author

The second commit excludes reserved characters and requires drive letters to actually be one letter. I'm not entirely sure whether this is a good idea. On the plus side it makes the function more accurate, on the minus side it moves this closer to a breaking change. It also becomes stricter than Python's os.path.splitdrive but I'm not sure whether that's a plus or a minus. Feel free to convince me whether to keep or drop this commit.

@musm
Copy link
Contributor

musm commented Apr 7, 2022

The second commit excludes reserved characters and requires drive letters to actually be one letter. I'm not entirely sure whether this is a good idea.

As far as I can tell, it's impossible for the drive letter to be longer than one letter as there can at most be 26 MS-DOS drives.

@musm musm force-pushed the windows_splitdrive branch from 075ed31 to 47ee662 Compare April 7, 2022 20:48
@musm musm added the merge me PR is reviewed. Merge when all tests are passing label Apr 7, 2022
@DilumAluthge
Copy link
Member

The failures in Windows CI look like they are related to the contents of this PR.

@DilumAluthge
Copy link
Member

Error in testset path:
Test Failed at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\path.jl:68
  Expression: joinpath(S("foo"), S("bar:baz")) == "bar:baz"
   Evaluated: "foo\\bar:baz" == "bar:baz"
Error in testset path:
Test Failed at C:\buildbot\worker-tabularasa\tester_win32\build\share\julia\test\path.jl:68
  Expression: joinpath(S("foo"), S("bar:baz")) == "bar:baz"
   Evaluated: "foo\\bar:baz" == "bar:baz"
Error in testset path:
Test Failed at C:\buildbot\worker-tabularasa\tester_win64\build\share\julia\test\path.jl:68
  Expression: joinpath(S("foo"), S("bar:baz")) == "bar:baz"
   Evaluated: "foo\\bar:baz" == "bar:baz"
Error in testset path:
Test Failed at C:\buildbot\worker-tabularasa\tester_win64\build\share\julia\test\path.jl:68
  Expression: joinpath(S("foo"), S("bar:baz")) == "bar:baz"
   Evaluated: "foo\\bar:baz" == "bar:baz"

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 12, 2022
@GunnarFarneback
Copy link
Contributor Author

Some archaeology indicates that this test was introduced in #22978, apparently with the intention that bar:baz should be interpreted as an absolute path. I'm fine with either backing out the requirement that a drive letter is a single letter or updating that test.

According to #20912 (comment), doing it like Python is preferred, or at least was five years ago. Can someone with a Windows Python investigate what

import os
os.path.join("foo", "bar:baz")

returns?

@vtjnash
Copy link
Member

vtjnash commented Apr 12, 2022

Since bar:baz is nonsense as a path construction, I would say chose any result that preserves the bar: appearing somewhere in the resulting path (effectively, making sure it is poisoned also). Since all options do that, you can change the test or the implementation–whichever you prefer.

@vtjnash vtjnash added system:windows Affects only Windows forget me not PRs that one wants to make sure aren't forgotten labels Feb 27, 2023
@mzaffalon
Copy link
Contributor

How does this PR deal with

julia> pwd()
"C:\\Users\\mzaffalon"

julia> joinpath(splitdrive(pwd()))
"\\Users\\mzaffalon"

ref: https://discourse.julialang.org/t/joinpath-fails-on-splitdrive-output/95328

@GunnarFarneback
Copy link
Contributor Author

I expect splitdrive is already working according to specification for that path, and this PR has nothing to do with joinpath.

@mzaffalon
Copy link
Contributor

On Julia v1.8.5 joinpath works fine for the following two cases

julia> joinpath("\\\\svfae01-solid01\\SED", "SE")
"\\\\svfae01-solid01\\SED\\SE"

julia> joinpath("C:\\", "Users\\mzaffalon")
"C:\\Users\\mzaffalon"

@musm
Copy link
Contributor

musm commented Jul 12, 2023

bump?

@GunnarFarneback
Copy link
Contributor Author

I'd encourage someone to pick this up. It's constantly below my attention threshold and I don't have a Windows machine myself.

@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed forget me not PRs that one wants to make sure aren't forgotten labels Nov 1, 2023
@vtjnash
Copy link
Member

vtjnash commented Nov 1, 2023

Merged to master, while removing the second commit (which is breaking, and not really desirable to be that strict, as it means glob patterns won't be handled correctly anymore)

@vtjnash
Copy link
Member

vtjnash commented Nov 2, 2023

Is there somehow a bug in the MozillaCerts_jll that makes it hang similar to the REPL one that @Keno fixed recently?
https://buildkite.com/julialang/julia-master/builds/29602#018b8cb4-a4bd-4883-bc8a-d71361d13ef4

@vtjnash vtjnash merged commit 09cbae8 into JuliaLang:master Nov 3, 2023
@GunnarFarneback GunnarFarneback deleted the windows_splitdrive branch November 4, 2023 10:03
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Nov 4, 2023
@vtjnash vtjnash mentioned this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

splitdrive doesn't respect UNC sharepoints when the path is constructed with forward slashes
6 participants