Improve path normalization and judgment on Windows #191

stevapple · 2021-02-23T10:22:11Z

Fixes the problem of .. being ignored during normalizing;
Avoids crash when validating an empty path.

tomerd · 2021-02-23T18:56:49Z

@swift-ci test

tomerd · 2021-02-23T18:57:02Z

@compnerd, this seems reasonable to me

Sources/TSCBasic/Path.swift

compnerd · 2021-02-25T15:33:18Z

Sources/TSCBasic/Path.swift

+#else
+        pathSeparator = "/"
+#endif
+        precondition(path.first != pathSeparator)


This precondition doesn't hold. \path is valid as a relative path. It is relative to the current drive.

Although it is literally "relative", Windows Shell (and Swift which depends on it) regards \path \?\path \\share\path as absolute paths (according to the response of PathIsRelativeW). Personally I think these paths should be treated the same with a regular absolute path. For example, \path\..\.. indicates \ like C:\path\..\.. indicates C:\, but path\..\.. indicates ...

UNC paths are always absolute. It is just a drive relative path which is not absolute. That distinction is important as it impacts the lexical traversal of the path string.

PathIsRelativeW regards \path as an absolute path. Also, if we treat it as relative, behavior regarding .. will be unaligned.

Ping @compnerd

I still think that the differences are material and need to be addressed. Paths in Windows do not behave like Unix, and so you will have differences. e.g.

path1 = "C:\Users\compnerd"
path2 = "D:"
path1 + path2 is not `C:\Users\compnerd\D:" but rather "C:\Windows\System32\WindowsMetadata"

because the current directory on D: happened to be "....\Windows\System32\WindowsMetadata"

I’ll look into more critical cases here.

However, if I regard \path as a relative path, it will change the behavior of a bunch of APIs (while this PR focuses on bug fixes), so that change definitely needs to be addressed in a separate PR.

IMO I believe this PR is fully capable of fixing faulty behavior without changing the current judgement of relative paths and absolute paths. To fully support Windows path system, we’d better work on another one which may contain a complete refactoring.

What this PR mainly addressed and fixed is the problem of .. being ignored, and of crashing on an empty path, which are critical bugs for tools like SwiftPM.

Sources/TSCBasic/Path.swift

compnerd · 2021-02-26T03:14:31Z

Sources/TSCBasic/Path.swift

+#else
+        pathSeparator = "/"
+#endif
+        precondition(path.first != pathSeparator)


UNC paths are always absolute. It is just a drive relative path which is not absolute. That distinction is important as it impacts the lexical traversal of the path string.

Sources/TSCBasic/Path.swift

compnerd · 2021-03-05T18:47:54Z

Sources/TSCBasic/Path.swift

+#else
+        pathSeparator = "/"
+#endif
+        precondition(path.first != pathSeparator)


I still think that the differences are material and need to be addressed. Paths in Windows do not behave like Unix, and so you will have differences. e.g.

path1 = "C:\Users\compnerd"
path2 = "D:"
path1 + path2 is not `C:\Users\compnerd\D:" but rather "C:\Windows\System32\WindowsMetadata"

because the current directory on D: happened to be "....\Windows\System32\WindowsMetadata"

stevapple · 2021-03-06T17:27:03Z

If possible, I’d like to completely refactor Path using swift-system after FilePath API is accepted. I don’t see the need to handle those tricky cases repeatedly in separate packages.

tomerd · 2021-04-07T23:11:55Z

If possible, I’d like to completely refactor Path using swift-system after FilePath API is accepted. I don’t see the need to handle those tricky cases repeatedly in separate packages.

@stevapple we'd love to see PRs to that end, its definitely the right direction

stevapple · 2021-04-07T23:31:31Z

CC @compnerd

Regardless of any future improvements, the bug fixed in this PR is critical for correcting SwiftPM behavior on Swift 5.4 and later. I’d like to see it merged into release/5.4 before the official release.

stevapple · 2021-04-15T05:17:20Z

@tomerd Could you merge this or touch @compnerd ? He seems out of track of this PR.
But it’s critical for SwiftPM on Windows in Swift 5.4. We’re running out of time.

compnerd · 2021-04-16T03:01:17Z

I think that this change is way too far for the 5.4 release.

I think that it is important to address the issues with the model though.

