SwiftDriver: initial (incomplete) windows port #878

compnerd · 2021-10-15T15:56:30Z

This is sufficient to build on Windows, and the resulting driver is able
to somewhat function. The tests are still not usable due to assumptions
about path which do not hold and the insufficiency of the path
representation in tools-support-core. This is likely best approached by
replacing the use of Path from t-s-c with the representation in
system. However, this allows some attempt to validate the swift-driver
which is increasingly becoming required for s-p-m, and thus unblocks
further work.

compnerd · 2021-10-15T15:59:42Z

@artemcm - the big problem with this change is that there are a few places where I cannot reliably make a distinction for the executable names (i.e. extensions - .exe). Do you have thoughts on how to handle this? I'm leaning towards adding a freestanding function:

func executable(_ name: String) -> String {
#if os(Windows)
  guard name.suffix(from: name.index(name.endIndex, offsetBy: -4))) == ".exe" else {
    return "\(name).exe"
  }
  return name
#else
  return name
#endif
}

compnerd · 2021-10-15T16:02:44Z

I'm aware that there was a previous port that attempted to add support for Windows, however, I feel that this is more complete and more closely reflects the C++ driver. This is sufficient for getting a fair number of the tests to pass. This is a pretty simplistic mapping for the tools, but it establishes a beachhead so that we can move forward.

compnerd · 2021-10-16T19:52:07Z

@swift-ci please test

compnerd · 2021-10-17T18:06:59Z

@swift-ci please test macOS platform

compnerd · 2021-10-18T20:43:45Z

@swift-ci please test

compnerd · 2021-10-18T20:58:14Z

Locally, Im still seeing ~30% of the tests fail, which I've not yet dug into, but once this is merged, we should have a baseline to work from.

artemcm · 2021-10-18T22:04:53Z

@artemcm - the big problem with this change is that there are a few places where I cannot reliably make a distinction for the executable names (i.e. extensions - .exe). Do you have thoughts on how to handle this? I'm leaning towards adding a freestanding function:
func executable(_ name: String) -> String {
#if os(Windows)
  guard name.suffix(from: name.index(name.endIndex, offsetBy: -4))) == ".exe" else {
    return "\(name).exe"
  }
  return name
#else
  return name
#endif
}

I think this is an entirely reasonable approach.

compnerd · 2021-10-18T22:27:41Z

@artemcm - the big problem with this change is that there are a few places where I cannot reliably make a distinction for the executable names (i.e. extensions - .exe). Do you have thoughts on how to handle this? I'm leaning towards adding a freestanding function:
func executable(_ name: String) -> String {
#if os(Windows)
  guard name.suffix(from: name.index(name.endIndex, offsetBy: -4))) == ".exe" else {
    return "\(name).exe"
  }
  return name
#else
  return name
#endif
}
I think this is an entirely reasonable approach.

Okay; just wanted to make sure that someone doesn't have a better idea (I can propose and implement and still dislike my idea right? :-D). There is some duplication - I'd like to move the function to a different file if I could get a bit of guidance as to where such a utility function should reside.

cltnschlosser

For the most part looks good, obviously lots of TODOs/FIXMEs for the future.

BTW is there a good "Beginners guide to Swift on Windows"?

Sources/SwiftDriver/Toolchains/Toolchain.swift

cltnschlosser · 2021-10-18T23:17:10Z

Sources/SwiftDriver/Toolchains/Toolchain.swift

