From 34b2f15e85243b02690517a58842e27eb793617d Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Mon, 20 Jan 2025 10:48:23 -0500 Subject: [PATCH 1/2] Eliminate the warning that swift-syntax isn't being used. Assume dependent products from prebuilts are being used. --- Sources/PackageGraph/ModulesGraph+Loading.swift | 12 ++++++++++-- Tests/WorkspaceTests/PrebuiltsTests.swift | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index ccbfbae3adf..783a87263a7 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -260,7 +260,12 @@ extension ModulesGraph { ) let rootPackages = resolvedPackages.filter { root.manifests.values.contains($0.manifest) } - checkAllDependenciesAreUsed(packages: resolvedPackages, rootPackages, observabilityScope: observabilityScope) + checkAllDependenciesAreUsed( + packages: resolvedPackages, + rootPackages, + prebuilts: prebuilts, + observabilityScope: observabilityScope + ) return try ModulesGraph( rootPackages: rootPackages, @@ -275,6 +280,7 @@ extension ModulesGraph { private func checkAllDependenciesAreUsed( packages: IdentifiableSet, _ rootPackages: [ResolvedPackage], + prebuilts: [PackageIdentity: [String: PrebuiltLibrary]], observabilityScope: ObservabilityScope ) { for package in rootPackages { @@ -352,9 +358,11 @@ private func checkAllDependenciesAreUsed( let usedByPackage = productDependencies.contains { $0.name == product.name } // We check if any of the products of this dependency is guarded by a trait. let traitGuarded = traitGuardedProductDependencies.contains(product.name) + // Consider prebuilts as used + let prebuilt = prebuilts[dependency.identity]?.keys.contains(product.name) ?? false // If the product is either used directly or guarded by a trait we consider it as used - return usedByPackage || traitGuarded + return usedByPackage || traitGuarded || prebuilt } if !dependencyIsUsed && !observabilityScope.errorsReportedInAnyScope { diff --git a/Tests/WorkspaceTests/PrebuiltsTests.swift b/Tests/WorkspaceTests/PrebuiltsTests.swift index 7083a4365a5..d25ce9e494a 100644 --- a/Tests/WorkspaceTests/PrebuiltsTests.swift +++ b/Tests/WorkspaceTests/PrebuiltsTests.swift @@ -168,7 +168,7 @@ final class PrebuiltsTests: XCTestCase { ) try await workspace.checkPackageGraph(roots: ["Foo"]) { modulesGraph, diagnostics in - XCTAssertTrue(diagnostics.filter({ $0.severity == .error }).isEmpty) + XCTAssertTrue(diagnostics.filter({ $0.severity == .error || $0.severity == .warning }).isEmpty) let rootPackage = try XCTUnwrap(modulesGraph.rootPackages.first) try checkSettings(rootPackage, "FooMacros", usePrebuilt: true) try checkSettings(rootPackage, "FooTests", usePrebuilt: true) From 8076391a27a35e4cae0315554fc85f7c16c026c8 Mon Sep 17 00:00:00 2001 From: Doug Schaefer Date: Tue, 21 Jan 2025 16:16:30 -0500 Subject: [PATCH 2/2] Remove unused prebuilts from workspace state. Found a bug where if you change swift syntax version in your manifest, we were not changing the use of prebuilts to match. This adds a scan to do so. Also requires that we add the version of the prebuilt to the workspace state. --- .../PackageGraph/ModulesGraph+Loading.swift | 1 - Sources/Workspace/ManagedPrebuilt.swift | 41 ++++--- Sources/Workspace/Workspace+Prebuilts.swift | 34 +++--- Sources/Workspace/Workspace+State.swift | 5 + .../ManifestExtensions.swift | 23 ++++ .../_InternalTestSupport/MockWorkspace.swift | 2 +- Tests/WorkspaceTests/PrebuiltsTests.swift | 110 +++++++++++++++++- 7 files changed, 181 insertions(+), 35 deletions(-) diff --git a/Sources/PackageGraph/ModulesGraph+Loading.swift b/Sources/PackageGraph/ModulesGraph+Loading.swift index 783a87263a7..e1576cfd7dd 100644 --- a/Sources/PackageGraph/ModulesGraph+Loading.swift +++ b/Sources/PackageGraph/ModulesGraph+Loading.swift @@ -361,7 +361,6 @@ private func checkAllDependenciesAreUsed( // Consider prebuilts as used let prebuilt = prebuilts[dependency.identity]?.keys.contains(product.name) ?? false - // If the product is either used directly or guarded by a trait we consider it as used return usedByPackage || traitGuarded || prebuilt } diff --git a/Sources/Workspace/ManagedPrebuilt.swift b/Sources/Workspace/ManagedPrebuilt.swift index e51b564bff2..a148f936843 100644 --- a/Sources/Workspace/ManagedPrebuilt.swift +++ b/Sources/Workspace/ManagedPrebuilt.swift @@ -10,6 +10,8 @@ // //===----------------------------------------------------------------------===// +import struct TSCUtility.Version + import Basics import PackageModel @@ -19,6 +21,9 @@ extension Workspace { /// The package identity public let identity: PackageIdentity + /// The package version + public let version: Version + /// The name of the binary target the artifact corresponds to. public let libraryName: String @@ -45,60 +50,60 @@ extension Workspace { /// A collection of managed artifacts which have been downloaded. public final class ManagedPrebuilts { /// A mapping from package identity, to target name, to ManagedArtifact. - private var artifactMap: [PackageIdentity: [String: ManagedPrebuilt]] + private var prebuiltMap: [PackageIdentity: [String: ManagedPrebuilt]] - internal var artifacts: AnyCollection { - AnyCollection(self.artifactMap.values.lazy.flatMap{ $0.values }) + internal var prebuilts: AnyCollection { + AnyCollection(self.prebuiltMap.values.lazy.flatMap{ $0.values }) } init() { - self.artifactMap = [:] + self.prebuiltMap = [:] } - init(_ artifacts: [ManagedPrebuilt]) throws { - let artifactsByPackagePath = Dictionary(grouping: artifacts, by: { $0.identity }) - self.artifactMap = try artifactsByPackagePath.mapValues{ artifacts in - try Dictionary(artifacts.map { ($0.libraryName, $0) }, uniquingKeysWith: { _, _ in + init(_ prebuilts: [ManagedPrebuilt]) throws { + let prebuiltsByPackagePath = Dictionary(grouping: prebuilts, by: { $0.identity }) + self.prebuiltMap = try prebuiltsByPackagePath.mapValues{ prebuilt in + try Dictionary(prebuilt.map { ($0.libraryName, $0) }, uniquingKeysWith: { _, _ in // should be unique - throw StringError("binary artifact already exists in managed artifacts") + throw StringError("prebuilt already exists in managed prebuilts") }) } } public subscript(packageIdentity packageIdentity: PackageIdentity, targetName targetName: String) -> ManagedPrebuilt? { - self.artifactMap[packageIdentity]?[targetName] + self.prebuiltMap[packageIdentity]?[targetName] } - public func add(_ artifact: ManagedPrebuilt) { - self.artifactMap[artifact.identity, default: [:]][artifact.libraryName] = artifact + public func add(_ prebuilt: ManagedPrebuilt) { + self.prebuiltMap[prebuilt.identity, default: [:]][prebuilt.libraryName] = prebuilt } public func remove(packageIdentity: PackageIdentity, targetName: String) { - self.artifactMap[packageIdentity]?[targetName] = nil + self.prebuiltMap[packageIdentity]?[targetName] = nil } } } extension Workspace.ManagedPrebuilts: Collection { public var startIndex: AnyIndex { - self.artifacts.startIndex + self.prebuilts.startIndex } public var endIndex: AnyIndex { - self.artifacts.endIndex + self.prebuilts.endIndex } public subscript(index: AnyIndex) -> Workspace.ManagedPrebuilt { - self.artifacts[index] + self.prebuilts[index] } public func index(after index: AnyIndex) -> AnyIndex { - self.artifacts.index(after: index) + self.prebuilts.index(after: index) } } extension Workspace.ManagedPrebuilts: CustomStringConvertible { public var description: String { - "" + "" } } diff --git a/Sources/Workspace/Workspace+Prebuilts.swift b/Sources/Workspace/Workspace+Prebuilts.swift index 8d7afd0021f..71e455477f1 100644 --- a/Sources/Workspace/Workspace+Prebuilts.swift +++ b/Sources/Workspace/Workspace+Prebuilts.swift @@ -269,8 +269,9 @@ extension Workspace { if fileSystem.exists(destination) { do { return try JSONDecoder().decode( - PrebuiltsManifest.self, - from: try Data(contentsOf: destination.asURL) + path: destination, + fileSystem: fileSystem, + as: PrebuiltsManifest.self ) } catch { // redownload it @@ -325,10 +326,10 @@ extension Workspace { } do { - let data = try fileSystem.readFileContents(destination) return try JSONDecoder().decode( - PrebuiltsManifest.self, - from: Data(data.contents) + path: destination, + fileSystem: fileSystem, + as: PrebuiltsManifest.self ) } catch { observabilityScope.emit( @@ -489,13 +490,11 @@ extension Workspace { return } - for prebuilt in prebuiltsManager.findPrebuilts( - packages: try manifests.requiredPackages - ) { + let addedPrebuilts = ManagedPrebuilts() + + for prebuilt in prebuiltsManager.findPrebuilts(packages: try manifests.requiredPackages) { guard - let manifest = manifests.allDependencyManifests[ - prebuilt.identity - ], + let manifest = manifests.allDependencyManifests[prebuilt.identity], let packageVersion = manifest.manifest.version, let prebuiltManifest = try await prebuiltsManager .downloadManifest( @@ -523,21 +522,30 @@ extension Workspace { artifact: artifact, observabilityScope: observabilityScope ) - { + { // Add to workspace state let managedPrebuilt = ManagedPrebuilt( identity: prebuilt.identity, + version: packageVersion, libraryName: library.name, path: path, products: library.products, cModules: library.cModules ) + addedPrebuilts.add(managedPrebuilt) self.state.prebuilts.add(managedPrebuilt) - try self.state.save() } } } } + + for prebuilt in self.state.prebuilts.prebuilts { + if !addedPrebuilts.contains(where: { $0.identity == prebuilt.identity && $0.version == prebuilt.version }) { + self.state.prebuilts.remove(packageIdentity: prebuilt.identity, targetName: prebuilt.libraryName) + } + } + + try self.state.save() } var hostPrebuiltsPlatform: PrebuiltsManifest.Platform? { diff --git a/Sources/Workspace/Workspace+State.swift b/Sources/Workspace/Workspace+State.swift index 9711f77a231..7027dcb2491 100644 --- a/Sources/Workspace/Workspace+State.swift +++ b/Sources/Workspace/Workspace+State.swift @@ -10,6 +10,8 @@ // //===----------------------------------------------------------------------===// +import struct TSCUtility.Version + import Basics import Foundation import PackageGraph @@ -447,6 +449,7 @@ extension WorkspaceStateStorage { struct Prebuilt: Codable { let identity: PackageIdentity + let version: TSCUtility.Version let libraryName: String let path: AbsolutePath let products: [String] @@ -454,6 +457,7 @@ extension WorkspaceStateStorage { init(_ managedPrebuilt: Workspace.ManagedPrebuilt) { self.identity = managedPrebuilt.identity + self.version = managedPrebuilt.version self.libraryName = managedPrebuilt.libraryName self.path = managedPrebuilt.path self.products = managedPrebuilt.products @@ -527,6 +531,7 @@ extension Workspace.ManagedPrebuilt { fileprivate init(_ prebuilt: WorkspaceStateStorage.V7.Prebuilt) throws { self.init( identity: prebuilt.identity, + version: prebuilt.version, libraryName: prebuilt.libraryName, path: prebuilt.path, products: prebuilt.products, diff --git a/Sources/_InternalTestSupport/ManifestExtensions.swift b/Sources/_InternalTestSupport/ManifestExtensions.swift index 13268bbac57..1c25966c30d 100644 --- a/Sources/_InternalTestSupport/ManifestExtensions.swift +++ b/Sources/_InternalTestSupport/ManifestExtensions.swift @@ -266,4 +266,27 @@ extension Manifest { traits: self.traits ) } + + public func with(dependencies: [PackageDependency]) -> Manifest { + Manifest( + displayName: self.displayName, + path: self.path, + packageKind: self.packageKind, + packageLocation: self.packageLocation, + defaultLocalization: self.defaultLocalization, + platforms: self.platforms, + version: self.version, + revision: self.revision, + toolsVersion: self.toolsVersion, + pkgConfig: self.pkgConfig, + providers: self.providers, + cLanguageStandard: self.cLanguageStandard, + cxxLanguageStandard: self.cxxLanguageStandard, + swiftLanguageVersions: self.swiftLanguageVersions, + dependencies: dependencies, + products: self.products, + targets: self.targets, + traits: self.traits + ) + } } diff --git a/Sources/_InternalTestSupport/MockWorkspace.swift b/Sources/_InternalTestSupport/MockWorkspace.swift index 8e2ef02e196..03c1b533d1c 100644 --- a/Sources/_InternalTestSupport/MockWorkspace.swift +++ b/Sources/_InternalTestSupport/MockWorkspace.swift @@ -507,7 +507,7 @@ public final class MockWorkspace { public func checkPackageGraph( roots: [String] = [], deps: [MockDependency], - _ result: (ModulesGraph, [Basics.Diagnostic]) -> Void + _ result: (ModulesGraph, [Basics.Diagnostic]) throws -> Void ) async throws { let dependencies = try deps.map { try $0.convert(baseURL: packagesDir, identityResolver: self.identityResolver) } try await self.checkPackageGraph(roots: roots, dependencies: dependencies, result) diff --git a/Tests/WorkspaceTests/PrebuiltsTests.swift b/Tests/WorkspaceTests/PrebuiltsTests.swift index d25ce9e494a..7d22d8aa86d 100644 --- a/Tests/WorkspaceTests/PrebuiltsTests.swift +++ b/Tests/WorkspaceTests/PrebuiltsTests.swift @@ -87,6 +87,7 @@ final class PrebuiltsTests: XCTestCase { ] ) + let swiftSyntax = try MockPackage( name: "swift-syntax", url: swiftSyntaxURL, @@ -100,7 +101,7 @@ final class PrebuiltsTests: XCTestCase { MockProduct(name: "SwiftCompilerPlugin", modules: ["SwiftCompilerPlugin"]), MockProduct(name: "SwiftSyntaxMacros", modules: ["SwiftSyntaxMacros"]), ], - versions: ["600.0.1", "600.0.2"] + versions: ["600.0.1", "600.0.2", "601.0.0"] ) return (manifest, rootPackage, swiftSyntax) @@ -139,7 +140,7 @@ final class PrebuiltsTests: XCTestCase { } else if request.url == "https://github.com/dschaefer2/swift-syntax/releases/download/600.0.1/\(self.swiftVersion)-MacroSupport-macos_aarch64.zip" { try fileSystem.writeFileContents(destination, data: artifact) return .okay() - } else { + } else { XCTFail("Unexpected URL \(request.url)") return .notFound() } @@ -177,6 +178,110 @@ final class PrebuiltsTests: XCTestCase { } } + func testVersionChange() async throws { + let sandbox = AbsolutePath("/tmp/ws") + let fs = InMemoryFileSystem() + + let artifact = Data() + let (manifest, rootPackage, swiftSyntax) = try initData(artifact: artifact, swiftSyntaxVersion: "600.0.1") + let manifestData = try JSONEncoder().encode(manifest) + + let httpClient = HTTPClient { request, progressHandler in + guard case .download(let fileSystem, let destination) = request.kind else { + throw StringError("invalid request \(request.kind)") + } + + if request.url == "https://github.com/dschaefer2/swift-syntax/releases/download/600.0.1/\(self.swiftVersion)-manifest.json" { + try fileSystem.writeFileContents(destination, data: manifestData) + return .okay() + } else if request.url == "https://github.com/dschaefer2/swift-syntax/releases/download/600.0.1/\(self.swiftVersion)-MacroSupport-macos_aarch64.zip" { + try fileSystem.writeFileContents(destination, data: artifact) + return .okay() + } else { + // make sure it's the updated one + XCTAssertEqual( + request.url, + "https://github.com/dschaefer2/swift-syntax/releases/download/601.0.0/\(self.swiftVersion)-manifest.json" + ) + return .notFound() + } + } + + let archiver = MockArchiver(handler: { _, archivePath, destination, completion in + XCTAssertEqual(archivePath.pathString, "/home/user/caches/org.swift.swiftpm/prebuilts/swift-syntax/600.0.1/\(self.swiftVersion)-MacroSupport-macos_aarch64.zip".fixwin) + XCTAssertEqual(destination.pathString, "/tmp/ws/.build/prebuilts/swift-syntax/\(self.swiftVersion)-MacroSupport-macos_aarch64".fixwin) + completion(.success(())) + }) + + let workspace = try await MockWorkspace( + sandbox: sandbox, + fileSystem: fs, + roots: [ + rootPackage + ], + packages: [ + swiftSyntax + ], + prebuiltsManager: .init( + httpClient: httpClient, + archiver: archiver + ), + customHostTriple: Triple("arm64-apple-macosx15.0") + ) + + try await workspace.checkPackageGraph(roots: [rootPackage.name]) { modulesGraph, diagnostics in + XCTAssertTrue(diagnostics.filter({ $0.severity == .error || $0.severity == .warning }).isEmpty) + let rootPackage = try XCTUnwrap(modulesGraph.rootPackages.first) + try checkSettings(rootPackage, "FooMacros", usePrebuilt: true) + try checkSettings(rootPackage, "FooTests", usePrebuilt: true) + try checkSettings(rootPackage, "Foo", usePrebuilt: false) + try checkSettings(rootPackage, "FooClient", usePrebuilt: false) + } + + // Change the version of swift syntax to one that doesn't have prebuilts + try workspace.closeWorkspace(resetState: false, resetResolvedFile: false) + let key = MockManifestLoader.Key(url: sandbox.appending(components: "roots", rootPackage.name).pathString) + let oldManifest = try XCTUnwrap(workspace.manifestLoader.manifests[key]) + let oldSCM: PackageDependency.SourceControl + if case let .sourceControl(scm) = oldManifest.dependencies[0] { + oldSCM = scm + } else { + XCTFail("not source control") + return + } + let newDep = PackageDependency.sourceControl( + identity: oldSCM.identity, + nameForTargetDependencyResolutionOnly: oldSCM.nameForTargetDependencyResolutionOnly, + location: oldSCM.location, + requirement: .exact(try XCTUnwrap(Version("601.0.0"))), + productFilter: oldSCM.productFilter + ) + let newManifest = oldManifest.with(dependencies: [newDep]) + workspace.manifestLoader.manifests[key] = newManifest + + try await workspace.checkPackageGraph(roots: [rootPackage.name]) { modulesGraph, diagnostics in + XCTAssertTrue(diagnostics.filter({ $0.severity == .error || $0.severity == .warning }).isEmpty) + let rootPackage = try XCTUnwrap(modulesGraph.rootPackages.first) + try checkSettings(rootPackage, "FooMacros", usePrebuilt: false) + try checkSettings(rootPackage, "FooTests", usePrebuilt: false) + try checkSettings(rootPackage, "Foo", usePrebuilt: false) + try checkSettings(rootPackage, "FooClient", usePrebuilt: false) + } + + // Change it back + try workspace.closeWorkspace(resetState: false, resetResolvedFile: false) + workspace.manifestLoader.manifests[key] = oldManifest + + try await workspace.checkPackageGraph(roots: [rootPackage.name]) { modulesGraph, diagnostics in + XCTAssertTrue(diagnostics.filter({ $0.severity == .error || $0.severity == .warning }).isEmpty) + let rootPackage = try XCTUnwrap(modulesGraph.rootPackages.first) + try checkSettings(rootPackage, "FooMacros", usePrebuilt: true) + try checkSettings(rootPackage, "FooTests", usePrebuilt: true) + try checkSettings(rootPackage, "Foo", usePrebuilt: false) + try checkSettings(rootPackage, "FooClient", usePrebuilt: false) + } + } + func testSSHURL() async throws { let sandbox = AbsolutePath("/tmp/ws") let fs = InMemoryFileSystem() @@ -233,6 +338,7 @@ final class PrebuiltsTests: XCTestCase { try checkSettings(rootPackage, "FooClient", usePrebuilt: false) } } + func testCachedArtifact() async throws { let sandbox = AbsolutePath("/tmp/ws") let fs = InMemoryFileSystem()