stevapple · 2021-04-16T03:07:18Z

I’d expect System to bring the fully-functional path parsing.

Shipping a SwiftPM which cannot resolve .package(path: "../AnotherPackage") with 5.4 is definitely not what we’d like to see.

stevapple · 2021-04-19T11:31:04Z

@ddunbar May you take a look at this PR? :) I believe that's a critical one for the 5.4 release.

Also @tomerd here because this mainly solves a bug in SwiftPM.

tomerd · 2021-04-19T16:17:20Z

I’d expect System to bring the fully-functional path parsing.

I propose a phased approach:

First, please work with @compnerd to get this PR to a mergeable state.

Later, we should start landing PRs that refactor TSC to use SwiftSystem.

Shipping a SwiftPM which cannot resolve .package(path: "../AnotherPackage") with 5.4 is definitely not what we’d like to see.

its too late for 5.4.0, but once we merge this PR into main, we should open a PR against the 5.4 branch so that if/when we roll a 5.4.1 dot release we can include it

stevapple · 2021-04-20T00:46:03Z

First, please work with @compnerd to get this PR to a mergeable state.

I’m just missing the point to evolve this PR. I believe in its current state it’s making things better (but not perfect yet) without introducing new problems.

@compnerd Can you make a detailed review on the codes?

stevapple · 2021-05-11T19:55:40Z

Any future directions? @compnerd

compnerd · 2021-05-11T20:01:07Z

I don't think that all the previous comments on the change have been addressed yet.

stevapple · 2021-05-11T20:15:41Z

The current implementation of TSC does not meet the Windows specifications. More sadly, it will give false outputs even with regular cases like ../anotherPackage (due to the poor PathCanonicalize API).

I would address again that this PR is only meant to correct the behavior for regular and critical cases. This module needs a complete rewrite if you expect it to fully support Windows paths. This is just a capable bug fix — and it's enough to make Swift tools robuster.

compnerd · 2021-05-11T20:29:03Z

I'm not sure that this is the right way to handle that. It just behaves differently after this change. That doesn't make it more robust. I agree with you that this is in need of a rewrite - and really, this seems like the ideal case and time to migrate the path handling to Swift System. The 5.5 release branch is already in slushy freeze, so we have plenty of time for the next release to get the implementation migrated and corrected.

stevapple · 2021-05-11T20:41:01Z

It just behaves differently after this change.

I would like to know which cases this PR breaks... I think we'd better get a workaround for 5.4, and then get to rewrite it with Swift System for the 5.5 release.

compnerd · 2021-05-11T20:45:08Z

I would like to know which cases this PR breaks... I think we'd better get a workaround for 5.4, and then get to rewrite it with Swift System for the 5.5 release.

It already is too late for all of those. Such a large change for 5.4 is entirely out of the question - it is already finalized. I think that such a change for 5.5 is unreasonable. It is too far in the cycle to make that type of change. This would be best slated for the next release for which we have plenty of time to do by means of replacing the path handling with Swift System.

stevapple · 2021-05-11T21:08:22Z

It already is too late for all of those. Such a large change for 5.4 is entirely out of the question - it is already finalized.

Any critical bug fix should be allowed. I don’t see this PR introducing new behavior AFAK (as for the result) and the bugs of old implementation are leading to unexpected crashes and extremely strange behavior.

stevapple · 2021-05-11T21:13:08Z

Why I regard it as critical — Local debugging relies largely on relative paths, and for now we’re not even able to set the path of a dependency package to ../package. This is unacceptable for user experience and will trouble toolchain development on Windows too.

Since we’re able to fix it, why not? Letting the bug alone with 5.4 while there’re still plenty of 5.4.xs to come…?

stevapple · 2021-05-27T10:40:54Z

import WinSDK

let path = CommandLine.argc > 1 ? CommandLine.arguments[1] : "..\\swift-tools-support-core"
print("user input: " + path)
var buffer: [WCHAR] = Array<WCHAR>(repeating: 0, count: Int(MAX_PATH + 1))
_ = path.withCString(encodedAs: UTF16.self) { PathCanonicalizeW(&buffer, $0) }
print("normalized (current): " + String(decodingCString: buffer, as: UTF16.self))

