Swift SDKs: implement `swiftResourcesPath` #6717

kabiroberai · 2023-07-18T13:58:13Z

This PR implements swiftResourcesPath and swiftStaticResourcesPath from the swift-sdk.json spec.

Motivation:

SwiftPM previously knew how to parse these fields from the SDK manifest, but didn't actually do anything with them.

Modifications:

Pass -resource-dir to swiftc for compile jobs
Pass -resource-dir to both swiftc and clang for link jobs

Result:

Toolchains that override swiftResourcesPath and/or swiftStaticResourcesPath should now behave as expected.

Sources/PackageModel/Toolchain.swift

Sources/PackageModel/UserToolchain.swift

MaxDesiatov

SGTM overall modulo missing doc comments.

Sources/PackageModel/Toolchain.swift

MaxDesiatov · 2023-07-18T14:52:40Z

@swift-ci smoke test

MaxDesiatov

Thanks!

BuildDescription: propagate resources to clang too Doc comments feedback

kabiroberai · 2023-07-18T16:01:05Z

@MaxDesiatov mind kicking off CI again? Had to rebase on top of 0e50b63.

Sources/Build/BuildDescription/ProductBuildDescription.swift

rauhul · 2023-07-18T16:04:11Z

Sources/Build/BuildDescription/ProductBuildDescription.swift

@@ -218,6 +219,7 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
                    self.observabilityScope.emit(.swiftBackDeployError)
                } else if self.buildParameters.targetTriple.isSupportingStaticStdlib {
                    args += ["-static-stdlib"]
+                    hasStaticStdlib = true


Shouldn't shouldLinkStaticSwiftStdlib when !isSupportingStaticStdlib be an error?

Perhaps refactor this to match the style of https://github.com/apple/swift-package-manager/pull/6717/files#diff-31b280ef997c175a0c39198547ce45d473e9fb89ac71aaa5ed2ad7e4a92473c8R934

It seems a bit harder to adopt that approach here because the inclusion of -static-stdlib in the linker invocation also depends on the product type. If I understand your point correctly, I agree that there should be better validation around shouldLinkStaticSwiftStdlib (e.g. emitting an error if the product is a dynamic library) but imo that would be better suited to a follow-up PR since it's orthogonal to this change — what do you think?

A follow-up PR is fine by me, but in that case would you mind adding a TODO or FIXME note in code comments? I hope this would be addressed sooner rather than later.

Sources/PackageModel/UserToolchain.swift

MaxDesiatov · 2023-07-18T16:42:42Z

@MaxDesiatov mind kicking off CI again? Had to rebase on top of 0e50b63.

We usually squash and merge here, so there's no need to rebase unless you really prefer that workflow. A lot of times a merge commit is easier to resolve conflicts with, and it won't show up after squash and merge in the target branch anyway.

kabiroberai · 2023-07-18T18:14:52Z

Gotcha, will keep that in mind — wasn't sure about the convention since it seems they do merge commits over on apple/swift.

kabiroberai · 2023-07-19T23:37:41Z

Sources/Build/BuildDescription/ProductBuildDescription.swift

@@ -243,6 +246,16 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
            throw InternalError("unexpectedly asked to generate linker arguments for a plugin product")
        }

