Windows Support #769

compnerd · 2023-05-11T17:19:35Z

This initial patch series beings the work to support Windows. It is incomplete in the sense that there are multiple issues with the tests that need to be ironed out still. However, the core of the changes seem correct to partially work through the test suite.

Of note is that the implementation here significantly diverges from Linux in at least one one sense: it is generally far more approximate to Darwin as the Windows Swift toolchain is a complete distribution and that includes libclang. This toolchain is also (partially) able to host the ObjC paths though the Foundation headers are not available as that is not part of the toolchain distribution.

One of the biggest issues that currently need to be resolved in this core set of changes is the toolchain installation location.

If it is helpful/reasonable, it should be possible to peel off parts of this series into individual PRs to get them merged earlier rather than later to help get more things in place (and make it easier for others to contribute as well).

One final item of note: this is currently not possible to build for most. This is reliant on a toolchain build that is newer than anything available in a prebuilt snapshot. I hope that we can get a new snapshot built in the next few days.

compnerd · 2023-05-22T15:17:30Z

@jpsim I'm happy to break this down into separate PRs. I still do not have a good solution to the HACK commits, and would love suggestions from you. The latest snapshot 5/18 should be able to build this. There is one failure that I am seeing locally due to the need to re-generate the expected results.

jpsim · 2023-06-13T17:06:48Z

Hi @compnerd, first sorry for the delay (laid off, job search, other priorities, etc).

Second, it's fantastic to see SourceKitten working on Windows, massive kudos 🙌.

My main point of feedback is that some changes break the non-Windows build or functionality. For example, some of the hardcoded paths (e.g. C:\Program Files\Git\cmd\git.exe, .build\checkouts\, C:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swift.exe) apply unconditionally on all OSes, where they should only be applied to Windows.

If you don't mind, I'll cherry pick some of your commits (keeping you as author) that don't break anything in the currently supported platforms to reduce the scope of the changes you want to land in this PR. As those smaller pieces get merged in, you can rebase this one as you fix the build/functionality regressions on macOS/Linux.

compnerd · 2023-06-13T17:10:37Z

Hi @compnerd, first sorry for the delay (laid off, job search, other priorities, etc).

Thank you for the response! Life often gets in the way, I hope that everything has worked out well now.

Second, it's fantastic to see SourceKitten working on Windows, massive kudos 🙌.

Thank you!

My main point of feedback is that some changes break the non-Windows build or functionality. For example, some of the hardcoded paths (e.g. C:\Program Files\Git\cmd\git.exe, .build\checkouts\, C:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swift.exe) apply unconditionally on all OSes, where they should only be applied to Windows.

Absolutely on point. In fact, they are being broken even further as we speak :). They are not entirely portable, and I've tried to call them out in the commits with the "HACK" prefix. They are not ready at all for merging and actually need some guidance on how to best approach the problems there.

If you don't mind, I'll cherry pick some of your commits (keeping you as author) that don't break anything in the currently supported platforms to reduce the scope of the changes you want to land in this PR. As those smaller pieces get merged in, you can rebase this one as you fix the build/functionality regressions on macOS/Linux.

That sounds great! The changes are ordered such that the ones marked as HACK are situated on top to help splitting things that are ready from those that are not.

jpsim · 2023-06-14T16:50:39Z

I've cherry-picked and merged many of the commits from this PR in #779.

Could you please rebase this PR with the remaining changes and I can add feedback inline?

Or you can open new PRs for incremental changes as they're ready if you prefer, up to you.

jpsim · 2023-06-14T16:56:01Z

Tests/SourceKittenFrameworkTests/SourceKitTests.swift

@@ -20,6 +20,210 @@ private let sourcekitStrings: [String] = {
        }
        fatalError("Could not find or load libsourcekitdInProc.so")
    }()
+#elseif os(Windows)


This is now defined in SourceKitStrings+Windows.swift as of #779.

compnerd · 2023-06-14T16:56:44Z

Rebased the changes. I think that we need to add a helper to locate the executable (basically implement a poor which). The location of swiftc.exe is going to change soon as well. This will break this and will need to be aware of looking up the paths by searching Path.

Tests/SourceKittenFrameworkTests/ModuleTests.swift

Xcode is only available on the macOS platform. Rather than enumerating all the other OSes, simply limit the test cases to that platform.

This adds Windows specific handling for dynamic symbol resolution into a library. The `dlopen` and `dlsym` APIs are not portable as they are part of POSIX rather than the C standard. Use the Windows equivalents of `LoadLibraryW` and `GetProcAddress`. TODO: this requires further refinement to detect the Swift toolchain installation.