var parts: [String] = []
var capacity = 0
for part in path.split(separator: "\\") {
    switch part.count {
    case 0: continue
    case 1 where part.first == ".": continue
    case 2 where part.first == "." && part.last == ".":
        guard let prev = parts.last else { fallthrough }
        if !(prev.count == 2 && prev.first == "." && prev.last == ".") {
            parts.removeLast()
            capacity -= prev.count
            continue
        }
        fallthrough
    default:
        parts.append(String(part))
        capacity += part.count
    }
}
capacity += max(parts.count - 1, 0)
var result = ""
result.reserveCapacity(capacity)
var iter = parts.makeIterator()
if let first = iter.next() {
    result.append(contentsOf: first)
    while let next = iter.next() {
        result.append("\\")
        result.append(contentsOf: next)
    }
}
print("normalized (updated): " + (result.isEmpty ? "." : result))

Here is a standalone script to demonstrate the behavioral changes on Windows before and after this patch. I'd treat this as a huge and urgent improvement. The current one is so buggy that it needs to be replaced ASAP on all the active branches.

C:\Users\stevapple>normalize.exe                                 
user input: ..\swift-tools-support-core
normalized (current): swift-tools-support-core
normalized (updated): ..\swift-tools-support-core

C:\Users\stevapple>normalize.exe ..\hello\hello\..\my-package
user input: ..\hello\hello\..\my-package
normalized (current): hello\my-package
normalized (updated): ..\hello\my-package

C:\Users\stevapple>normalize.exe hello\..\..\hello2\..\my-package   
user input: hello\..\..\hello2\..\my-package
normalized (current): \my-package
normalized (updated): ..\my-package

C:\Users\stevapple>normalize.exe .\hello\..\.\hello
user input: .\hello\..\.\hello
normalized (current): \hello
normalized (updated): hello

CC: @compnerd @abertelrud

compnerd · 2021-05-27T15:28:05Z

Why I regard it as critical — Local debugging relies largely on relative paths, and for now we’re not even able to set the path of a dependency package to ../package. This is unacceptable for user experience and will trouble toolchain development on Windows too.

