Introduce a notion of ConfiguredTargets into the build system
#1249
Conversation
…nstead of a public extension
Instead of asking for build settings of a file, the build system manager asks for the targets of a file and then asks for the build settings of that file in a specific target. This has two advantages: - We know about targets and can prepare the targets for background indexing - Once we support build systems in which a single file can be part of multiple targets, we can have a centralized place that picks preferred targets for a file, eg. based on user configuration
|
@swift-ci Please test |
|
swiftlang/swift-package-manager#7540 @swift-ci Please test |
| public struct ConfiguredTarget: Hashable, Sendable { | ||
| /// An opaque string that represents the target. | ||
| /// | ||
| /// The target's ID should be generated by the build system that handles the target and only interpreted by that | ||
| /// build system. | ||
| public let targetID: String | ||
|
|
||
| /// An opaque string that represents the run destination. | ||
| /// | ||
| /// The run destination's ID should be generated by the build system that handles the target and only interpreted by | ||
| /// that build system. | ||
| public let runDestinationID: String | ||
|
|
||
| public init(targetID: String, runDestinationID: String) { | ||
| self.targetID = targetID | ||
| self.runDestinationID = runDestinationID | ||
| } | ||
| } |
There was a problem hiding this comment.
I was going to suggest modeling this as associated type on BuildSystem, which would allow e.g the Package.swift and header cases to be better represented for SwiftPM, but it looks like that would make things more awkward for things like BuildSystemManager. So I guess this is fine 🤷
There was a problem hiding this comment.
Looks like we had similar thought. I tried that as well, had too many issues in BuildSystemManager and then decided to just go with strings.
| /// | ||
| /// The run destination's ID should be generated by the build system that handles the target and only interpreted by | ||
| /// that build system. | ||
| public let runDestinationID: String |
There was a problem hiding this comment.
Do we need a "configuration" as well, eg. debug/release/etc? Run destination is fairly general, so you could definitely include platform/sdk/architecture/etc in that, but I'm not sure configuration makes much sense there.
There was a problem hiding this comment.
Discussed offline that we don’t need it for now because the configuration is picked globally when launching sourcekit-lsp. If we do allow changing the configuration while sourcekit-lsp is running, we’ll need to re-consider whether to add configuration here.
| return [ConfiguredTarget(targetID: "", runDestinationID: "dummy")] | ||
| } | ||
|
|
||
| if url.pathExtension == "h", let target = try? target(forHeader: path) { |
There was a problem hiding this comment.
Could use defaultLanguage here too.
There was a problem hiding this comment.
Same here, just assume that a file is a header if it’s not part of the sources. Even if the file is not a header, I think this gives reasonable results.
Say, there was a new .cpp file in a target and for some reason the SwiftPMBuildSystem doesn’t know about it. Then we would infer the target based on the file’s location on disk and generate compiler arguments for it by picking a source file in that target, getting its compiler arguments and then patching up the compiler arguments by replacing the substitute file with the .cpp file.
There was a problem hiding this comment.
Did you also rename target(forHeader:)? Maybe inferTarget(for:)?
Address review comments to #1249
Instead of asking for build settings of a file, the build system manager asks for the targets of a file and then asks for the build settings of that file in a specific target. This has two advantages:
Depends on swiftlang/swift-package-manager#7540