Skip to content

Commit

Permalink
[PackageLoading] Add a method to properly reset manifest cache (#2888)
Browse files Browse the repository at this point in the history
Workspace's reset method removes the cache directory while the sqlite
database is in use, which is a considered a bug in client of sqlite
databases. This exposes a proper way to reset a cache that might be used
by a particular manifest loader implementation.

<rdar://problem/67712262>
  • Loading branch information
aciidgh authored Aug 26, 2020
1 parent 5ce8179 commit 3e7ced6
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 6 deletions.
33 changes: 27 additions & 6 deletions Sources/PackageLoading/ManifestLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ public protocol ManifestLoaderProtocol {
fileSystem: FileSystem?,
diagnostics: DiagnosticsEngine?
) throws -> Manifest

/// Reset any internal cache held by the manifest loader.
func resetCache() throws
}

extension ManifestLoaderProtocol {
Expand Down Expand Up @@ -112,6 +115,9 @@ extension ManifestLoaderProtocol {
diagnostics: diagnostics
)
}

public func resetCache() throws {
}
}

public protocol ManifestLoaderDelegate {
Expand All @@ -136,7 +142,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
}
let cacheDir: AbsolutePath!
let delegate: ManifestLoaderDelegate?
let cache: PersistentCacheProtocol?
private(set) var cache: PersistentCacheProtocol?

public init(
manifestResources: ManifestResourceProvider,
Expand All @@ -155,11 +161,6 @@ public final class ManifestLoader: ManifestLoaderProtocol {
try? localFileSystem.createDirectory(cacheDir, recursive: true)
}
self.cacheDir = cacheDir.map(resolveSymlinks)

self.cache = cacheDir.flatMap {
// FIXME: It would be nice to emit a warning if we weren't able to create the cache.
try? SQLiteBackedPersistentCache(cacheFilePath: $0.appending(component: "manifest.db"))
}
}

@available(*, deprecated)
Expand Down Expand Up @@ -237,6 +238,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
fileSystem: FileSystem? = nil,
diagnostics: DiagnosticsEngine? = nil
) throws -> Manifest {
try self.createCacheIfNeeded()

// Inform the delegate.
self.delegate?.willLoad(manifest: inputPath)
Expand Down Expand Up @@ -836,6 +838,25 @@ public final class ManifestLoader: ManifestLoaderProtocol {
// Bin dir will be set when developing swiftpm without building all of the runtimes.
return resources.binDir ?? resources.libDir.appending(version.runtimeSubpath)
}

/// Returns path to the manifest database inside the given cache directory.
private static func manifestCacheDBPath(_ cacheDir: AbsolutePath) -> AbsolutePath {
return cacheDir.appending(component: "manifest.db")
}

func createCacheIfNeeded() throws {
// Return if we have already created the cache.
guard self.cache == nil else { return }
guard let manifestCacheDBPath = cacheDir.flatMap({ Self.manifestCacheDBPath($0) }) else { return }
self.cache = try SQLiteBackedPersistentCache(cacheFilePath: manifestCacheDBPath)
}

public func resetCache() throws {
guard let manifestCacheDBPath = cacheDir.flatMap({ Self.manifestCacheDBPath($0) }) else { return }
self.cache = nil
// Also remove the database file from disk.
try localFileSystem.removeFileTree(manifestCacheDBPath)
}
}

/// Returns the sandbox profile to be used when parsing manifest on macOS.
Expand Down
1 change: 1 addition & 0 deletions Sources/Workspace/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ extension Workspace {
guard removed else { return }

repositoryManager.reset()
try? manifestLoader.resetCache()
try? fileSystem.removeFileTree(dataPath)
}

Expand Down
5 changes: 5 additions & 0 deletions Tests/PackageLoadingTests/PD4_2LoadingTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,11 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
for _ in 0..<2 {
check(loader: noCacheLoader, expectCached: false)
}

// Resetting the cache should allow us to remove the cache
// directory without triggering assertions in sqlite.
try manifestLoader.resetCache()
try localFileSystem.removeFileTree(path)
}
}

Expand Down

0 comments on commit 3e7ced6

Please sign in to comment.