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

rdar://109108066 (Derive '-external-plugin-path' compiler arguments in SwiftPM) #6560

Merged
merged 1 commit into from May 19, 2023

Conversation

neonichu
Copy link
Member

This derives the required -external-plugin-path arguments when using an OSS toolchain with SwiftPM.

@neonichu neonichu requested a review from abertelrud as a code owner May 16, 2023 00:01
@neonichu neonichu self-assigned this May 16, 2023
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

Not really sure how to test this, but I manually checked that all Swift targets get the relevant arguments.

@neonichu
Copy link
Member Author

Fun, looks like NSDictionary is different on macOS vs Linux?

/home/build-user/swiftpm/Sources/PackageModel/UserToolchain.swift:459:167: error: extra argument 'error' in call
        if localFileSystem.exists(toolchainPlistPath), let toolchainPlist = try? NSDictionary(contentsOf: URL(fileURLWithPath: toolchainPlistPath.pathString), error: ()), let overrideBuildSettings = toolchainPlist["OverrideBuildSettings"] as? NSDictionary, let isSwiftDevelopmentToolchainStringValue = overrideBuildSettings["SWIFT_DEVELOPMENT_TOOLCHAIN"] as? String {

I guess since this is supposed to be macOS only, we can conditionally compile.

@neonichu neonichu force-pushed the swift-plugin-server-support branch from 3e633ef to 895d520 Compare May 16, 2023 00:19
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu neonichu force-pushed the swift-plugin-server-support branch from 895d520 to 02f8393 Compare May 17, 2023 21:01
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

1 similar comment
@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@@ -392,6 +392,18 @@ public final class SwiftTargetBuildDescription {
}
#endif

// If we're using an OSS toolchain, add the required arguments bringing in the plugin server from the default toolchain if available.
if self.buildParameters.toolchain.isSwiftDevelopmentToolchain, driverSupport.checkSupportedFrontendFlags(flags: ["-external-plugin-path"], toolchain: self.buildParameters.toolchain, fileSystem: self.fileSystem), let pluginServer = self.buildParameters.toolchain.swiftPluginServerPath {
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR but we should change checkSupportedFrontendFlag to take an enum instead of a string so we can centralize the usage of the string expression

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean for the flags we're checking? Seems a bit odd to do that specifically for checkSupportedFrontendFlag when we're using literal strings all over the Build module. I could see a broader change that uses a typed way for this everywhere, but IMO should be coming from the swift-driver library.

Copy link
Member

Choose a reason for hiding this comment

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

yea, the problem is that all these strings everywhere can get out of sync and are focused on actual CLI invocation instead of on the capabilities. I think the call sites would be better if they look more like the following:

if driverSupport.supports(.externalPlugin) {
    ...
    args += driverSupport.flag(for: .externalPlugin, with: value)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I agree with that.

Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

some non-blocking suggestions

…n SwiftPM)

This derives the required `-external-plugin-path` arguments when using an OSS toolchain with SwiftPM.
@neonichu neonichu force-pushed the swift-plugin-server-support branch from 02f8393 to 5f786fe Compare May 18, 2023 22:03
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

2 similar comments
@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu
Copy link
Member Author

No Windows, I guess ¯\_(ツ)_/¯

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu neonichu merged commit 72d5b34 into main May 19, 2023
5 checks passed
@neonichu neonichu deleted the swift-plugin-server-support branch May 19, 2023 17:02
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.

None yet

2 participants