From 35e7c9127bc430d949b5a2d13dbebe8356f83005 Mon Sep 17 00:00:00 2001 From: Ankit Aggarwal Date: Tue, 16 Jul 2019 11:24:59 -0700 Subject: [PATCH] Revert "[Workspace] Improve how we handle changes to Package.resolved" This reverts commit 32e144d605bbd107c70a65056b7870217d06fd27. --- Sources/Workspace/PinsStore.swift | 4 - Sources/Workspace/Workspace.swift | 92 ++++++++---------- Tests/WorkspaceTests/WorkspaceTests.swift | 105 +-------------------- Tests/WorkspaceTests/XCTestManifests.swift | 1 - 4 files changed, 43 insertions(+), 159 deletions(-) diff --git a/Sources/Workspace/PinsStore.swift b/Sources/Workspace/PinsStore.swift index 7627f39430a..fd2449c2e5f 100644 --- a/Sources/Workspace/PinsStore.swift +++ b/Sources/Workspace/PinsStore.swift @@ -99,10 +99,6 @@ public final class PinsStore { pinsMap[pin.packageRef.identity] = pin } - func remove(identity: String) { - pinsMap[identity] = nil - } - /// Pin a managed dependency at its checkout state. /// /// This method does nothing if the dependency is in edited state. diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift index 14a9fda562c..4e697938a26 100644 --- a/Sources/Workspace/Workspace.swift +++ b/Sources/Workspace/Workspace.swift @@ -1236,27 +1236,21 @@ extension Workspace { return currentManifests } - // Load the pinstore and check if its outdated. - guard let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else { + validatePinsStore(dependencyManifests: currentManifests, diagnostics: diagnostics) + + // Abort if pinsStore is unloadable or if diagnostics has errors. + guard !diagnostics.hasErrors, let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else { return currentManifests } - let isPinStoreOutdated = pinsStore.isOutdated( - dependencyManifests: currentManifests, - managedDependencies: managedDependencies - ) - // Compute the missing package identities and scrub any out-of-date entry from the pin store.. + // Compute the missing package identities. let missingPackageURLs = currentManifests.missingPackageURLs() - for missingURL in missingPackageURLs { - pinsStore.remove(identity: missingURL.identity) - try? pinsStore.saveState() - } // The pins to use in case we need to run the resolution. var validPins = pinsStore.createConstraints() // Compute if we need to run the resolver. We always run the resolver if - // there are missing packages. + // there are extra constraints. if missingPackageURLs.isEmpty { // Use root constraints, dependency manifest constraints and extra // constraints to compute if a new resolution is required. @@ -1269,11 +1263,9 @@ extension Workspace { let result = isResolutionRequired(root: graphRoot, dependencies: dependencies, pinsStore: pinsStore) - // We don't need to resolve if: - // 1. Nothing in the manifest changed. - // 2. PinStore is valid. - // 3. We have no extra constraints. - if !result.resolve && extraConstraints.isEmpty && !isPinStoreOutdated { + // If we don't need resolution and there are no extra constraints, + // just validate pinsStore and return. + if !result.resolve && extraConstraints.isEmpty { return currentManifests } @@ -1472,7 +1464,38 @@ extension Workspace { } } - return (false, validPins) + return (false, []) + } + + /// Validates that each checked out managed dependency has an entry in pinsStore. + private func validatePinsStore(dependencyManifests: DependencyManifests, diagnostics: DiagnosticsEngine) { + guard let pinsStore = diagnostics.wrap({ try pinsStore.load() }) else { + return + } + + let pins = pinsStore.pinsMap.keys + let requiredURLs = dependencyManifests.computePackageURLs().required + + for dependency in managedDependencies.values { + switch dependency.state { + case .checkout: break + case .edited, .local: continue + } + + let identity = dependency.packageRef.identity + + if requiredURLs.contains(where: { $0.path == dependency.packageRef.path }) { + // If required identity contains this dependency, it should be in the pins store. + if let pin = pinsStore.pinsMap[identity], pin.packageRef.path == dependency.packageRef.path { + continue + } + } else if !pins.contains(identity) { + // Otherwise, it should *not* be in the pins store. + continue + } + + return self.pinAll(dependencyManifests: dependencyManifests, pinsStore: pinsStore, diagnostics: diagnostics) + } } /// This enum represents state of an external package. @@ -1993,36 +2016,3 @@ public final class LoadableResult { return try loadResult().dematerialize() } } - -extension PinsStore { - /// Returns true if the pin store is outdated with the content in the managed dependencies. - func isOutdated( - dependencyManifests: Workspace.DependencyManifests, - managedDependencies: ManagedDependencies - ) -> Bool { - let pins = pinsMap.keys - let requiredURLs = dependencyManifests.computePackageURLs().required - - for dependency in managedDependencies.values { - switch dependency.state { - case .checkout: break - case .edited, .local: continue - } - - let identity = dependency.packageRef.identity - - if requiredURLs.contains(where: { $0.path == dependency.packageRef.path }) { - // If required identity contains this dependency, it should be in the pins store. - if let pin = pinsMap[identity], pin.packageRef.path == dependency.packageRef.path { - continue - } - } else if !pins.contains(identity) { - // Otherwise, it should *not* be in the pins store. - continue - } - - return true - } - return false - } -} diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index a3135800728..d9aa975fb9a 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -861,7 +861,7 @@ final class WorkspaceTests: XCTestCase { ], pinsStore: pinsStore) XCTAssertEqual(result.resolve, false) - XCTAssertEqual(result.validPins.map({$0.identifier.repository.url}).sorted(), ["/A", "/B", "/C"]) + XCTAssertEqual(result.validPins, []) } } @@ -2575,7 +2575,7 @@ final class WorkspaceTests: XCTestCase { XCTAssertNoDiagnostics(diagnostics) } workspace.checkManagedDependencies() { result in - result.check(notPresent: "foo") + result.check(dependency: "foo", at: .checkout(.version("1.0.0"))) } workspace.checkResolved() { result in result.check(notPresent: "foo") @@ -3049,107 +3049,6 @@ final class WorkspaceTests: XCTestCase { } } - func testUpdateToResolvedFileResolvesCorrectly() throws { - let sandbox = AbsolutePath("/tmp/ws/") - let fs = InMemoryFileSystem() - - let workspace = try TestWorkspace( - sandbox: sandbox, - fs: fs, - roots: [ - TestPackage( - name: "Root", - targets: [ - TestTarget(name: "Root", dependencies: ["Bar"]), - ], - products: [], - dependencies: [ - TestDependency(name: "Bar", requirement: .upToNextMajor(from: "1.0.0")), - ] - ), - ], - packages: [ - TestPackage( - name: "Bar", - targets: [ - TestTarget(name: "Bar", dependencies: ["Foo"]), - ], - products: [ - TestProduct(name: "Bar", targets: ["Bar"]), - ], - dependencies: [ - TestDependency(name: "Foo", requirement: .upToNextMajor(from: "1.0.0")), - ], - versions: ["1.0.0"] - ), - TestPackage( - name: "Bar", - targets: [ - TestTarget(name: "Bar", dependencies: []), - ], - products: [ - TestProduct(name: "Bar", targets: ["Bar"]), - ], - dependencies: [ - ], - versions: ["1.1.0"] - ), - TestPackage( - name: "Foo", - targets: [ - TestTarget(name: "Foo"), - ], - products: [ - TestProduct(name: "Foo", targets: ["Foo"]), - ], - versions: ["1.0.0"] - ), - ] - ) - - let deps: [TestWorkspace.PackageDependency] = [ - .init(name: "Bar", requirement: .exact("1.0.0")), - ] - workspace.checkPackageGraph(roots: ["Root"], deps: deps) { (graph, diagnostics) in - PackageGraphTester(graph) { result in - result.check(roots: "Root") - result.check(packages: "Bar", "Foo", "Root") - } - XCTAssertNoDiagnostics(diagnostics) - } - workspace.checkManagedDependencies() { result in - result.check(dependency: "foo", at: .checkout(.version("1.0.0"))) - result.check(dependency: "bar", at: .checkout(.version("1.0.0"))) - } - - // Emulate a complete change in Package.resolved file. - do { - let ws = workspace.createWorkspace() - let pinsStore = try ws.pinsStore.load() - let barPin = pinsStore.pins.first(where: { $0.packageRef.identity == "bar" })! - - let barRepo = workspace.repoProvider.specifierMap[RepositorySpecifier(url: barPin.packageRef.path)]! - let revision = try barRepo.resolveRevision(tag: "1.1.0") - let newState = CheckoutState(revision: revision, version: "1.1.0") - - pinsStore.unpinAll() - pinsStore.pin(packageRef: barPin.packageRef, state: newState) - try pinsStore.saveState() - } - - workspace.checkPackageGraph(roots: ["Root"]) { (graph, diagnostics) in - PackageGraphTester(graph) { result in - result.check(roots: "Root") - result.check(packages: "Bar", "Root") - } - XCTAssertNoDiagnostics(diagnostics) - } - workspace.checkManagedDependencies() { result in - result.check(notPresent: "foo") - result.check(dependency: "bar", at: .checkout(.version("1.1.0"))) - } - } - func testRootPackagesOverrideBasenameMismatch() throws { let sandbox = AbsolutePath("/tmp/ws/") let fs = InMemoryFileSystem() diff --git a/Tests/WorkspaceTests/XCTestManifests.swift b/Tests/WorkspaceTests/XCTestManifests.swift index e14e2986b03..b3fd6b635af 100644 --- a/Tests/WorkspaceTests/XCTestManifests.swift +++ b/Tests/WorkspaceTests/XCTestManifests.swift @@ -86,7 +86,6 @@ extension WorkspaceTests { ("testToolsVersionRootPackages", testToolsVersionRootPackages), ("testTransitiveDependencySwitchWithSameIdentity", testTransitiveDependencySwitchWithSameIdentity), ("testUpdate", testUpdate), - ("testUpdateToResolvedFileResolvesCorrectly", testUpdateToResolvedFileResolvesCorrectly), ] }