I strongly disagree with this statement and see this as purely hyperbole, which really does not add much to the discussion. I definitely am using SPM actively both for local and CI purposes, it definitely works fine for local debugging. (I would note that it is not just personal experience, others have reported using SPM on Windows on the Forums, and I've run into other cases on GitHub where packages had started using it).

Here is a standalone script to demonstrate the behavioral changes on Windows before and after this patch. I'd treat this as a huge and urgent improvement. The current one is so buggy that it needs to be replaced ASAP on all the active branches.

That doesn't change the fact that the handling does not work for a variety of paths on Windows. This is a very large change that fixes a very particular case (which would require you to go outside of the source tree, which is really not how SPM normally operates). Thus, this is trying to fix a corner case for a corner case. I think that the efforts are better spent on porting to System.

Alternatively, I would be fine with temporarily unifying the paths with Foundation.

stevapple · 2021-06-06T16:43:35Z

Succeeded by #219

elsh · 2021-07-12T21:45:30Z

Closing as it's superseded by #219

stevapple requested review from abertelrud, aciidgh, friedbunny, neonichu and tomerd as code owners February 23, 2021 10:22

stevapple force-pushed the empty-path branch from ec0cb88 to 8fe451b Compare February 23, 2021 13:56

Handle empty path on Windows

c13a770

stevapple force-pushed the empty-path branch 2 times, most recently from 4d1b06a to 7b892e5 Compare February 23, 2021 14:55

stevapple mentioned this pull request Feb 23, 2021

[DNM][WIP] Implement Windows toolchain swiftlang/swift-driver#419

Closed

15 tasks

Fix normalizing paths

4aff4de

stevapple force-pushed the empty-path branch from c21674a to 4aff4de Compare February 24, 2021 05:15

compnerd requested changes Feb 24, 2021

View reviewed changes

Sources/TSCBasic/Path.swift Outdated Show resolved Hide resolved

Sources/TSCBasic/Path.swift Show resolved Hide resolved

Sources/TSCBasic/Path.swift Outdated Show resolved Hide resolved

Sources/TSCBasic/Path.swift Show resolved Hide resolved

stevapple added 3 commits February 25, 2021 01:48

Minor fix

c048ed9

Manually normalizing relative path on Windows

f3bc044

Style update

8fd9767

compnerd requested changes Feb 25, 2021

View reviewed changes

Various improvements

a279f53

stevapple changed the title ~~Handle empty path on Windows~~ Fix path normalization and judgment on Windows Feb 25, 2021

compnerd requested changes Feb 26, 2021

View reviewed changes

Minor fixes

9aef445

stevapple force-pushed the empty-path branch from 98853de to 9aef445 Compare February 26, 2021 04:11

compnerd requested changes Mar 5, 2021

View reviewed changes

stevapple changed the title ~~Fix path normalization and judgment on Windows~~ Improve path normalization and judgment on Windows Mar 6, 2021

stevapple requested a review from compnerd March 6, 2021 18:12

stevapple force-pushed the empty-path branch from d915513 to 48dfe9d Compare March 7, 2021 13:23

Bug fixes

acc3eaa

stevapple force-pushed the empty-path branch from 48dfe9d to acc3eaa Compare March 7, 2021 13:26

abertelrud assigned compnerd Apr 19, 2021

elsh closed this Jul 12, 2021

stevapple mentioned this pull request Dec 6, 2021

[SR-15549] Relative paths to local dependencies not working on Windows swiftlang/swift-package-manager#4373

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve path normalization and judgment on Windows #191

Improve path normalization and judgment on Windows #191

stevapple commented Feb 23, 2021 •

edited

Loading

tomerd commented Feb 23, 2021

tomerd commented Feb 23, 2021 •

edited

Loading

compnerd Feb 25, 2021

stevapple Feb 25, 2021

compnerd Feb 26, 2021

stevapple Feb 26, 2021

stevapple Mar 1, 2021

compnerd Mar 5, 2021

stevapple Mar 6, 2021

stevapple Mar 6, 2021

compnerd Feb 26, 2021

compnerd Mar 5, 2021

stevapple commented Mar 6, 2021 •

edited

Loading

tomerd commented Apr 7, 2021

stevapple commented Apr 7, 2021

stevapple commented Apr 15, 2021

compnerd commented Apr 16, 2021

stevapple commented Apr 16, 2021

stevapple commented Apr 19, 2021

tomerd commented Apr 19, 2021

stevapple commented Apr 20, 2021

stevapple commented May 11, 2021

compnerd commented May 11, 2021

stevapple commented May 11, 2021 •

edited

Loading

compnerd commented May 11, 2021

stevapple commented May 11, 2021

compnerd commented May 11, 2021

stevapple commented May 11, 2021

stevapple commented May 11, 2021 •

edited

Loading

stevapple commented May 27, 2021 •

edited

Loading

compnerd commented May 27, 2021 •

edited

Loading

stevapple commented Jun 6, 2021 •

edited

Loading

elsh commented Jul 12, 2021

Improve path normalization and judgment on Windows #191

Improve path normalization and judgment on Windows #191

Conversation

stevapple commented Feb 23, 2021 • edited Loading

tomerd commented Feb 23, 2021

tomerd commented Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevapple commented Mar 6, 2021 • edited Loading

tomerd commented Apr 7, 2021

stevapple commented Apr 7, 2021

stevapple commented Apr 15, 2021

compnerd commented Apr 16, 2021

stevapple commented Apr 16, 2021

stevapple commented Apr 19, 2021

tomerd commented Apr 19, 2021

stevapple commented Apr 20, 2021

stevapple commented May 11, 2021

compnerd commented May 11, 2021

stevapple commented May 11, 2021 • edited Loading

compnerd commented May 11, 2021

stevapple commented May 11, 2021

compnerd commented May 11, 2021

stevapple commented May 11, 2021

stevapple commented May 11, 2021 • edited Loading

stevapple commented May 27, 2021 • edited Loading

compnerd commented May 27, 2021 • edited Loading

stevapple commented Jun 6, 2021 • edited Loading

elsh commented Jul 12, 2021

stevapple commented Feb 23, 2021 •

edited

Loading

tomerd commented Feb 23, 2021 •

edited

Loading

stevapple commented Mar 6, 2021 •

edited

Loading

stevapple commented May 11, 2021 •

edited

Loading

stevapple commented May 11, 2021 •

edited

Loading

stevapple commented May 27, 2021 •

edited

Loading

compnerd commented May 27, 2021 •

edited

Loading

stevapple commented Jun 6, 2021 •

edited

Loading