refactor path model #6509

tomerd · 2023-05-02T08:32:30Z

motivation: deprecate TSC, move to swift-system

changes:

create AbsolutePath and RelativePath in SwiftPM, at this point just a wrapper for the TSC version. In next PR we will start refactoring them
remove all direct imports of TSBasic and replace them with data type specific imports to help migration
reduce sue of TSCTestSupport, stop exporting it from SPMTestSupport
update all call sites and tests

tomerd · 2023-05-02T08:32:44Z

apple/swift-tools-support-core#418

@swift-ci smoke test

tomerd · 2023-05-02T08:35:46Z

Sources/Basics/FileSystem/AbsolutePath.swift

+/// normalization, because it is normally the responsibility of the shell and
+/// not the program being invoked (e.g. when invoking `cd ~`, it is the shell
+/// that evaluates the tilde; the `cd` command receives an absolute path).
+public struct AbsolutePath: Hashable, Sendable {


this is the key change

tomerd · 2023-05-02T08:36:21Z

Sources/Basics/FileSystem/FileSystem+Extensions.swift

+
+// MARK: - Custom path
+
+extension FileSystem {


this is an adapter so FS work with the "new" types

tomerd · 2023-05-02T08:37:51Z

Sources/Basics/FileSystem/RelativePath.swift

+/// This string manipulation may change the meaning of a path if any of the
+/// path components are symbolic links on disk.  However, the file system is
+/// never accessed in any way when initializing a RelativePath.
+public struct RelativePath: Hashable, Sendable {


this is the key change

tomerd · 2023-05-02T08:38:28Z

Sources/Basics/FileSystem/TSCFreeFunctionsAdapter.swift

+// See http://swift.org/LICENSE.txt for license information
+// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//


this is a shim between utility functions in TSC and the "new" types

tomerd · 2023-05-02T08:39:41Z

Sources/Basics/FileSystem/VirtualFileSystem.swift

@@ -119,18 +133,31 @@ private extension FileSystem {
 public class VirtualFileSystem: FileSystem {


concrete FS implementation use the TSC types

tomerd · 2023-05-02T08:46:52Z

apple/swift-tools-support-core#418

@swift-ci smoke test

tomerd · 2023-05-02T08:47:17Z

"self-hosted" tests expected to fail since they do not support cross repo testing

tomerd · 2023-05-02T19:11:00Z

@swift-ci smoke test

tomerd · 2023-05-02T19:26:36Z

@swift-ci smoke test windows

tomerd · 2023-05-02T19:41:23Z

cc @abertelrud @compnerd this is the first step towards cleaning this up

tomerd · 2023-05-02T21:36:01Z

apple/sourcekit-lsp#741

@swift-ci smoke test

tomerd · 2023-05-02T21:40:51Z

apple/sourcekit-lsp#741

@swift-ci smoke test

neonichu · 2023-05-02T21:47:12Z

Sources/Basics/FileSystem/AbsolutePath.swift

+
+    /// Unlike ``AbsolutePath//extension``, this property returns all characters after the first `.` character in a
+    /// filename. If no dot character is present in the filename or first dot is the last character, `nil` is returned.
+    public var allExtensions: [String]? {


I guess this was a Basics-provided extension before? Might be worth keeping that separation?

neonichu · 2023-05-02T21:47:42Z

Sources/Basics/FileSystem/AbsolutePath.swift

+    /// Returns the absolute path with additional extension appended.
+    ///
+    public func appending(extension: String) -> AbsolutePath {
+        guard !self.isRoot else { return self }


same as above

neonichu · 2023-05-02T21:50:08Z

Sources/Basics/Netrc.swift

@@ -74,7 +76,7 @@ public struct Netrc {
    }
 }

-public struct NetrcParser {
+public enum NetrcParser {


swift format made me do it

That's pretty weird?

neonichu · 2023-05-02T21:51:13Z

Sources/Basics/SwiftVersion.swift

@@ -23,11 +23,11 @@ public struct SwiftVersion {
    public var buildIdentifier: String?

    /// The major component of the version number.
-    public var major: Int { return self.version.major }


neonichu

Looks good, some small comments

tomerd · 2023-05-03T00:27:49Z

apple/sourcekit-lsp#741

@swift-ci smoke test

tomerd · 2023-05-03T00:28:51Z

apple/sourcekit-lsp#741

@swift-ci smoke test windows

tomerd · 2023-05-03T03:23:12Z

@compnerd does the windows build failure make sense you? looks like a compiler crash

compnerd · 2023-05-03T03:57:53Z

It is an assertion failure:

Assertion failed: Ptr && "Cannot dereference a null Type!", file C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\swift\include\swift/AST/Type.h, line 249

Looks like a swift compiler issue.

MaxDesiatov · 2023-05-03T10:52:06Z

@swift-ci test windows

tomerd · 2023-05-03T16:35:48Z

@compnerd its only crashing in windows tho, is this something you can help with?

compnerd · 2023-05-03T16:40:44Z

I think that @xymus would be better able to help than I would. However, the last run seems to be failing differently:

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:108:59: error: cannot convert value of type 'TSCBasic.AbsolutePath' to expected argument type 'Basics.AbsolutePath'

    var location = try Workspace.Location(forRootPackage: packageRoot, fileSystem: fileSystem)

                                                          ^

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:179:51: error: cannot convert value of type 'TSCBasic.AbsolutePath' to expected element type 'Basics.AbsolutePath'

      rootInput: PackageGraphRootInput(packages: [packageRoot]),

                                                  ^

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:238:28: error: cannot convert return expression of type 'Basics.AbsolutePath' to return type 'TSCBasic.AbsolutePath'

    return buildParameters.buildPath

           ~~~~~~~~~~~~~~~~^~~~~~~~~

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:242:51: error: cannot convert return expression of type 'AbsolutePath' to return type 'AbsolutePath?'

    return buildParameters.indexStoreMode == .off ? nil : buildParameters.indexStore

           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:360:19: error: cannot convert value of type 'TSCBasic.AbsolutePath' to expected argument type 'Basics.AbsolutePath'

        filePath: path, 

                  ^

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:422:55: error: binary operator '==' cannot be applied to operands of type 'TSCBasic.AbsolutePath' and 'Basics.AbsolutePath'

      for package in packageGraph.packages where path == package.manifest.path {

                                                 ~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~

C:\Users\swift-ci\jenkins\workspace\swiftpm-PR-windows\sourcekit-lsp\Sources\SKSwiftPMWorkspace\SwiftPMWorkspace.swift:506:70: error: binary operator '==' cannot be applied to operands of type 'Basics.AbsolutePath' and 'TSCBasic.AbsolutePath'

    let compilePath = try td.compilePaths().first(where: { $0.source == nativePath })

                                                           ~~~~~~~~~ ^  ~~~~~~~~~~

tomerd · 2023-05-03T17:07:17Z

@compnerd I think its because @MaxDesiatov forgot to include the LSP PR

tomerd · 2023-05-03T17:07:30Z

apple/sourcekit-lsp#741

@swift-ci smoke test windows

motivation: deprecate TSC, move to swift-system changes: * create AbsolutePath and RelativePath in SwiftPM, at this point just a wrapper for the TSC version. In next PR we will start refactoring them * remove all direct imports of TSBasic and replace them with data type specific imports to help migration * reduce sue of TSCTestSupport, stop exporting it from SPMTestSupport * update all call sites and tests

tomerd · 2023-05-04T19:36:33Z

apple/sourcekit-lsp#741

@swift-ci smoke test windows

tomerd · 2023-05-04T19:36:40Z

apple/sourcekit-lsp#741

@swift-ci smoke test

tomerd · 2023-05-04T20:46:26Z

apple/sourcekit-lsp#741

@swift-ci smoke test windows

tomerd requested review from abertelrud, neonichu and MaxDesiatov as code owners May 2, 2023 08:32

tomerd commented May 2, 2023

View reviewed changes

tomerd force-pushed the fractor/path-01 branch from 1032a81 to bf100cc Compare May 2, 2023 08:46

tomerd force-pushed the fractor/path-01 branch 2 times, most recently from d1f03e1 to 2befe02 Compare May 2, 2023 21:40

tomerd mentioned this pull request May 2, 2023

adjust to latest SwiftPM APIs apple/sourcekit-lsp#741

Merged

neonichu reviewed May 2, 2023

View reviewed changes

neonichu approved these changes May 2, 2023

View reviewed changes

MaxDesiatov approved these changes May 3, 2023

View reviewed changes

tomerd force-pushed the fractor/path-01 branch from 6c8873d to 9d249b0 Compare May 4, 2023 19:36

tomerd enabled auto-merge (squash) May 4, 2023 19:37

tomerd merged commit 828c41f into apple:main May 4, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor path model #6509

refactor path model #6509

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd May 2, 2023

tomerd May 2, 2023

tomerd May 2, 2023

tomerd May 2, 2023

tomerd May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023 •

edited

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

neonichu May 2, 2023

neonichu May 2, 2023

neonichu May 2, 2023

tomerd May 2, 2023

neonichu May 2, 2023

neonichu May 2, 2023

neonichu left a comment

tomerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 3, 2023

compnerd commented May 3, 2023

MaxDesiatov commented May 3, 2023

tomerd commented May 3, 2023

compnerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 4, 2023

tomerd commented May 4, 2023

tomerd commented May 4, 2023

		@@ -119,18 +133,31 @@ private extension FileSystem {
		public class VirtualFileSystem: FileSystem {

refactor path model #6509

refactor path model #6509

Conversation

tomerd commented May 2, 2023

tomerd commented May 2, 2023

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 May 2, 2023

tomerd commented May 2, 2023 • edited

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

tomerd commented May 2, 2023

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

neonichu left a comment

Choose a reason for hiding this comment

tomerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 3, 2023

compnerd commented May 3, 2023

MaxDesiatov commented May 3, 2023

tomerd commented May 3, 2023

compnerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 3, 2023

tomerd commented May 4, 2023

tomerd commented May 4, 2023

tomerd commented May 4, 2023

tomerd commented May 2, 2023 •

edited