Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Windows linker discovery #163

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kcieplak
Copy link

  • Enable Windows linker discovery to get linker type and version
  • Make Platform Registry initialization async
  • Change the function signature of additionalPlatformExecutableSearchPaths to allow passing of a filesystem for discovery. Plugins will need to be updated to match new signatures
  • Add the visual studio install directory to platform search paths for executables.
  • Add Windows platform plugin extension to get install directory
  • Add new internal build setting _LINKER_EXEC for testing purposes.
  • Change the search paths of a Platform to a StackedSearchPath, as done with the Toolchain Registry
  • Add test for discovery of windows linker

@kcieplak
Copy link
Author

@swift-ci test

1 similar comment
@owenv
Copy link
Collaborator

owenv commented Feb 14, 2025

@swift-ci test

@@ -1232,11 +1231,18 @@ public final class LdLinkerSpec : GenericLinkerSpec, SpecIdentifierType, @unchec

override public func discoveredCommandLineToolSpecInfo(_ producer: any CommandProducer, _ scope: MacroEvaluationScope, _ delegate: any CoreClientTargetDiagnosticProducingDelegate) async -> (any DiscoveredCommandLineToolSpecInfo)? {
let alternateLinker = scope.evaluate(BuiltinMacros.ALTERNATE_LINKER)
var linker = if alternateLinker != "" { Path(alternateLinker) } else { producer.hostOperatingSystem == .windows ? Path("link.exe") : Path("ld") }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be ideal to use the linker driver (clang) to find the linker. That allows the customization point for the default linker (CLANG_DEFAULT_LINKER).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean use clang as the linker? Or something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, using clang as the linker driver (it knows what to do to locate and construct the invocation).

Copy link
Author

@kcieplak kcieplak Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang is used to invoke the linker. The discovery of the linker via discoveredCommandLineToolSpecInfo attempts to find out details about the linker that clang would invoke. Internally depending on the linker type or the version discovered different linker options can be provided (via -Xlinker)

The ALTERNATE_LINKER build setting sets the -fuse-ld option forcing clang to use a different named linker. I will verify if on Windows clang will use link.exe by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that indeed clang on windows by default will use link.exe.

@@ -1540,7 +1546,7 @@ public final class LibtoolLinkerSpec : GenericLinkerSpec, SpecIdentifierType, @u
public func discoveredLinkerToolsInfo(_ producer: any CommandProducer, _ delegate: any CoreClientTargetDiagnosticProducingDelegate, at toolPath: Path) async -> (any DiscoveredCommandLineToolSpecInfo)? {
do {
do {
let commandLine = [toolPath.str, "-version_details"]
let commandLine = [toolPath.str, producer.hostOperatingSystem == .windows ? "/VERSION" : "-version_details"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do ld, gold, mold, wild support -version_details? I thought that they use -version.

Copy link
Author

@kcieplak kcieplak Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct they do not appear to support -version_details. The command will not produce valid json and will not match any of the patterns then throw CommandLineOutputJSONParsingError, falling into the catch, where a secondary call with "-version" is run.

Note: There is a bug as the regex pattern is looking for a space after the version which will not match (At least on windows)

package var ldPath: Path {
get async throws {
let (core, defaultToolchain) = try await coreAndToolchain()
let fallbacklibtool = Path("/usr/bin/ld")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path is a relative path on Windows. I think that we should conditionalise this on platform that we are building swift build for.

let toolName = core.hostOperatingSystem == .windows ? "link" : "ld"

// Look in the default toolchain.
if let executable = defaultToolchain.executableSearchPaths.findExecutable(operatingSystem: core.hostOperatingSystem, basename: toolName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Windows, I think that we also want to consider lld-link.

Copy link
Author

@kcieplak kcieplak Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the current swift package manager on windows uses lld-link. I will change the default to that for windows platforms.(in a different PR)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compnerd lld-link is an independent implementation which is compatible with link.exe's command line, but doesn't use link.exe -- right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakepetroules right - it is compatible with link.exe for invocation, but is a complete implementation. At this point, on main, the toolchain should be relatively complete. We have the last piece enabled now which was llvm-mt which replaces mt.

For users of Swift, the only thing that they should be depending on from Microsoft would be the headers and import libraries. We have equivalent tools to allow them to hopefully perform all necessary operations.

* Enable Windows linker discovery to get linker type and version
* Make Platform Registry initalization async
* Change the function signature of additionalPlatformExecutableSearchPaths to allow passing
  of a filesystem for discovery.  Plugins will need to be updated to match
  new signatures
* Add the visual studio install directory to platform search
  paths for executables.
  * Add Windows platform plugin extention to get install directory
* Add new internal build setting _LINKER_EXEC for testing purposes.
* Change the search paths of a Platform to a StackedSearchPath, as
  done with the Toolchain Registry
* Add test for discovery of windows linker
@kcieplak kcieplak force-pushed the topics/windows-linker-discovery branch from e99f8d0 to fc25b1e Compare February 18, 2025 14:41
@kcieplak
Copy link
Author

@swift-ci test

  with no arguments, will output the version string.
* Use proper path seperator based on running host for
  environment variable seperator.
* Check host arch to determine which subpath to check
  for visual studio installed binaries
@kcieplak kcieplak force-pushed the topics/windows-linker-discovery branch from fc25b1e to d2f386b Compare February 18, 2025 16:23
@kcieplak
Copy link
Author

@swift-ci test

1 similar comment
@daveinglis
Copy link
Contributor

@swift-ci test

@kcieplak
Copy link
Author

@swift-ci test

1 similar comment
@kcieplak
Copy link
Author

@swift-ci test

@shahmishal
Copy link
Member

@swift-ci test

@kcieplak
Copy link
Author

Going to rebase this ontop of #207, as it fixes the missing link.exe discovery.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants