From 4369d9f1dc2c9c8349520ccf15e6dceb882ff987 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Sat, 22 Jun 2024 09:11:10 -0700 Subject: [PATCH] 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 --- .../LLBuildManifestBuilder.swift | 27 +++---------- Sources/Build/BuildPlan/BuildPlan.swift | 38 ++++++++++++++++++- .../SourceKitLSPAPI/BuildDescription.swift | 27 +++++++++++++ Sources/Workspace/Workspace.swift | 9 ----- 4 files changed, 69 insertions(+), 32 deletions(-) diff --git a/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift b/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift index f83ec18193d..468365ba660 100644 --- a/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift +++ b/Sources/Build/BuildManifest/LLBuildManifestBuilder.swift @@ -174,28 +174,11 @@ public class LLBuildManifestBuilder { extension LLBuildManifestBuilder { private func addPackageStructureCommand() { - let inputs = self.plan.graph.rootPackages.flatMap { package -> [Node] in - var inputs = package.targets - .map(\.sources.root) - .sorted() - .map { Node.directoryStructure($0) } - - // Add the output paths of any prebuilds that were run, so that we redo the plan if they change. - var derivedSourceDirPaths: [AbsolutePath] = [] - for result in self.plan.prebuildCommandResults.values.flatMap({ $0 }) { - derivedSourceDirPaths.append(contentsOf: result.outputDirectories) + let inputs = self.plan.inputs.map { + switch $0 { + case .directoryStructure(let path): return Node.directoryStructure(path) + case .file(let path): return Node.file(path) } - inputs.append(contentsOf: derivedSourceDirPaths.sorted().map { Node.directoryStructure($0) }) - - // FIXME: Need to handle version-specific manifests. - inputs.append(file: package.manifest.path) - - // FIXME: This won't be the location of Package.resolved for multiroot packages. - inputs.append(file: package.path.appending("Package.resolved")) - - // FIXME: Add config file as an input - - return inputs } let name = "PackageStructure" @@ -215,7 +198,7 @@ extension LLBuildManifestBuilder { extension LLBuildManifestBuilder { // Creates commands for copying all binary artifacts depended on in the plan. private func addBinaryDependencyCommands() { - // Make sure we don't have multiple copy commands for each destination by mapping each destination to + // Make sure we don't have multiple copy commands for each destination by mapping each destination to // its source binary. var destinations = [AbsolutePath: AbsolutePath]() for target in self.plan.targetMap.values { diff --git a/Sources/Build/BuildPlan/BuildPlan.swift b/Sources/Build/BuildPlan/BuildPlan.swift index 261c6a58488..d615938f74e 100644 --- a/Sources/Build/BuildPlan/BuildPlan.swift +++ b/Sources/Build/BuildPlan/BuildPlan.swift @@ -165,6 +165,14 @@ extension BuildParameters { /// A build plan for a package graph. public class BuildPlan: SPMBuildCore.BuildPlan { + /// Return value of `inputs()` + package enum Input { + /// Any file in this directory affects the build plan + case directoryStructure(AbsolutePath) + /// The file at the given path affects the build plan + case file(AbsolutePath) + } + public enum Error: Swift.Error, CustomStringConvertible, Equatable { /// There is no buildable target in the graph. case noBuildableTarget @@ -173,7 +181,7 @@ public class BuildPlan: SPMBuildCore.BuildPlan { switch self { case .noBuildableTarget: return """ - The package does not contain a buildable target. + The package does not contain a buildable target. Add at least one `.target` or `.executableTarget` to your `Package.swift`. """ } @@ -656,6 +664,34 @@ public class BuildPlan: SPMBuildCore.BuildPlan { } return try description.symbolGraphExtractArguments() } + + /// Returns the files and directories that affect the build process of this build plan. + package var inputs: [Input] { + var inputs: [Input] = [] + for package in self.graph.rootPackages { + inputs += package.targets + .map(\.sources.root) + .sorted() + .map { .directoryStructure($0) } + + // Add the output paths of any prebuilds that were run, so that we redo the plan if they change. + var derivedSourceDirPaths: [AbsolutePath] = [] + for result in self.prebuildCommandResults.values.flatMap({ $0 }) { + derivedSourceDirPaths.append(contentsOf: result.outputDirectories) + } + inputs.append(contentsOf: derivedSourceDirPaths.sorted().map { .directoryStructure($0) }) + + // FIXME: Need to handle version-specific manifests. + inputs.append(.file(package.manifest.path)) + + // FIXME: This won't be the location of Package.resolved for multiroot packages. + inputs.append(.file(package.path.appending("Package.resolved"))) + + // FIXME: Add config file as an input + + } + return inputs + } } extension Basics.Diagnostic { diff --git a/Sources/SourceKitLSPAPI/BuildDescription.swift b/Sources/SourceKitLSPAPI/BuildDescription.swift index 8d7391217d5..a2503dcd95d 100644 --- a/Sources/SourceKitLSPAPI/BuildDescription.swift +++ b/Sources/SourceKitLSPAPI/BuildDescription.swift @@ -104,9 +104,14 @@ private struct WrappedSwiftTargetBuildDescription: BuildTarget { public struct BuildDescription { private let buildPlan: Build.BuildPlan + /// The inputs of the build plan so we don't need to re-compute them on every call to + /// `fileAffectsSwiftOrClangBuildSettings`. + private let inputs: [BuildPlan.Input] + // FIXME: should not use `BuildPlan` in the public interface public init(buildPlan: Build.BuildPlan) { self.buildPlan = buildPlan + self.inputs = buildPlan.inputs } // FIXME: should not use `ResolvedTarget` in the public interface @@ -143,4 +148,26 @@ public struct BuildDescription { getBuildTarget(for: $0, in: modulesGraph) } } + + /// Returns `true` if the file at the given path might influence build settings for a `swiftc` or `clang` invocation + /// generated by SwiftPM. + public func fileAffectsSwiftOrClangBuildSettings(_ url: URL) -> Bool { + guard let filePath = try? AbsolutePath(validating: url.path) else { + return false + } + + for input in self.inputs { + switch input { + case .directoryStructure(let path): + if path.isAncestor(of: filePath) { + return true + } + case .file(let path): + if filePath == path { + return true + } + } + } + return false + } } diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index 668274d4fac..b8453971898 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -1204,15 +1204,6 @@ extension Workspace { } } - /// Returns `true` if the file at the given path might influence build settings for a `swiftc` or `clang` invocation - /// generated by SwiftPM. - public func fileAffectsSwiftOrClangBuildSettings(filePath: AbsolutePath, packageGraph: ModulesGraph) -> Bool { - // TODO: Implement a more sophisticated check that also verifies if the file is in the sources directories of the passed in `packageGraph`. - FileRuleDescription.builtinRules.contains { fileRuleDescription in - fileRuleDescription.match(path: filePath, toolsVersion: self.currentToolsVersion) - } - } - public func acceptIdentityChange( package: PackageIdentity, version: Version,