Skip to content

Commit c21a148

Browse files
authored
[6.0] Implement fileAffectsSwiftOrClangBuildSettings with the logic from LLBuildManifestBuilder (#7709)
- **Explanation**: Instead of inspecting file extensions to decide whether a file might affect build settings, check which files are in directories that are affecting compilation. This is the same logic that `LLBuildManifestBuilder` uses and should be more stable. In particular, this stops us from reloading the package manifest in SourceKit-LSP when a header file is written to a `.build` directory. - **Scope**: Only affects SourceKit-LSP - **Risk**: Low, only affects SourceKit-LSP - **Testing**: Adding a SourceKit-LSP test case in swiftlang/sourcekit-lsp#1512 - **Issue**: rdar://128573306 - **Reviewer**: @MaxDesiatov @xedin on #7699
1 parent 001c74a commit c21a148

File tree

4 files changed

+69
-32
lines changed

4 files changed

+69
-32
lines changed

Sources/Build/BuildManifest/LLBuildManifestBuilder.swift

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -174,28 +174,11 @@ public class LLBuildManifestBuilder {
174174

175175
extension LLBuildManifestBuilder {
176176
private func addPackageStructureCommand() {
177-
let inputs = self.plan.graph.rootPackages.flatMap { package -> [Node] in
178-
var inputs = package.targets
179-
.map(\.sources.root)
180-
.sorted()
181-
.map { Node.directoryStructure($0) }
182-
183-
// Add the output paths of any prebuilds that were run, so that we redo the plan if they change.
184-
var derivedSourceDirPaths: [AbsolutePath] = []
185-
for result in self.plan.prebuildCommandResults.values.flatMap({ $0 }) {
186-
derivedSourceDirPaths.append(contentsOf: result.outputDirectories)
177+
let inputs = self.plan.inputs.map {
178+
switch $0 {
179+
case .directoryStructure(let path): return Node.directoryStructure(path)
180+
case .file(let path): return Node.file(path)
187181
}
188-
inputs.append(contentsOf: derivedSourceDirPaths.sorted().map { Node.directoryStructure($0) })
189-
190-
// FIXME: Need to handle version-specific manifests.
191-
inputs.append(file: package.manifest.path)
192-
193-
// FIXME: This won't be the location of Package.resolved for multiroot packages.
194-
inputs.append(file: package.path.appending("Package.resolved"))
195-
196-
// FIXME: Add config file as an input
197-
198-
return inputs
199182
}
200183

201184
let name = "PackageStructure"
@@ -215,7 +198,7 @@ extension LLBuildManifestBuilder {
215198
extension LLBuildManifestBuilder {
216199
// Creates commands for copying all binary artifacts depended on in the plan.
217200
private func addBinaryDependencyCommands() {
218-
// Make sure we don't have multiple copy commands for each destination by mapping each destination to
201+
// Make sure we don't have multiple copy commands for each destination by mapping each destination to
219202
// its source binary.
220203
var destinations = [AbsolutePath: AbsolutePath]()
221204
for target in self.plan.targetMap.values {

Sources/Build/BuildPlan/BuildPlan.swift

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,14 @@ extension BuildParameters {
165165

166166
/// A build plan for a package graph.
167167
public class BuildPlan: SPMBuildCore.BuildPlan {
168+
/// Return value of `inputs()`
169+
package enum Input {
170+
/// Any file in this directory affects the build plan
171+
case directoryStructure(AbsolutePath)
172+
/// The file at the given path affects the build plan
173+
case file(AbsolutePath)
174+
}
175+
168176
public enum Error: Swift.Error, CustomStringConvertible, Equatable {
169177
/// There is no buildable target in the graph.
170178
case noBuildableTarget
@@ -173,7 +181,7 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
173181
switch self {
174182
case .noBuildableTarget:
175183
return """
176-
The package does not contain a buildable target.
184+
The package does not contain a buildable target.
177185
Add at least one `.target` or `.executableTarget` to your `Package.swift`.
178186
"""
179187
}
@@ -656,6 +664,34 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
656664
}
657665
return try description.symbolGraphExtractArguments()
658666
}
667+
668+
/// Returns the files and directories that affect the build process of this build plan.
669+
package var inputs: [Input] {
670+
var inputs: [Input] = []
671+
for package in self.graph.rootPackages {
672+
inputs += package.targets
673+
.map(\.sources.root)
674+
.sorted()
675+
.map { .directoryStructure($0) }
676+
677+
// Add the output paths of any prebuilds that were run, so that we redo the plan if they change.
678+
var derivedSourceDirPaths: [AbsolutePath] = []
679+
for result in self.prebuildCommandResults.values.flatMap({ $0 }) {
680+
derivedSourceDirPaths.append(contentsOf: result.outputDirectories)
681+
}
682+
inputs.append(contentsOf: derivedSourceDirPaths.sorted().map { .directoryStructure($0) })
683+
684+
// FIXME: Need to handle version-specific manifests.
685+
inputs.append(.file(package.manifest.path))
686+
687+
// FIXME: This won't be the location of Package.resolved for multiroot packages.
688+
inputs.append(.file(package.path.appending("Package.resolved")))
689+
690+
// FIXME: Add config file as an input
691+
692+
}
693+
return inputs
694+
}
659695
}
660696

661697
extension Basics.Diagnostic {

Sources/SourceKitLSPAPI/BuildDescription.swift

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,14 @@ private struct WrappedSwiftTargetBuildDescription: BuildTarget {
104104
public struct BuildDescription {
105105
private let buildPlan: Build.BuildPlan
106106

107+
/// The inputs of the build plan so we don't need to re-compute them on every call to
108+
/// `fileAffectsSwiftOrClangBuildSettings`.
109+
private let inputs: [BuildPlan.Input]
110+
107111
// FIXME: should not use `BuildPlan` in the public interface
108112
public init(buildPlan: Build.BuildPlan) {
109113
self.buildPlan = buildPlan
114+
self.inputs = buildPlan.inputs
110115
}
111116

112117
// FIXME: should not use `ResolvedTarget` in the public interface
@@ -143,4 +148,26 @@ public struct BuildDescription {
143148
getBuildTarget(for: $0, in: modulesGraph)
144149
}
145150
}
151+
152+
/// Returns `true` if the file at the given path might influence build settings for a `swiftc` or `clang` invocation
153+
/// generated by SwiftPM.
154+
public func fileAffectsSwiftOrClangBuildSettings(_ url: URL) -> Bool {
155+
guard let filePath = try? AbsolutePath(validating: url.path) else {
156+
return false
157+
}
158+
159+
for input in self.inputs {
160+
switch input {
161+
case .directoryStructure(let path):
162+
if path.isAncestor(of: filePath) {
163+
return true
164+
}
165+
case .file(let path):
166+
if filePath == path {
167+
return true
168+
}
169+
}
170+
}
171+
return false
172+
}
146173
}

Sources/Workspace/Workspace.swift

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1197,15 +1197,6 @@ extension Workspace {
11971197
}
11981198
}
11991199

1200-
/// Returns `true` if the file at the given path might influence build settings for a `swiftc` or `clang` invocation
1201-
/// generated by SwiftPM.
1202-
public func fileAffectsSwiftOrClangBuildSettings(filePath: AbsolutePath, packageGraph: ModulesGraph) -> Bool {
1203-
// TODO: Implement a more sophisticated check that also verifies if the file is in the sources directories of the passed in `packageGraph`.
1204-
FileRuleDescription.builtinRules.contains { fileRuleDescription in
1205-
fileRuleDescription.match(path: filePath, toolsVersion: self.currentToolsVersion)
1206-
}
1207-
}
1208-
12091200
public func acceptIdentityChange(
12101201
package: PackageIdentity,
12111202
version: Version,

0 commit comments

Comments
 (0)