Skip to content

Conversation

@bnbarham
Copy link
Contributor

@bnbarham bnbarham commented Jun 5, 2024

Prefer the toolchain provided in the build plan rather than the default host toolchain so that the returned arguments align with the toolchain used to construct the build plan.

return PluginTargetBuildDescription(
target: target,
toolsVersion: package.manifest.toolsVersion,
toolchain: buildPlan.toolsBuildParameters.toolchain as? UserToolchain,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I very much dislike this, but ManifestLoader has a bunch of uses of members in UserToolchain that are not in Toolchain. Should we add requirements for them all?

Copy link
Contributor

Choose a reason for hiding this comment

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

If they make sense for Toolchain protocol, why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any other implementations of Toolchain other than UserToolchain and MockToolchain? If not, maybe we should reconsider our approach to mocking that doesn't require maintaining a separate protocol?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a FIXME right on top of UserToolchain, might be the time to make it right...

let isPartOfRootPackage: Bool

init(target: ResolvedModule, toolsVersion: ToolsVersion, isPartOfRootPackage: Bool) {
init(target: ResolvedModule, toolsVersion: ToolsVersion, toolchain: UserToolchain?, isPartOfRootPackage: Bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we pass BuildParameters here which should allow us make toolchain field non-Optional, we can assign it this way in the initializer:

self.toolchain = if let toolchain = buildParameters.toolchain as? UserToolchain {
  toolchain
} else {
  UserToolchain(swiftSDK: .hostSwiftSDK()))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up adding a static method to get the arguments and then adding swiftPMLibrariesLocation to Toolchain, which then gets rid of this optional problem entirely.

Prefer the toolchain provided in the build plan rather than the default
host toolchain so that the returned arguments align with the toolchain
used to construct the build plan.
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit 41405ac into swiftlang:main Jul 17, 2024
@bnbarham bnbarham deleted the plugin-toolchain branch July 17, 2024 01:09
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.

3 participants