+        if let resourcesPath = self.buildParameters.toolchain.swiftResourcesPath(isStatic: isLinkingStaticStdlib) {
+            args += ["-resource-dir", "\(resourcesPath)"]


@MaxDesiatov is it correct to rely on interpolation here instead of pathString?

~~should this just move into the if self.buildParameters.shouldLinkStaticSwiftStdlib branch? its used only there it seems~~ hmm read this wrong

Interpolation will render it properly on Windows: https://github.com/apple/swift-package-manager/blob/14ff299060cfd7f1bab3a4fc9065f77d427f8093/Sources/Basics/FileSystem/NativePathExtensions.swift#L16

MaxDesiatov · 2023-09-05T18:06:16Z

@swift-ci smoke test

MaxDesiatov · 2023-09-05T18:06:22Z

@swift-ci test windows

MaxDesiatov · 2023-09-06T16:21:57Z

@swift-ci test

compnerd · 2023-09-12T14:18:34Z

This seems to have broken SPM on Windows, I'm going to revert this for now, please make sure to run the tests on Windows locally in the future.

This reverts commit 76e3594.

tomerd · 2023-09-12T18:11:39Z

please make sure to run the tests on Windows locally in the future.

isn't this what CI is for? can we get the tests running on CI?

MaxDesiatov · 2023-09-13T14:27:02Z

This seems to have broken SPM on Windows, I'm going to revert this for now, please make sure to run the tests on Windows locally in the future.

@compnerd Would you be able to provide the exact error message? This would help us fix the issue with no readily available local access to Windows and no testing set up on CI either. +1 to Tom's question why wasn't this caught on CI in the first place?

compnerd · 2023-09-13T16:24:32Z

@MaxDesiatov I think that it was actually the change after this (XCTest integration), at least after a bit more digging. This wasn't caught on CI due to the test suite not running in CI, only building SPM.

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

MaxDesiatov requested a review from rauhul July 18, 2023 13:58

MaxDesiatov reviewed Jul 18, 2023

View reviewed changes

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

MaxDesiatov reviewed Jul 18, 2023

View reviewed changes

Sources/PackageModel/UserToolchain.swift Show resolved Hide resolved

MaxDesiatov reviewed Jul 18, 2023

View reviewed changes

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

MaxDesiatov changed the title ~~Swift SDKs: implement swiftResourcesPath~~ Swift SDKs: implement swiftResourcesPath Jul 18, 2023

MaxDesiatov self-assigned this Jul 18, 2023

MaxDesiatov added the cross-compilation label Jul 18, 2023

MaxDesiatov approved these changes Jul 18, 2023

View reviewed changes

BuildDescription: propagate swiftResourcesPath

1561e81

BuildDescription: propagate resources to clang too Doc comments feedback

kabiroberai force-pushed the resource-dir-overrides branch from af85360 to 1561e81 Compare July 18, 2023 16:00

rauhul reviewed Jul 18, 2023

View reviewed changes

naming

0f9c31a

MaxDesiatov requested a review from rauhul July 18, 2023 18:29

kabiroberai added 3 commits July 19, 2023 19:31

add todo

28cf429

isStatic -> isLinkingStaticStdlib

2855a19

minor tweaks

43350df

kabiroberai commented Jul 19, 2023

View reviewed changes

Merge branch 'main' into resource-dir-overrides

0bdad11

MaxDesiatov merged commit 76e3594 into swiftlang:main Sep 7, 2023

compnerd added a commit that referenced this pull request Sep 12, 2023

Revert "Swift SDKs: implement swiftResourcesPath (#6717)"

02684bf

This reverts commit 76e3594.

Swift SDKs: implement swiftResourcesPath #6717

Swift SDKs: implement swiftResourcesPath #6717

Uh oh!

Conversation

kabiroberai commented Jul 18, 2023

Motivation:

Modifications:

Result:

Uh oh!

Uh oh!

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxDesiatov commented Jul 18, 2023

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

kabiroberai commented Jul 18, 2023

Uh oh!

Uh oh!

rauhul Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

rauhul Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

kabiroberai Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxDesiatov commented Jul 18, 2023

Uh oh!

kabiroberai commented Jul 18, 2023

Uh oh!

kabiroberai Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

tomerd Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Sep 5, 2023

Uh oh!

MaxDesiatov commented Sep 5, 2023

Uh oh!

MaxDesiatov commented Sep 6, 2023

Uh oh!

compnerd commented Sep 12, 2023

Uh oh!

tomerd commented Sep 12, 2023

Uh oh!

MaxDesiatov commented Sep 13, 2023

Uh oh!

compnerd commented Sep 13, 2023

Uh oh!

Uh oh!

Swift SDKs: implement `swiftResourcesPath` #6717

Swift SDKs: implement `swiftResourcesPath` #6717

tomerd Jul 20, 2023 •

edited

Loading