Swift SDKs: fix `toolset.linker.path` not passed to `-ld-path` #6719

kabiroberai · 2023-07-18T15:05:47Z

This PR implements support for the linker.path field in the Swift SDK toolset spec.

Depends on apple/swift#68495 and apple/swift-driver#1441.

Motivation:

This field was previously parsed but not respected.

Modifications:

Add -ld-path=\(linker.path) to the toolchain's Swift flags if a linker path override is supplied

Result:

The linker path is now respected.

kabiroberai · 2023-07-18T15:11:51Z

Sources/PackageModel/UserToolchain.swift

+        var swiftCompilerFlags = destination.toolset.knownTools[.swiftCompiler]?.extraCLIOptions ?? []
+
+        if let linker = destination.toolset.knownTools[.linker]?.path {
+            swiftCompilerFlags += ["-use-ld=\(linker.pathString)"]


cc @compnerd could this interfere with the use-ld check that's used to infer the librarian on Windows? Specifically, this uses an absolute path rather than just the name — so maybe determineLibrarian should just look at the last path component?

Does -use-ld= support absolute paths? If so, yes, this would need to have an associated change to the testing for the librarian check for Windows. Additionally, I think that you should be using linker._nativePathString(escaped: false) here ... to ensure that we actually test with the proper path on windows, e.g. -use-ld=C:\Program Files\Swift\Toolchain\0.0.0+Asserts\usr\bin\lld-link.exe. Note the space in the path and the use of \.

If #6730 is merged _nativePathString(escaped: false) will be applied automatically in that string interpolation.

Does -use-ld= support absolute paths?

It does on macOS. As we assume we're always passing this to Clang, wouldn't Clang on Windows maintain this support for consistency?

Hmm it looks like passing a path to -fuse-ld is supported but not recommended: https://github.com/apple/llvm-project/blob/1604b03867ff8c0ece77a61d16e3f609e3cfcb3c/clang/lib/Driver/ToolChain.cpp#L820

The correct flag expected by the clang driver seems to be --ld-path. Will switch to that instead.

This raises another question though: should we attempt to derive the linker flavor based on linker.path? Or should we just pass --ld-path and allow SDK vendors to specify the flavor via extraCLIArguments if they wish?

MaxDesiatov · 2023-07-18T15:28:27Z

@swift-ci smoke test

MaxDesiatov · 2023-07-18T16:50:08Z

@swift-ci smoke test

MaxDesiatov · 2023-07-18T17:36:32Z

@swift-ci test windows

MaxDesiatov · 2023-07-18T20:16:18Z

I need to test these changes with some Swift SDKs I have generated locally, and then assuming that goes well it can be merged.

MaxDesiatov

Just ran end-to-end tests on my side with this change, working well. Good stuff, thanks for cleaning it up!

compnerd

Please account for the absolute path in the librarian checks in Windows.

kabiroberai · 2023-07-19T15:01:05Z

@compnerd what would the appropriate API for extracting the last path component from an absolute-or-relative path be? I'm assuming we don't want to use Foundation.URL here

MaxDesiatov · 2023-07-19T15:04:42Z

what would the appropriate API for extracting the last path component from an absolute-or-relative path be? I'm assuming we don't want to use Foundation.URL here

Note that a deserialized Toolset already uses absolute paths, so there's no need to account for relative ones. AbsolutePath.basename seems like a good fit. If that's not the case on Windows that feels like a bug to me that we should fix in a separate PR.

Although I then have another question, how exactly would you use this last path component? If Clang on Windows supports absolute paths (with spaces and all, quoted if needed), then wouldn't we prefer to always use absolute paths here and not deal with relative ones at all?

kabiroberai · 2023-07-19T15:22:13Z

Note that a deserialized Toolset already uses absolute paths, so there's no need to account for relative ones.

my bad, I think my comment was a bit vague: we should be fine on L334 because we already have an AbsolutePath (I'll switch to _nativePathString there). My concern is L171, where there's a (somewhat unrelated) check for -use-ld= on Windows. Since we're now adding -use-ld with an absolute path to the compiler flags, that check no longer works consistently because it assumes the value is the linker name rather than the path. In fact the L171 check would've always had this issue (assuming clang on Windows accepts file paths for -fuse-ld) but it's just more likely to manifest now. So to fix this we need to parse the -use-ld= argument as a path and get the last component, I'm just not sure how to parse the path since it can either be absolute (C:\path\to\ld) or relative (ld).

MaxDesiatov · 2023-07-19T17:08:11Z

It doesn't look like deriveSwiftCFlags calls determineLibrarian in its codepaths, so L171 check seems unrelated. Please make sure that's the case, and if so we just need to verify that -use-ld= works with absolute paths on Windows and that its argument is correctly quoted so that spaces in paths don't break anything.

kabiroberai · 2023-07-19T18:59:19Z

It doesn't look like deriveSwiftCFlags calls determineLibrarian in its codepaths

The concern is the other way around: determineLibrarian takes extraSwiftFlags which are based on the output of deriveSwiftCFlags. So if deriveSwiftCFlags returned ["-use-ld=C:\path\to\lld"] then determineLibrarian wouldn't work as expected because the linker == "lld" check would fail. Something like (linker as NSString).lastPathComponent == "lld" would fix it but I don't know if that's portable.

MaxDesiatov · 2023-07-19T19:04:43Z

The concern is the other way around: determineLibrarian takes extraSwiftFlags which are based on the output of deriveSwiftCFlags. So if deriveSwiftCFlags returned ["-use-ld=C:\path\to\lld"] then determineLibrarian wouldn't work as expected because the linker == "lld" check would fail. Something like (linker as NSString).lastPathComponent == "lld" would fix it but I don't know if that's portable.

It's likely to be "-use-ld=C:\path\to\lld.exe" having .exe extension in the real world.

@compnerd would using AbsoluteString.basenameWithoutExt be acceptable workaround or do we need a different approach here to make sure this code is cross-platform?

Sources/PackageModel/UserToolchain.swift

kabiroberai · 2023-07-19T19:53:01Z

Switching to --ld-path should have resolved the issues with the Windows -use-ld check — I think this PR is in a good state now

This PR introduces an `--ld-path` option, which is forwarded to the Clang linker-driver option with the same name. This change is required to support the `toolset.linker.path` option in [SE-0387](https://github.com/apple/swift-evolution/blob/main/proposals/0387-cross-compilation-destinations.md). ## See also Upstream: apple/swift#67956. Downstream: apple/swift-package-manager#6719.

This PR defines an `--ld-path` option for the Swift Driver, which is forwarded to the Clang linker-driver option with the same name. This change is required to support the `toolset.linker.path` option in [SE-0387](https://github.com/apple/swift-evolution/blob/main/proposals/0387-cross-compilation-destinations.md). ## See Also Downstream: apple/swift-driver#1416, apple/swift-package-manager#6719.

MaxDesiatov · 2023-09-05T17:38:08Z

@swift-ci smoke test

MaxDesiatov · 2023-09-05T17:38:13Z

@swift-ci test windows

MaxDesiatov

Thanks, sorry about the delay. This LGTM!

MaxDesiatov · 2023-09-06T16:23:14Z

@swift-ci test windows

kabiroberai · 2023-09-07T08:58:07Z

@compnerd mind taking a look at this again please? I think I've addressed the changes that you requested — it should be possible to merge this PR once you approve.

compnerd · 2023-09-13T16:35:30Z

Sources/PackageModel/UserToolchain.swift

+        var swiftCompilerFlags = swiftSDK.toolset.knownTools[.swiftCompiler]?.extraCLIOptions ?? []
+
+        if let linker = swiftSDK.toolset.knownTools[.linker]?.path {
+            swiftCompilerFlags += ["--ld-path=\(linker)"]


Why not use -use-ld=\(linker) here? I'd really rather not introduce new spellings for existing functionality.

@kabiroberai I had a chat about this offline and we landed on -ld-path, I've submitted follow up PRs for that:

apple/swift#68495
apple/swift-driver#1441

There's a still a question what happens when both options are passed as -use-ld=silly-linker -ld-path=/usr/bin/funny-linker. We also need a test to verify that, whatever behavior is when both of those options are passed, it doesn't regress.

thanks for handling the upstream side of things! I believe -use-ld and -ld-path are (mostly) orthogonal but I see how that could regress; that test would be best suited to swift-driver right?

Yes, tests for this are more appropriate in Swift Driver I believe. cc @artemcm

Upstream dependencies: apple/swift#68495 apple/swift-driver#1441

MaxDesiatov · 2023-09-15T10:03:08Z

@swift-ci test

MaxDesiatov · 2023-09-15T14:09:26Z

@swift-ci test windows

compnerd

I think that we should really follow up on this to determine the best way forward for the flags. It is unclear how -ld-path= and -use-ld= compose and feels like this is something we should be paying attention to. We should aim to avoid the mistakes of the gcc driver.

neonichu · 2023-09-20T22:26:37Z

Looks like this broke existing Swift SDKs:

ld.lld: error: unable to find library -ld-path=/Users/neonacho/Library/org.swift.swiftpm/swift-sdks/5.8-RELEASE_rhel_ubi9_aarch64.artifactbundle/5.8-RELEASE_rhel_ubi9_aarch64/aarch64-unknown-linux-gnu/swift.xctoolchain/usr/bin/ld.lld

@tomerd suggested to revert, I'll open a PR for that.

#6719)" This reverts commit d328002. This broke existing Swift SDKs: ``` ld.lld: error: unable to find library -ld-path=/Users/neonacho/Library/org.swift.swiftpm/swift-sdks/5.8-RELEASE_rhel_ubi9_aarch64.artifactbundle/5.8-RELEASE_rhel_ubi9_aarch64/aarch64-unknown-linux-gnu/swift.xctoolchain/usr/bin/ld.lld ```

#6719)" (#6939) This reverts commit d328002. This broke existing Swift SDKs: ``` ld.lld: error: unable to find library -ld-path=/Users/neonacho/Library/org.swift.swiftpm/swift-sdks/5.8-RELEASE_rhel_ubi9_aarch64.artifactbundle/5.8-RELEASE_rhel_ubi9_aarch64/aarch64-unknown-linux-gnu/swift.xctoolchain/usr/bin/ld.lld ```

…-ld-path` (apple#6719)" (apple#6939)" This reverts commit 57d0a55.

This PR implements support for the `linker.path` field in the Swift SDK toolset spec. Depends on apple/swift#68495 and apple/swift-driver#1441. ### Motivation: This field was previously parsed but not respected. ### Modifications: Add `-ld-path=\(linker.path)` to the toolchain's Swift flags if a linker path override is supplied ### Result: The linker path is now respected.

#6719)" (#6939) This reverts commit d328002. This broke existing Swift SDKs: ``` ld.lld: error: unable to find library -ld-path=/Users/neonacho/Library/org.swift.swiftpm/swift-sdks/5.8-RELEASE_rhel_ubi9_aarch64.artifactbundle/5.8-RELEASE_rhel_ubi9_aarch64/aarch64-unknown-linux-gnu/swift.xctoolchain/usr/bin/ld.lld ```

This PR implements support for the `linker.path` field in the Swift SDK toolset spec. Depends on apple/swift#68495 and apple/swift-driver#1441. ### Motivation: This field was previously parsed but not respected. ### Modifications: Add `-ld-path=\(linker.path)` to the toolchain's Swift flags if a linker path override is supplied ### Result: The linker path is now respected.

#6719)" (#6939) This reverts commit d328002. This broke existing Swift SDKs: ``` ld.lld: error: unable to find library -ld-path=/Users/neonacho/Library/org.swift.swiftpm/swift-sdks/5.8-RELEASE_rhel_ubi9_aarch64.artifactbundle/5.8-RELEASE_rhel_ubi9_aarch64/aarch64-unknown-linux-gnu/swift.xctoolchain/usr/bin/ld.lld ```

kabiroberai requested review from abertelrud, neonichu, tomerd and MaxDesiatov as code owners July 18, 2023 15:05

kabiroberai commented Jul 18, 2023

View reviewed changes

kabiroberai changed the title ~~Swift SDKs: Implement toolset.linker.path~~ Swift SDKs: implement toolset.linker.path Jul 18, 2023

PackageModel: Respect toolset linker

8da811e

kabiroberai force-pushed the use-ld branch from 2dd4716 to 8da811e Compare July 18, 2023 15:56

neonichu approved these changes Jul 18, 2023

View reviewed changes

MaxDesiatov requested a review from rauhul July 18, 2023 16:49

MaxDesiatov self-assigned this Jul 18, 2023

MaxDesiatov added the cross-compilation label Jul 18, 2023

MaxDesiatov changed the title ~~Swift SDKs: implement toolset.linker.path~~ Swift SDKs: fix toolset.linker.path not passed to -use-ld Jul 18, 2023

MaxDesiatov approved these changes Jul 19, 2023

View reviewed changes

MaxDesiatov requested a review from compnerd July 19, 2023 13:51

compnerd requested changes Jul 19, 2023

View reviewed changes

Use linker._nativePathString

7c6b40f

MaxDesiatov reviewed Jul 19, 2023

View reviewed changes

Sources/PackageModel/UserToolchain.swift Outdated Show resolved Hide resolved

Feedback

9ca669c

kabiroberai mentioned this pull request Aug 16, 2023

Add ld-path driver option apple/swift#67956

Merged

Use --ld-path from Swift Driver

5b38348

neonichu approved these changes Aug 16, 2023

View reviewed changes

MaxDesiatov approved these changes Sep 5, 2023

View reviewed changes

MaxDesiatov enabled auto-merge (squash) September 5, 2023 17:48

compnerd reviewed Sep 13, 2023

View reviewed changes

Update -ld-path flag

a4aebb1

Upstream dependencies: apple/swift#68495 apple/swift-driver#1441

auto-merge was automatically disabled September 14, 2023 11:53
Head branch was pushed to by a user without write access

kabiroberai requested a review from compnerd September 19, 2023 19:35

MaxDesiatov changed the title ~~Swift SDKs: fix toolset.linker.path not passed to --ld-path~~ Swift SDKs: fix toolset.linker.path not passed to -ld-path Sep 19, 2023

MaxDesiatov enabled auto-merge (squash) September 19, 2023 21:00

compnerd approved these changes Sep 19, 2023

View reviewed changes

MaxDesiatov merged commit d328002 into apple:main Sep 19, 2023
5 checks passed

kabiroberai added a commit to kabiroberai/swift-package-manager that referenced this pull request Sep 21, 2023

Revert "Revert "Swift SDKs: fix toolset.linker.path not passed to `…

19e7d2d

…-ld-path` (apple#6719)" (apple#6939)" This reverts commit 57d0a55.

MaxDesiatov mentioned this pull request Jan 3, 2024

Fail to build (link) executable using generated swift-sdk #7222

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift SDKs: fix `toolset.linker.path` not passed to `-ld-path` #6719

Swift SDKs: fix `toolset.linker.path` not passed to `-ld-path` #6719

kabiroberai commented Jul 18, 2023 •

edited by MaxDesiatov

kabiroberai Jul 18, 2023 •

edited

compnerd Jul 19, 2023

MaxDesiatov Jul 19, 2023

MaxDesiatov Jul 19, 2023 •

edited

kabiroberai Jul 19, 2023

kabiroberai Jul 19, 2023

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023 •

edited

MaxDesiatov left a comment

compnerd left a comment

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Jul 19, 2023 •

edited

kabiroberai commented Jul 19, 2023 •

edited

MaxDesiatov commented Jul 19, 2023

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Jul 19, 2023 •

edited

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Sep 5, 2023

MaxDesiatov commented Sep 5, 2023

MaxDesiatov left a comment

MaxDesiatov commented Sep 6, 2023

kabiroberai commented Sep 7, 2023

compnerd Sep 13, 2023

MaxDesiatov Sep 13, 2023 •

edited

kabiroberai Sep 14, 2023

MaxDesiatov Sep 14, 2023 •

edited

MaxDesiatov commented Sep 15, 2023

MaxDesiatov commented Sep 15, 2023

compnerd left a comment

neonichu commented Sep 20, 2023

Swift SDKs: fix toolset.linker.path not passed to -ld-path #6719

Swift SDKs: fix toolset.linker.path not passed to -ld-path #6719

Conversation

kabiroberai commented Jul 18, 2023 • edited by MaxDesiatov

Motivation:

Modifications:

Result:

kabiroberai Jul 18, 2023 • edited

Choose a reason for hiding this comment

compnerd Jul 19, 2023

Choose a reason for hiding this comment

MaxDesiatov Jul 19, 2023

Choose a reason for hiding this comment

MaxDesiatov Jul 19, 2023 • edited

Choose a reason for hiding this comment

kabiroberai Jul 19, 2023

Choose a reason for hiding this comment

kabiroberai Jul 19, 2023

Choose a reason for hiding this comment

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023

MaxDesiatov commented Jul 18, 2023 • edited

MaxDesiatov left a comment

Choose a reason for hiding this comment

compnerd left a comment

Choose a reason for hiding this comment

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Jul 19, 2023 • edited

kabiroberai commented Jul 19, 2023 • edited

MaxDesiatov commented Jul 19, 2023

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Jul 19, 2023 • edited

kabiroberai commented Jul 19, 2023

MaxDesiatov commented Sep 5, 2023

MaxDesiatov commented Sep 5, 2023

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Sep 6, 2023

kabiroberai commented Sep 7, 2023

compnerd Sep 13, 2023

Choose a reason for hiding this comment

MaxDesiatov Sep 13, 2023 • edited

Choose a reason for hiding this comment

kabiroberai Sep 14, 2023

Choose a reason for hiding this comment

MaxDesiatov Sep 14, 2023 • edited

Choose a reason for hiding this comment

MaxDesiatov commented Sep 15, 2023

MaxDesiatov commented Sep 15, 2023

compnerd left a comment

Choose a reason for hiding this comment

neonichu commented Sep 20, 2023

Swift SDKs: fix `toolset.linker.path` not passed to `-ld-path` #6719

Swift SDKs: fix `toolset.linker.path` not passed to `-ld-path` #6719

kabiroberai commented Jul 18, 2023 •

edited by MaxDesiatov

kabiroberai Jul 18, 2023 •

edited

MaxDesiatov Jul 19, 2023 •

edited

MaxDesiatov commented Jul 18, 2023 •

edited

MaxDesiatov commented Jul 19, 2023 •

edited

kabiroberai commented Jul 19, 2023 •

edited

MaxDesiatov commented Jul 19, 2023 •

edited

MaxDesiatov Sep 13, 2023 •

edited

MaxDesiatov Sep 14, 2023 •

edited