[DNM] Refactor Path APIs with SwiftSystem #219

stevapple · 2021-06-06T04:31:36Z

This patch contains API changes. Minor version bump required.

Refactor Path APIs with SwiftSystem, and meanwhile align some APIs’ behavior. This includes:

No longer allow blank component "". This was in the doc but not actually taken into practice.
No longer include root component in AbsolutePath.components.
Add public var root: String? { get } for all Path instances.
Creating RelativePath against another AbsolutePath may throw now, because AbsolutePaths could have different roots on Windows.
Deprecate AbsolutePath.root for better stability and clearer usage.

TBD:

Fix tests on Windows.

This PR may require parallel changes in all the dependent packages:

swift-llbuild
swift-package-manager
sourcekit-lsp
swift-driver
swift-llbuild
swiftpm-on-llbuild2
swift-stress-tester
swift-tools-support-async

stevapple · 2021-06-06T04:34:00Z

Please point out if we can minimize the impact while keeping correct behavior.

Also CC @compnerd @milseman

milseman

This LGTM. I'm not a domain expert on SPM or this use case. @compnerd what do you think about this approach?

(Sorry for the delay. I had a 90% completed review but got distracted before I could finish, and forgot about it)

milseman · 2021-06-08T18:14:09Z

Sources/TSCBasic/Path.swift

+    /// Public initializer from String.
+    init(_ string: String)
+
+    /// Convenience initializer that verifies that the path lexically.


What does it verify?

The initializer will throw if the argument isn’t a valid relative/absolute path.

milseman · 2021-06-08T18:18:44Z

Sources/TSCBasic/Path.swift

+    /// component: the `.` string.
+    ///
+    /// NOTE: Path components no longer include the root.  Use `root` instead.
+    var components: [String] { get }


What's the main reason for this protocol in SPM?

Also, should these methods be customization hooks or should they just be on the extension? How many of them should be dropped or superseded by FilePath?

What's the main reason for this protocol in SPM?

These are just common APIs of RelativePath and AbsolutePath.

How many of them should be dropped or superseded by FilePath?

I don’t think I’m capable enough of answering this question. If we’d like to drop any of them, maybe we’d get other TSC specialists in here.

milseman · 2021-06-08T18:21:42Z

Sources/TSCBasic/Path.swift

+            return "."
+        }
+        return filepath.string
+    }


We might want to put this on FilePath and Component itself, actually. We went with "" as the canonical empty path over "." because that's what it is at the OS layer as well as Collection semantics, but a common convention is to produce ".".

We'd want to come up with a good name for it for both FilePath and Component. Something better than stringOrCurrentDirectory.

You could consider extending FilePath to add it, picking your preferred name. I don't know if that's an easier way to structure your code for the future. Let me ask some folk's opinion on extending types from dependencies: @lorentey @natecook1000

public var pathString: String { filepath.isEmpty ? "." : filepath.string }

milseman · 2021-06-08T18:22:55Z

Sources/TSCBasic/Path.swift

+            return "."
+        }
+        return dirname
+    }


public var dirname: String { filepath.removingLastComponent().pathString }

milseman · 2021-06-08T19:08:31Z

Sources/TSCBasic/Path.swift

+            return AbsolutePath("/")
+#else
+            if let drive = ProcessEnv.vars["SystemDrive"] ?? ProcessEnv.vars["HomeDrive"] {
+                return AbsolutePath(drive + "\\")


Why the trailing backslash?

%SystemDrive% appears like C:, and C:\ is the file path of its root.

milseman · 2021-06-18T14:06:26Z

Package.swift

@@ -28,7 +28,9 @@ let package = Package(
            name: "TSCTestSupport",
            targets: ["TSCTestSupport"]),
    ],
-    dependencies: [],
+    dependencies: [
+        .package(url: "https://github.com/apple/swift-system.git", .upToNextMinor(from: "0.0.1"))


Should this be 0.0.1 or 0.0.2?

It should be 0.0.3 given that the first two releases actually not compiles (0.0.2 only compiles with release config).

upToNextMinor() indicates the latest 0.0.x, once 0.0.3 is released it should resolve.

milseman · 2021-06-18T14:27:00Z

Sources/TSCBasic/Path.swift

+    }
+
+    public func appending(components names: [String]) -> Self {
+        let components = names.map(FilePath.Component.init)


You can also consider throwing in a .lazy.

milseman · 2021-06-18T14:32:41Z

Sources/TSCBasic/Path.swift

-    /// Root directory (whose string representation is just a path separator).
-    public static let root = AbsolutePath(PathImpl.root)
+    /// Returns the lowest common ancestor path.
+    public func lowestCommonAncestor(with path: AbsolutePath) -> AbsolutePath? {


Common ancestor queries is also a good candidate for FilePath. We had operations like lexicallyRelative(to:) in earlier pitches.

milseman · 2021-06-18T14:35:17Z

Sources/TSCBasic/Path.swift

-            // Special case, which is a plain path without `..` components.  It
-            // might be an empty path (when self and the base are equal).
-            let relComps = pathComps.dropFirst(baseComps.count)
+    public func relative(to base: AbsolutePath) throws -> RelativePath {


Another candidate for FilePath.

milseman · 2021-06-18T14:39:17Z

Sources/TSCBasic/PathShims.swift

-        } else if self.pathString.hasPrefix(dir.pathString + "/") {
-            return "./" + self.relative(to: dir).pathString
+        if let first = rel.components.first,
+            first != ".." {


What is the situation you're checking here? ..s should be removed as part of lexical normalization except at the start of relative paths.

I just copied the old behavior here. The old code shows that the returned string starts with either .. or ..

tomerd · 2021-07-07T19:42:01Z

@stevapple what is the status of this work? is this something you want and have the time to bring over the finish line? we are very interested in this.

stevapple · 2021-07-08T05:33:16Z

@tomerd I’m afraid not yet. Here’s a simple checklist:

We would like to keep using POSIX path style for in-memory file systems, and POSIX FilePath is not supported on Windows yet. We’d better get @milseman in to see if it’s possible to add support for that. This would help to remove the .posix() workaround, which largely reduces API breakage.
Because this is an API breaking change, we also need to update and dependent projects against it. Such work needs to be done after the API is fixed.

@milseman also suggests to move some implementation details to swift-system, but this could be addressed in another PR.

tomerd · 2022-02-02T20:56:18Z

@stevapple @compnerd we now have swift-system integrated into the main branch of TSC and can start making use of it / reduce code to help support windows paths etc better

stevapple · 2022-02-03T19:10:56Z

@stevapple @compnerd we now have swift-system integrated into the main branch of TSC and can start making use of it / reduce code to help support windows paths etc better

IMO FilePath is too platform-specific to implement the general interface of TSC. For example, InMemoryFileSystem can only represent a simple FS, which falls to a rather small part of the Windows path system. By using FilePath, we need to take extra care about these unsupported paths.

My suggested migration path is to directly rewriting TSC's clients with FilePath, until we can finally deprecate the whole path submodule in TSC.

I'm also working on a high-level and cross-platform path package (something like karwa/swift-url, but I think that help little to TSC.

into filepath

stevapple · 2022-03-01T14:56:43Z

@tomerd @compnerd @milseman

While it's still undecided whether we're able to split POSIXPath out of FilePath, it's now possible for us to start the migration.

I introduced a convention that will automatically rebase a /-based path string to the system root, which enables smoother transition for non-Windows file systems.

One big breaking change now is that AbsolutePath.relative(to:) throws, although it rarely throws in production IMO. The other is about splitting root off components, which is relatively easy to follow in unit tests.

stevapple · 2022-03-01T15:01:00Z

Some tests are failing / skipped because of:

helper functions broken on Windows;
path separator being \; on Windows instead of /:;
command line tools missing or different on Windows;
difference in executable and process;
difference in file permission;
bugs of Foundation.

stevapple · 2022-03-08T20:15:15Z

For anyone who may be concerned, you can start to review the code now.

I would investigate the impact upon TSC clients and open corresponding PR to test it with CI.

stevapple added 5 commits June 6, 2021 11:32

Reimplement Path API with SwiftSystem

d7ec183

Drop Pathcch dependency

b127725

Fix style

d8888c6

Clarify the change of Path.components

49264e3

Deprecate static method AbsolutePath.root

76ae479

stevapple requested review from abertelrud, aciidgh, friedbunny, neonichu and tomerd as code owners June 6, 2021 04:31

Minor style changes

55565ba

stevapple force-pushed the filepath branch from 3903c62 to 55565ba Compare June 6, 2021 04:51

stevapple added 2 commits June 6, 2021 21:22

Minor update

39704db

Fix build

44f7f25

stevapple force-pushed the filepath branch from dc4a184 to 44f7f25 Compare June 6, 2021 13:32

This was referenced Jun 6, 2021

[Experimental][DNM] Use SwiftSystem for file operations #221

Draft

Improve path normalization and judgment on Windows #191

Closed

stevapple added 4 commits June 7, 2021 00:58

Minor fix

2727fbf

Rename Path to FileSystemPath

df853dc

Improve stability on Windows

d27bd63

Fix some tests on Windows

08ece27

stevapple mentioned this pull request Jun 8, 2021

Fix building on Windows apple/swift-system#51

Merged

stevapple marked this pull request as draft June 8, 2021 07:43

stevapple mentioned this pull request Jun 8, 2021

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

Closed

15 tasks

milseman approved these changes Jun 18, 2021

View reviewed changes

stevapple mentioned this pull request Aug 12, 2021

[GSoC] SwiftPM PR stevapple/swift-package-manager#1

Open

stevapple added 2 commits March 1, 2022 21:06

Merge branch 'main' of https://github.com/apple/swift-tools-support-core

f3bac65

into filepath

Deprecate AbsolutePath.withPOSIX(path:)

3b3b779

stevapple force-pushed the filepath branch from a77a2e2 to c634ee4 Compare March 1, 2022 15:17

tomerd added the wip Work in progress label Mar 2, 2022

stevapple added 2 commits March 9, 2022 04:28

Make tests run on Windows.

6b1b327

Complete AbsolutePath.relative(to:)

82d7f53

stevapple force-pushed the filepath branch from c634ee4 to 82d7f53 Compare March 8, 2022 20:29

stevapple mentioned this pull request Mar 16, 2022

Improve finding executable on Windows #175

Open

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

[DNM] Refactor Path APIs with SwiftSystem #219

[DNM] Refactor Path APIs with SwiftSystem #219

stevapple commented Jun 6, 2021 •

edited

Loading

stevapple commented Jun 6, 2021

milseman left a comment

milseman Jun 8, 2021

stevapple Jun 18, 2021

milseman Jun 8, 2021

stevapple Jun 18, 2021 •

edited

Loading

milseman Jun 8, 2021

milseman Jun 8, 2021

milseman Jun 8, 2021

stevapple Jun 18, 2021

milseman Jun 18, 2021

stevapple Jun 18, 2021

milseman Jun 18, 2021

milseman Jun 18, 2021

milseman Jun 18, 2021

milseman Jun 18, 2021

stevapple Jun 18, 2021

tomerd commented Jul 7, 2021 •

edited

Loading

stevapple commented Jul 8, 2021

tomerd commented Feb 2, 2022

stevapple commented Feb 3, 2022 •

edited

Loading

stevapple commented Mar 1, 2022 •

edited

Loading

stevapple commented Mar 1, 2022 •

edited

Loading

stevapple commented Mar 8, 2022

[DNM] Refactor Path APIs with SwiftSystem #219

Are you sure you want to change the base?

[DNM] Refactor Path APIs with SwiftSystem #219

Conversation

stevapple commented Jun 6, 2021 • edited Loading

stevapple commented Jun 6, 2021

milseman left a 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 Jun 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd commented Jul 7, 2021 • edited Loading

stevapple commented Jul 8, 2021

tomerd commented Feb 2, 2022

stevapple commented Feb 3, 2022 • edited Loading

stevapple commented Mar 1, 2022 • edited Loading

stevapple commented Mar 1, 2022 • edited Loading

stevapple commented Mar 8, 2022

stevapple commented Jun 6, 2021 •

edited

Loading

stevapple Jun 18, 2021 •

edited

Loading

tomerd commented Jul 7, 2021 •

edited

Loading

stevapple commented Feb 3, 2022 •

edited

Loading

stevapple commented Mar 1, 2022 •

edited

Loading

stevapple commented Mar 1, 2022 •

edited

Loading