initialization warnings

tomerd · tomerd · commit 6f52ce90652f · 2022-01-26T15:39:06.000-08:00
diff --git a/Sources/Basics/FileSystem+Extensions.swift b/Sources/Basics/FileSystem+Extensions.swift
@@ -89,7 +89,7 @@ extension FileSystem {
 }
 
 extension FileSystem {
-    public func getOrCreateSwiftPMConfigurationDirectory() throws -> AbsolutePath {
+    public func getOrCreateSwiftPMConfigurationDirectory(warningHandler: (String) -> Void) throws -> AbsolutePath {
         let idiomaticConfigurationDirectory = self.swiftPMConfigurationDirectory
 
         // temporary 5.6, remove on next version: transition from previous configuration location
@@ -111,9 +111,7 @@ extension FileSystem {
                     if !self.exists(destination) {
                         try self.copy(from: file, to: destination)
                     }
-                    // FIXME: We should emit a warning here using the diagnostic engine.
-                    TSCBasic.stderrStream.write("warning: Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.\n")
-                    TSCBasic.stderrStream.flush()
+                    warningHandler("Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.")
                 }
             }
         // in the case where ~/.swiftpm/configuration is the idiomatic location (eg on Linux)
@@ -130,9 +128,7 @@ extension FileSystem {
                     if !self.exists(destination) {
                         try self.copy(from: file, to: destination)
                     }
-                    // FIXME: We should emit a warning here using the diagnostic engine.
-                    TSCBasic.stderrStream.write("warning: Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.\n")
-                    TSCBasic.stderrStream.flush()
+                    warningHandler("Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.")
                 }
             }
         }
diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift
@@ -515,6 +515,7 @@ public class SwiftTool {
                 sharedRepositoriesCacheEnabled: self.options.useRepositoriesCache,
                 fingerprintCheckingMode: self.options.resolverFingerprintCheckingMode
             ),
+            initializationWarningHandler: { self.observabilityScope.emit(warning: $0) },
             customManifestLoader: self.getManifestLoader(), // FIXME: ideally we would not customize the manifest loader
             customRepositoryProvider: repositoryProvider, // FIXME: ideally we would not customize the repository provider. its currently done for shutdown handling which can be better abstracted
             delegate: delegate
diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift
@@ -72,18 +72,22 @@ public class RepositoryManager {
     /// Create a new empty manager.
     ///
     /// - Parameters:
+    ///   - fileSystem: The filesystem to operate on.
     ///   - path: The path under which to store repositories. This should be a
     ///           directory in which the content can be completely managed by this
     ///           instance.
     ///   - provider: The repository provider.
+    ///   - cachePath: The repository cache location.
+    ///   - cacheLocalPackages: Should cache local packages as well. For testing purposes.
+    ///   - initializationWarningHandler: Initialization warnings handler.
     ///   - delegate: The repository manager delegate.
-    ///   - fileSystem: The filesystem to operate on.
     public init(
         fileSystem: FileSystem,
         path: AbsolutePath,
         provider: RepositoryProvider,
         cachePath: AbsolutePath? =  .none,
         cacheLocalPackages: Bool = false,
+        initializationWarningHandler: (String) -> Void,
         delegate: RepositoryManagerDelegate? = .none
     ) {
         self.fileSystem = fileSystem
@@ -107,9 +111,7 @@ public class RepositoryManager {
         } catch {
             self.repositories = [:]
             try? self.storage.reset()
-            // FIXME: We should emit a warning here using the diagnostic engine.
-            TSCBasic.stderrStream.write("warning: unable to restore checkouts state: \(error)\n")
-            TSCBasic.stderrStream.flush()
+            initializationWarningHandler("unable to restore checkouts state: \(error)")
         }
     }
 
diff --git a/Sources/Workspace/Workspace.swift b/Sources/Workspace/Workspace.swift
@@ -254,6 +254,7 @@ public class Workspace {
     ///   - location: Workspace location configuration.
     ///   - authorizationProvider: Provider of authentication information for outbound network requests.
     ///   - configuration: Configuration to fine tune the dependency resolution behavior.
+    ///   - initializationWarningHandler: Initialization warnings handler
     ///   - customManifestLoader: Custom manifest loader. Used to customize how manifest are loaded.
     ///   - customPackageContainerProvider: Custom package container provider. Used to provide specialized package providers.
     ///   - customRepositoryProvider: Custom repository provider. Used to customize source control access.
@@ -263,6 +264,7 @@ public class Workspace {
         location: Location,
         authorizationProvider: AuthorizationProvider? = .none,
         configuration: WorkspaceConfiguration? = .none,
+        initializationWarningHandler: ((String) -> Void)? = .none,
         // optional customization used for advanced integration situations
         customManifestLoader: ManifestLoaderProtocol? = .none,
         customPackageContainerProvider: PackageContainerProvider? = .none,
@@ -275,6 +277,7 @@ public class Workspace {
             location: location,
             authorizationProvider: authorizationProvider,
             configuration: configuration,
+            initializationWarningHandler: initializationWarningHandler,
             customRegistriesConfiguration: .none,
             customFingerprints: .none,
             customMirrors: .none,
@@ -303,6 +306,7 @@ public class Workspace {
     ///   - forRootPackage: The path for the root package.
     ///   - authorizationProvider: Provider of authentication information for outbound network requests.
     ///   - configuration: Configuration to fine tune the dependency resolution behavior.
+    ///   - initializationWarningHandler: Initialization warnings handler
     ///   - customManifestLoader: Custom manifest loader. Used to customize how manifest are loaded.
     ///   - customPackageContainerProvider: Custom package container provider. Used to provide specialized package providers.
     ///   - customRepositoryProvider: Custom repository provider. Used to customize source control access.
@@ -312,6 +316,7 @@ public class Workspace {
         forRootPackage packagePath: AbsolutePath,
         authorizationProvider: AuthorizationProvider? = .none,
         configuration: WorkspaceConfiguration? = .none,
+        initializationWarningHandler: ((String) -> Void)? = .none,
         // optional customization used for advanced integration situations
         customManifestLoader: ManifestLoaderProtocol? = .none,
         customPackageContainerProvider: PackageContainerProvider? = .none,
@@ -324,6 +329,7 @@ public class Workspace {
         try self.init(
             fileSystem: fileSystem,
             location: location,
+            initializationWarningHandler: initializationWarningHandler,
             customManifestLoader: customManifestLoader,
             customPackageContainerProvider: customPackageContainerProvider,
             customRepositoryProvider: customRepositoryProvider,
@@ -342,6 +348,7 @@ public class Workspace {
     ///   - forRootPackage: The path for the root package.
     ///   - authorizationProvider: Provider of authentication information for outbound network requests.
     ///   - configuration: Configuration to fine tune the dependency resolution behavior.
+    ///   - initializationWarningHandler: Initialization warnings handler
     ///   - customToolchain: Custom toolchain. Used to create a customized ManifestLoader, customizing how manifest are loaded.
     ///   - customPackageContainerProvider: Custom package container provider. Used to provide specialized package providers.
     ///   - customRepositoryProvider: Custom repository provider. Used to customize source control access.
@@ -351,6 +358,7 @@ public class Workspace {
         forRootPackage packagePath: AbsolutePath,
         authorizationProvider: AuthorizationProvider? = .none,
         configuration: WorkspaceConfiguration? = .none,
+        initializationWarningHandler: ((String) -> Void)? = .none,
         // optional customization used for advanced integration situations
         customToolchain: UserToolchain,
         customPackageContainerProvider: PackageContainerProvider? = .none,
@@ -369,15 +377,14 @@ public class Workspace {
             forRootPackage: packagePath,
             authorizationProvider: authorizationProvider,
             configuration: configuration,
+            initializationWarningHandler: initializationWarningHandler,
             customManifestLoader: manifestLoader,
             customPackageContainerProvider: customPackageContainerProvider,
             customRepositoryProvider: customRepositoryProvider,
             delegate: delegate
         )
     }
 
-
-
     // deprecate 12/21
     @_disfavoredOverload
     @available(*, deprecated, message: "use alternative initializer")
@@ -418,6 +425,7 @@ public class Workspace {
             location: location,
             authorizationProvider: authorizationProvider,
             configuration: configuration,
+            initializationWarningHandler: .none,
             customRegistriesConfiguration: registries,
             customFingerprints: customFingerprintStorage,
             customMirrors: mirrors,
@@ -533,6 +541,7 @@ public class Workspace {
         location: Location,
         authorizationProvider: AuthorizationProvider? = .none,
         configuration: WorkspaceConfiguration? = .none,
+        initializationWarningHandler: ((String) -> Void)? = .none,
         // optional customization, primarily designed for testing but also used in some cases by libSwiftPM consumers
         customRegistriesConfiguration: RegistryConfiguration? = .none,
         customFingerprints: PackageFingerprintStorage? = .none,
@@ -555,6 +564,7 @@ public class Workspace {
             location: location,
             authorizationProvider: authorizationProvider,
             configuration: configuration,
+            initializationWarningHandler: initializationWarningHandler,
             customRegistriesConfiguration: customRegistriesConfiguration,
             customFingerprints: customFingerprints,
             customMirrors: customMirrors,
@@ -578,6 +588,7 @@ public class Workspace {
         location: Location,
         authorizationProvider: AuthorizationProvider?,
         configuration: WorkspaceConfiguration?,
+        initializationWarningHandler: ((String) -> Void)?,
         // optional customization, primarily designed for testing but also used in some cases by libSwiftPM consumers
         customRegistriesConfiguration: RegistryConfiguration?,
         customFingerprints: PackageFingerprintStorage?,
@@ -595,11 +606,11 @@ public class Workspace {
         // delegate
         delegate: WorkspaceDelegate?
     ) throws {
-        // we do not store the observabilityScope in the workspace initializer as the workspace is designed to be long lived.
+        // we do not store an observabilityScope in the workspace initializer as the workspace is designed to be long lived.
         // instead, observabilityScope is passed into the individual workspace methods which are short lived.
-
+        let initializationWarningHandler = initializationWarningHandler ?? warnToStderr
         // validate locations, returning a potentially modified one to deal with non-accessible or non-writable shared locations
-        let location = try location.validatingSharedLocations(fileSystem: fileSystem)
+        let location = try location.validatingSharedLocations(fileSystem: fileSystem, warningHandler: initializationWarningHandler)
 
         let currentToolsVersion = customToolsVersion ?? ToolsVersion.currentToolsVersion
         let toolsVersionLoader = ToolsVersionLoader(currentToolsVersion: currentToolsVersion)
@@ -623,6 +634,7 @@ public class Workspace {
             path: location.repositoriesDirectory,
             provider: repositoryProvider,
             cachePath: configuration.sharedRepositoriesCacheEnabled ? location.sharedRepositoriesCacheDirectory : .none,
+            initializationWarningHandler: initializationWarningHandler,
             delegate: delegate.map(WorkspaceRepositoryManagerDelegate.init(workspaceDelegate:))
         )
 
@@ -680,7 +692,11 @@ public class Workspace {
 
         self.configuration = configuration
 
-        self.state = WorkspaceState(fileSystem: fileSystem, storageDirectory: self.location.workingDirectory)
+        self.state = WorkspaceState(
+            fileSystem: fileSystem,
+            storageDirectory: self.location.workingDirectory,
+            initializationWarningHandler: initializationWarningHandler
+        )
     }
 }
 
@@ -4014,16 +4030,16 @@ extension FileSystem {
 }
 
 extension Workspace.Location {
-    func validatingSharedLocations(fileSystem: FileSystem) throws -> Self {
+    func validatingSharedLocations(
+        fileSystem: FileSystem,
+        warningHandler: (String) -> Void
+    ) throws -> Self {
         var location = self
 
-        // local configuration directory must be accessible, throw if we cannot access it
-        //try fileSystem.withLock(on: location.localConfigurationDirectory, type: .exclusive, {})
-
         // check that shared configuration directory is accessible, or warn + reset if not
         if let sharedConfigurationDirectory = self.sharedConfigurationDirectory {
             // it may not always be possible to create default location (for example de to restricted sandbox)
-            let defaultDirectory = try? fileSystem.getOrCreateSwiftPMConfigurationDirectory()
+            let defaultDirectory = try? fileSystem.getOrCreateSwiftPMConfigurationDirectory(warningHandler: warningHandler)
             if sharedConfigurationDirectory != defaultDirectory {
                 // custom location must be writable, throw if we cannot access it
                 try withTemporaryFile(dir: sharedConfigurationDirectory) { _ in }
@@ -4033,9 +4049,7 @@ extension Workspace.Location {
                     try withTemporaryFile(dir: sharedConfigurationDirectory) { _ in }
                 } catch {
                     location.sharedConfigurationDirectory = .none
-                    // FIXME: We should emit a warning here using the diagnostic engine.
-                    TSCBasic.stderrStream.write("warning: \(sharedConfigurationDirectory) is not accessible or not writable, disabling user-level configuration features. \(error)\n")
-                    TSCBasic.stderrStream.flush()
+                    warningHandler("\(sharedConfigurationDirectory) is not accessible or not writable, disabling user-level configuration features. \(error)")
                 }
             }
         }
@@ -4053,9 +4067,7 @@ extension Workspace.Location {
                     try withTemporaryFile(dir: sharedSecurityDirectory) { _ in }
                 } catch {
                     location.sharedSecurityDirectory = .none
-                    // FIXME: We should emit a warning here using the diagnostic engine.
-                    TSCBasic.stderrStream.write("warning: \(sharedSecurityDirectory) is not accessible or not writable, disabling user-level security features. \(error)\n")
-                    TSCBasic.stderrStream.flush()
+                    warningHandler("\(sharedSecurityDirectory) is not accessible or not writable, disabling user-level security features. \(error)")
                 }
             }
         }
@@ -4073,12 +4085,15 @@ extension Workspace.Location {
                     try withTemporaryFile(dir: sharedCacheDirectory) { _ in }
                 } catch {
                     location.sharedCacheDirectory = .none
-                    // FIXME: We should emit a warning here using the diagnostic engine.
-                    TSCBasic.stderrStream.write("warning: \(sharedCacheDirectory) is not accessible or not writable, disabling user-level cache features. \(error)\n")
-                    TSCBasic.stderrStream.flush()
+                    warningHandler("\(sharedCacheDirectory) is not accessible or not writable, disabling user-level cache features. \(error)")
                 }
             }
         }
         return location
     }
 }
+
+fileprivate func warnToStderr(_ message: String) {
+    TSCBasic.stderrStream.write("warning: \(message)\n")
+    TSCBasic.stderrStream.flush()
+}
diff --git a/Sources/Workspace/WorkspaceState.swift b/Sources/Workspace/WorkspaceState.swift
@@ -29,7 +29,11 @@ public final class WorkspaceState {
     /// storage
     private let storage: WorkspaceStateStorage
 
-    init(fileSystem: FileSystem, storageDirectory: AbsolutePath) {
+    init(
+        fileSystem: FileSystem,
+        storageDirectory: AbsolutePath,
+        initializationWarningHandler: (String) -> Void
+    ) {
         self.storagePath = storageDirectory.appending(component: "workspace-state.json")
         self.storage = WorkspaceStateStorage(path: self.storagePath, fileSystem: fileSystem)
 
@@ -49,9 +53,7 @@ public final class WorkspaceState {
             self.dependencies = Workspace.ManagedDependencies()
             self.artifacts = Workspace.ManagedArtifacts()
             try? self.storage.reset()
-            // FIXME: We should emit a warning here using the diagnostic engine.
-            TSCBasic.stderrStream.write("warning: unable to restore workspace state: \(error)\n")
-            TSCBasic.stderrStream.flush()
+            initializationWarningHandler("unable to restore workspace state: \(error)")
         }
     }
 
diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift
@@ -705,6 +705,25 @@ class RepositoryManagerTests: XCTestCase {
 }
 
 extension RepositoryManager {
+    public convenience init(
+        fileSystem: FileSystem,
+        path: AbsolutePath,
+        provider: RepositoryProvider,
+        cachePath: AbsolutePath? =  .none,
+        cacheLocalPackages: Bool = false,
+        delegate: RepositoryManagerDelegate? = .none
+    ) {
+        self.init(
+            fileSystem: fileSystem,
+            path: path,
+            provider: provider,
+            cachePath: cachePath,
+            cacheLocalPackages: cacheLocalPackages,
+            initializationWarningHandler: { _ in },
+            delegate: delegate
+        )
+    }
+
     fileprivate func lookup(repository: RepositorySpecifier, skipUpdate: Bool = false) throws -> RepositoryHandle {
         return try tsc_await { self.lookup(repository: repository, skipUpdate: skipUpdate, on: .global(), completion: $0) }
     }
diff --git a/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift b/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift
@@ -673,3 +673,24 @@ extension PackageContainerProvider {
         try tsc_await { self.getContainer(for: package, skipUpdate: skipUpdate, observabilityScope: ObservabilitySystem.NOOP, on: .global(), completion: $0)  }
     }
 }
+
+extension RepositoryManager {
+    fileprivate convenience init(
+        fileSystem: FileSystem,
+        path: AbsolutePath,
+        provider: RepositoryProvider,
+        cachePath: AbsolutePath? =  .none,
+        cacheLocalPackages: Bool = false,
+        delegate: RepositoryManagerDelegate? = .none
+    ) {
+        self.init(
+            fileSystem: fileSystem,
+            path: path,
+            provider: provider,
+            cachePath: cachePath,
+            cacheLocalPackages: cacheLocalPackages,
+            initializationWarningHandler: { _ in },
+            delegate: delegate
+        )
+    }
+}
diff --git a/Tests/WorkspaceTests/WorkspaceStateTests.swift b/Tests/WorkspaceTests/WorkspaceStateTests.swift
@@ -490,3 +490,9 @@ final class WorkspaceStateTests: XCTestCase {
         XCTAssertTrue(state.artifacts.isEmpty)
     }
 }
+
+extension WorkspaceState {
+    fileprivate convenience init(fileSystem: FileSystem, storageDirectory: AbsolutePath) {
+        self.init(fileSystem: fileSystem, storageDirectory: storageDirectory, initializationWarningHandler: { _ in })
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ extension FileSystem {`
`89`	`89`	`}`
`90`	`90`
`91`	`91`	`extension FileSystem {`
`92`		`- public func getOrCreateSwiftPMConfigurationDirectory() throws -> AbsolutePath {`
	`92`	`+ public func getOrCreateSwiftPMConfigurationDirectory(warningHandler: (String) -> Void) throws -> AbsolutePath {`
`93`	`93`	`let idiomaticConfigurationDirectory = self.swiftPMConfigurationDirectory`
`94`	`94`
`95`	`95`	`// temporary 5.6, remove on next version: transition from previous configuration location`
`@@ -111,9 +111,7 @@ extension FileSystem {`
`111`	`111`	`if !self.exists(destination) {`
`112`	`112`	`try self.copy(from: file, to: destination)`
`113`	`113`	`}`
`114`		`- // FIXME: We should emit a warning here using the diagnostic engine.`
`115`		`- TSCBasic.stderrStream.write("warning: Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.\n")`
`116`		`- TSCBasic.stderrStream.flush()`
	`114`	`+ warningHandler("Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.")`
`117`	`115`	`}`
`118`	`116`	`}`
`119`	`117`	`// in the case where ~/.swiftpm/configuration is the idiomatic location (eg on Linux)`
`@@ -130,9 +128,7 @@ extension FileSystem {`
`130`	`128`	`if !self.exists(destination) {`
`131`	`129`	`try self.copy(from: file, to: destination)`
`132`	`130`	`}`
`133`		`- // FIXME: We should emit a warning here using the diagnostic engine.`
`134`		`- TSCBasic.stderrStream.write("warning: Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.\n")`
`135`		`- TSCBasic.stderrStream.flush()`
	`131`	`+ warningHandler("Usage of \(file) has been deprecated. Please delete it and use the new \(destination) instead.")`
`136`	`132`	`}`
`137`	`133`	`}`
`138`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -490,3 +490,9 @@ final class WorkspaceStateTests: XCTestCase {`
`490`	`490`	`XCTAssertTrue(state.artifacts.isEmpty)`
`491`	`491`	`}`
`492`	`492`	`}`
	`493`	`+`
	`494`	`+extension WorkspaceState {`
	`495`	`+ fileprivate convenience init(fileSystem: FileSystem, storageDirectory: AbsolutePath) {`
	`496`	`+ self.init(fileSystem: fileSystem, storageDirectory: storageDirectory, initializationWarningHandler: { _ in })`
	`497`	`+ }`
	`498`	`+}`