Skip to content

Commit 99d92ca

Browse files
committed
Implement fileAffectsSwiftOrClangBuildSettings with the logic from LLBuildManifestBuilder
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. rdar://128573306
1 parent b86d22e commit 99d92ca

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
@@ -173,28 +173,11 @@ public class LLBuildManifestBuilder {
173173

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

200183
let name = "PackageStructure"
@@ -214,7 +197,7 @@ extension LLBuildManifestBuilder {
214197
extension LLBuildManifestBuilder {
215198
// Creates commands for copying all binary artifacts depended on in the plan.
216199
private func addBinaryDependencyCommands() {
217-
// Make sure we don't have multiple copy commands for each destination by mapping each destination to
200+
// Make sure we don't have multiple copy commands for each destination by mapping each destination to
218201
// its source binary.
219202
var destinations = [AbsolutePath: AbsolutePath]()
220203
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
@@ -163,6 +163,14 @@ extension BuildParameters {
163163

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

659695
extension Basics.Diagnostic {

Sources/SourceKitLSPAPI/BuildDescription.swift

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

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

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

Sources/Workspace/Workspace.swift

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1240,15 +1240,6 @@ extension Workspace {
12401240
}
12411241
}
12421242

1243-
/// Returns `true` if the file at the given path might influence build settings for a `swiftc` or `clang` invocation
1244-
/// generated by SwiftPM.
1245-
public func fileAffectsSwiftOrClangBuildSettings(filePath: AbsolutePath, packageGraph: ModulesGraph) -> Bool {
1246-
// TODO: Implement a more sophisticated check that also verifies if the file is in the sources directories of the passed in `packageGraph`.
1247-
FileRuleDescription.builtinRules.contains { fileRuleDescription in
1248-
fileRuleDescription.match(path: filePath, toolsVersion: self.currentToolsVersion)
1249-
}
1250-
}
1251-
12521243
public func acceptIdentityChange(
12531244
package: PackageIdentity,
12541245
version: Version,

0 commit comments

Comments
 (0)