@@ -131,6 +142,9 @@ extension Toolchain {
  private func envVarName(for toolName: String) -> String {
    let lookupName = toolName
      .replacingOccurrences(of: "-", with: "_")
+      // FIXME(compnerd) we should extract the extension for generating the
+      // toolname rather than assuming that we can convert the tool name blindly
+      .replacingOccurrences(of: ".", with: "_")


Yeah, I guess it would be nice if the environment variables were consistent across platforms. So windows doesn't always have an extra _EXE in its variables.

Yeah, I think that we will need to resolve this, but I think that doing that subsequently is better than trying to address everything immediately.

Sources/SwiftDriver/Driver/Driver.swift

cltnschlosser · 2021-10-18T23:19:51Z

Sources/SwiftDriver/Toolchains/Toolchain.swift

@@ -104,7 +115,7 @@ public protocol Toolchain {

 extension Toolchain {
  public var searchPaths: [AbsolutePath] {
-    getEnvSearchPaths(pathString: env["PATH"], currentWorkingDirectory: fileSystem.currentWorkingDirectory)
+    getEnvSearchPaths(pathString: ProcessEnv.path, currentWorkingDirectory: fileSystem.currentWorkingDirectory)


Does this break tests or something? Where an (arbitrary) env might have been injected in.

Yeah, it was breaking both tests as well as use of the driver.

Oh I meant the change. Since it isn’t using the passed in env anymore. But if it’s not breaking anything, then 🤷‍♂️

Oh, the tests and the driver seem to be behaving fine with the change. They should be equivalent in that ProcessEnv.path will map to env["PATH"] on Darwin and Linux, but env["Path"] on Windows.

It's not using this env, which is where I could see problems maybe arising, but if not, then roll with it :)

public protocol Toolchain { init(env: [String: String], executor: DriverExecutor, fileSystem: FileSystem, compilerExecutableDir: AbsolutePath?, toolDirectory: AbsolutePath?) var env: [String: String] { get }

I'm not convinced this won't potentially break downstream driver clients. Toolchain.env comes all the way from a Driver.init argument, which defaults to ProcessEnv.vars, but can also be invoked with a custom environment when the driver is used as a library.

Can we instead continue using env and disambiguate "Path" or "PATH" based on the platform like TSC does?

cltnschlosser · 2021-10-18T23:22:48Z

Sources/SwiftDriver/Toolchains/WindowsToolchain.swift

+      // FIXME(compnerd) we should capitalise the OS name, e.g. windows ->
+      // Windows; we get away with this for now as Windows has a
+      // case-insensitive file system.
+      let os: String = target.os?.rawValue ?? "none"


You can probably capitalize the rawValue of this enum for Windows.

Is "none" a better value than just returning nil here?

Ah, I think I see what you are proposing. I think that the returning nil actually could work and be better. I'll make that change. The capitalization though I don't know how to handle :-(.

I think this is the right enum: https://github.com/apple/swift-driver/blob/3af50728df679017de677743320d827402ee2c03/Sources/SwiftDriver/Utilities/Triple.swift#L1091

But is it windows or win32 currently?

It's both. The component is 'windows', the enum entry is 'win32'.

compnerd · 2021-10-18T23:35:07Z

For the most part looks good, obviously lots of TODOs/FIXMEs for the future.

Yeah, porting everything is going to take quite some time. This should at least put some of the paths in place.

BTW is there a good "Beginners guide to Swift on Windows"?

Hmm, from the driver perspective it should mostly be identical to macOS with the following differences:

you need VS and WinSDK installed (for MSVCRT and WinSDK headers and import libraries respectively)
you need to explicitly specify -sdk as there is no xcrun equivalent

For a user, at this point, its "install visual studio, install Swift using the installer from swift.org". Mostly everything else is now in place so that is all that should be needed. If there is something that needs documentation, I'm happy to try to write that up.

compnerd · 2021-10-19T17:30:31Z

@swift-ci please test

artemcm

I have just one concern about the Toolchain's use of environment variables, but other than that this is great!

Sources/SwiftDriver/Driver/WindowsExtensions.swift

artemcm · 2021-10-19T23:07:26Z

Sources/SwiftDriver/Toolchains/Toolchain.swift

@@ -104,7 +115,7 @@ public protocol Toolchain {

 extension Toolchain {
  public var searchPaths: [AbsolutePath] {
-    getEnvSearchPaths(pathString: env["PATH"], currentWorkingDirectory: fileSystem.currentWorkingDirectory)
+    getEnvSearchPaths(pathString: ProcessEnv.path, currentWorkingDirectory: fileSystem.currentWorkingDirectory)


I'm not convinced this won't potentially break downstream driver clients. Toolchain.env comes all the way from a Driver.init argument, which defaults to ProcessEnv.vars, but can also be invoked with a custom environment when the driver is used as a library.

Can we instead continue using env and disambiguate "Path" or "PATH" based on the platform like TSC does?

Sources/SwiftDriver/Toolchains/Toolchain.swift

compnerd · 2021-10-20T00:43:46Z

I think that we really should find something better for this - what happens if some OS uses something different? I think that changing this for now is fine, but for longer term, we really should be looking to abstract this using other libraries.

This is sufficient to build on Windows, and the resulting driver is able to somewhat function. The tests are still not usable due to assumptions about path which do not hold and the insufficiency of the path representation in tools-support-core. This is likely best approached by replacing the use of `Path` from t-s-c with the representation in system. However, this allows some attempt to validate the swift-driver which is increasingly becoming required for s-p-m, and thus unblocks further work.

compnerd · 2021-10-20T00:52:54Z

@swift-ci please test

compnerd force-pushed the windows branch from e2442a6 to 984359c Compare October 16, 2021 19:51

compnerd marked this pull request as ready for review October 16, 2021 19:51

compnerd force-pushed the windows branch from 984359c to 658c136 Compare October 17, 2021 18:06

compnerd force-pushed the windows branch from 658c136 to e794943 Compare October 18, 2021 20:43

compnerd requested review from artemcm and cltnschlosser and removed request for artemcm October 18, 2021 22:02

cltnschlosser reviewed Oct 18, 2021

View reviewed changes

compnerd force-pushed the windows branch from e794943 to 7128a34 Compare October 19, 2021 00:34

artemcm requested changes Oct 19, 2021

View reviewed changes

compnerd force-pushed the windows branch from 7128a34 to 633bd1f Compare October 20, 2021 00:52

artemcm approved these changes Oct 20, 2021

View reviewed changes

compnerd merged commit 6401fbb into swiftlang:main Oct 21, 2021

compnerd deleted the windows branch October 21, 2021 04:36

SwiftDriver: initial (incomplete) windows port #878

SwiftDriver: initial (incomplete) windows port #878

Uh oh!

Conversation

compnerd commented Oct 15, 2021

Uh oh!

compnerd commented Oct 15, 2021

Uh oh!

compnerd commented Oct 15, 2021

Uh oh!

compnerd commented Oct 16, 2021

Uh oh!

compnerd commented Oct 17, 2021

Uh oh!

compnerd commented Oct 18, 2021

Uh oh!

compnerd commented Oct 18, 2021

Uh oh!

artemcm commented Oct 18, 2021

Uh oh!

compnerd commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cltnschlosser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Oct 18, 2021

Uh oh!

compnerd commented Oct 19, 2021

Uh oh!

artemcm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

compnerd commented Oct 20, 2021

Uh oh!

compnerd commented Oct 20, 2021

Uh oh!

Uh oh!

compnerd commented Oct 18, 2021 •

edited

Loading