Ensure that we normalise the path when we substitute the path into the results to properly remove the basename of the fixture.

`\` must be escaped for valid JSON. Ensure that we properly escape the character and form the reference string with the proper representation of the path.

Use the proper spelling for the filepath so that we can remove the basename programmatically subsequently.

This ensures that we are able to remove the basename on the included files when matching the golden references.

Add the content to get the reference data for Windows. This is required to run the tests on Windows.

Windows does not have `/usr/bin/env` nor an approximate. We cannot invoke `git` without an absolute path.

We match a string by a subpath. Because we cannot formulate the string using composition, add the expected spelling.

We should figure out the proper way to identify the toolchain installation rather than assuming the install location.

jpsim · 2023-06-14T17:02:31Z

Tests/SourceKittenFrameworkTests/ModuleTests.swift

+        let cloneArguments = ["clone", "https://github.com/Carthage/Commandant.git"]
+        let cloneResult = Exec.run("C:\\Program Files\\Git\\cmd\\git.exe", cloneArguments, currentDirectory: temporaryURL.path)


Can you introduce new functions in Exec.swift that abstracts platform-specific execution of the commands we use in SourceKitten? For example:

extension Exec { static func runGit(_ arguments: String..., currentDirectory: String = FileManager.default.currentDirectoryPath, stderr: Stderr = .inherit) -> Results { #if os(Windows) let command = #"C:\Program Files\Git\cmd\git.exe"# #else let command = "/usr/bin/env" let arguments = ["git"] + arguments #endif return run(command, arguments, currentDirectory: currentDirectory, stderr: stderr) } }

I like the idea of the helper. However, I do wonder if we should replicate /usr/bin/env? I think that we want that for Windows as well, but does not have that. We could explicitly use CreateProcess but that is a lot of complexity.

The equivalent of /usr/bin/env on windows would certainly be helpful.

Maybe some abstraction should live in https://github.com/apple/swift-system? In any case I’m happy to consider it out of scope for initial Windows support in SourceKitten.

I put together https://github.com/compnerd/Finder and we can simply do which("git") and which("swiftc") - it respects PATHEXT which means that we don't even need to worry about the suffix.

jpsim · 2023-06-14T17:05:19Z

Source/SourceKittenFramework/Module.swift

@@ -50,7 +50,7 @@ public struct Module {
                return nodeModuleName == spmName
            }
            let inputs = node["inputs"]?.array(of: String.self) ?? []
-            return inputs.allSatisfy({ !$0.contains(".build/checkouts/") }) && !nodeModuleName.hasSuffix("Tests")
+            return inputs.allSatisfy({ !$0.contains(".build\\checkouts\\") }) && !nodeModuleName.hasSuffix("Tests")


Suggested change

return inputs.allSatisfy({ !$0.contains(".build\\checkouts\\") }) && !nodeModuleName.hasSuffix("Tests")

#if os(Windows)

let checkoutsPath = #".build\checkouts\"#

#else

let checkoutsPath = ".build/checkouts/"

#endif

return inputs.allSatisfy({ !$0.contains(checkoutsPath) }) && !nodeModuleName.hasSuffix("Tests")

Source/SourceKittenFramework/library_wrapper.swift

jpsim · 2023-06-14T17:09:44Z

Tests/SourceKittenFrameworkTests/ClangTranslationUnitTests.swift

@@ -30,8 +30,14 @@ class ClangTranslationUnitTests: XCTestCase {
    }

    private func compare(clangFixture fixture: String) {
-        let unit = ClangTranslationUnit(headerFiles: [fixturesDirectory + fixture + ".h"],
-                                        compilerArguments: ["-x", "objective-c", "-isysroot", sdkPath(), "-I", fixturesDirectory])
+        let path = URL(fileURLWithPath: fixturesDirectory + fixture + ".h")


This pattern is used a lot now: URL(fileURLWithPath: ...).standardizedFileURL.withUnsafeFileSystemRepresentation { String(cString: $0!) }

Can you add some sort of shared helper for this? Open to API design ideas, but as a strawman: FileUtils.unsafeSystemPath(for path: String) -> String

I don't think that the unsafeSystemPath makes sense, perhaps FileUtils.nativePathRepresentation? The withUnsafeFileSystemRepresentation is because the closure receives a UnsafePointer<CChar>, but we are transacting in String at this layer.

jpsim · 2023-06-14T17:10:23Z

Tests/SourceKittenFrameworkTests/SwiftDocsTests.swift

-    let jsonString = String(describing: jsonString).replacingOccurrences(of: escapedFixturesDirectory, with: "")
+    let escapedFixturesDirectory = URL(fileURLWithPath: rootDirectory).standardizedFileURL.withUnsafeFileSystemRepresentation {
+        String(cString: $0!) + FileManager.pathSeparator
+    }.replacingOccurrences(of: "\\", with: "\\\\").replacingOccurrences(of: "/", with: "\\/")


Same here, could you please use raw string literals to avoid having to escape \ which adds a lot of noise?

compnerd force-pushed the windows branch 2 times, most recently from 50a3d0f to b832600 Compare May 15, 2023 16:40

compnerd marked this pull request as ready for review May 22, 2023 15:15

compnerd mentioned this pull request May 22, 2023

Windows Port realm/SwiftLint#5030

Open

jpsim mentioned this pull request Jun 14, 2023

Add partial Windows support #779

Merged

compnerd force-pushed the windows branch from b832600 to 9eb1dd5 Compare June 14, 2023 16:53

jpsim reviewed Jun 14, 2023

View reviewed changes

Tests/SourceKittenFrameworkTests/ModuleTests.swift Outdated Show resolved Hide resolved

compnerd and others added 10 commits June 14, 2023 09:59

Tests: disable Xcode tests on non-macOS platforms

ed06601

Xcode is only available on the macOS platform. Rather than enumerating all the other OSes, simply limit the test cases to that platform.

Tests: normalise the fixtures path for Windows

b3711f5

Ensure that we normalise the path when we substitute the path into the results to properly remove the basename of the fixture.

Tests: escape the path separator for Windows

b2d02cd

`\` must be escaped for valid JSON. Ensure that we properly escape the character and form the reference string with the proper representation of the path.

Tests: normalise the filename before translation

3288433

Use the proper spelling for the filepath so that we can remove the basename programmatically subsequently.

Tests: normalise the include directory for translations

8fa7cfd

This ensures that we are able to remove the basename on the included files when matching the golden references.

Tests: add reference models for Windows

ded59b2

Add the content to get the reference data for Windows. This is required to run the tests on Windows.

HACK: hardcode a path to the git binary

8157d4e

Windows does not have `/usr/bin/env` nor an approximate. We cannot invoke `git` without an absolute path.

HACK: change the path arc separator

2f8a9f6

We match a string by a subpath. Because we cannot formulate the string using composition, add the expected spelling.

HACK: hardcode toolchain location

4272c79

We should figure out the proper way to identify the toolchain installation rather than assuming the install location.

compnerd force-pushed the windows branch from 9eb1dd5 to 4272c79 Compare June 14, 2023 17:01

jpsim reviewed Jun 14, 2023

View reviewed changes

compnerd mentioned this pull request Dec 27, 2023

The default swiftlint.path value is invalid on Windows vknabel/vscode-swiftlint#67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Support #769

Windows Support #769

compnerd commented May 11, 2023 •

edited

Loading

compnerd commented May 22, 2023

jpsim commented Jun 13, 2023

compnerd commented Jun 13, 2023

jpsim commented Jun 14, 2023

jpsim Jun 14, 2023

compnerd commented Jun 14, 2023

jpsim Jun 14, 2023

compnerd Jun 14, 2023

jpsim Jun 14, 2023

compnerd Jun 26, 2023

jpsim Jun 14, 2023

jpsim Jun 14, 2023

compnerd Jun 14, 2023

jpsim Jun 14, 2023

		let cloneArguments = ["clone", "https://github.com/Carthage/Commandant.git"]
		let cloneResult = Exec.run("C:\\Program Files\\Git\\cmd\\git.exe", cloneArguments, currentDirectory: temporaryURL.path)

-            return inputs.allSatisfy({ !$0.contains(".build\\checkouts\\") }) && !nodeModuleName.hasSuffix("Tests")
+#if os(Windows)
+            let checkoutsPath = #".build\checkouts\"#
+#else
+            let checkoutsPath = ".build/checkouts/"
+#endif
+            return inputs.allSatisfy({ !$0.contains(checkoutsPath) }) && !nodeModuleName.hasSuffix("Tests")

Windows Support #769

Are you sure you want to change the base?

Windows Support #769

Conversation

compnerd commented May 11, 2023 • edited Loading

compnerd commented May 22, 2023

jpsim commented Jun 13, 2023

compnerd commented Jun 13, 2023

jpsim commented Jun 14, 2023

Choose a reason for hiding this comment

compnerd commented Jun 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented May 11, 2023 •

edited

Loading