Fix issue where pkgconfig flags were all deleted if any was not whitelisted

Remove unnecessary commented code

Update comments to match changes to the filter function

Author: PopFlamingo

Sources/PackageLoading/Target+PkgConfig.swift

@@ -38,19 +38,16 @@ public struct PkgConfigResult {
         }
     }
 
-    /// Create a successful result with given cflags and libs.
-    fileprivate init(pkgConfigName: String, cFlags: [String], libs: [String]) {
-        self.pkgConfigName = pkgConfigName
+    /// Create a result.
+    fileprivate init(
+        pkgConfigName: String,
+        cFlags: [String] = [],
+        libs: [String] = [],
+        error: Swift.Error? = nil,
+        provider: SystemPackageProviderDescription? = nil
+    ) {
         self.cFlags = cFlags
         self.libs = libs
-        self.error = nil
-        self.provider = nil
-    }
-
-    /// Create an error result.
-    fileprivate init(pkgConfigName: String, error: Swift.Error, provider: SystemPackageProviderDescription?) {
-        self.cFlags = []
-        self.libs = []
         self.error = error
         self.provider = provider
         self.pkgConfigName = pkgConfigName
@@ -61,21 +58,38 @@ public struct PkgConfigResult {
 public func pkgConfigArgs(for target: SystemLibraryTarget, diagnostics: DiagnosticsEngine, fileSystem: FileSystem = localFileSystem) -> PkgConfigResult? {
     // If there is no pkg config name defined, we're done.
     guard let pkgConfigName = target.pkgConfig else { return nil }
+
     // Compute additional search paths for the provider, if any.
     let provider = target.providers?.first { $0.isAvailable }
     let additionalSearchPaths = provider?.pkgConfigSearchPath() ?? []
+
     // Get the pkg config flags.
     do {
         let pkgConfig = try PkgConfig(
             name: pkgConfigName,
             additionalSearchPaths: additionalSearchPaths,
             diagnostics: diagnostics,
             fileSystem: fileSystem)
+
         // Run the whitelist checker.
-        try whitelist(pcFile: pkgConfigName, flags: (pkgConfig.cFlags, pkgConfig.libs))
+        let filtered = whitelist(pcFile: pkgConfigName, flags: (pkgConfig.cFlags, pkgConfig.libs))
+
         // Remove any default flags which compiler adds automatically.
-        let (cFlags, libs) = removeDefaultFlags(cFlags: pkgConfig.cFlags, libs: pkgConfig.libs)
-        return PkgConfigResult(pkgConfigName: pkgConfigName, cFlags: cFlags, libs: libs)
+        let (cFlags, libs) = removeDefaultFlags(cFlags: filtered.cFlags, libs: filtered.libs)
+
+        // Set the error if there are any unallowed flags.
+        var error: Swift.Error?
+        if !filtered.unallowed.isEmpty {
+            error = PkgConfigError.nonWhitelistedFlags(filtered.unallowed.joined(separator: ", "))
+        }
+
+        return PkgConfigResult(
+            pkgConfigName: pkgConfigName,
+            cFlags: cFlags,
+            libs: libs,
+            error: error,
+            provider: provider
+        )
     } catch {
         return PkgConfigResult(pkgConfigName: pkgConfigName, error: error, provider: provider)
     }
@@ -135,20 +149,24 @@ extension SystemPackageProviderDescription {
 /// compiler/linker. List of allowed flags:
 /// cFlags: -I, -F
 /// libs: -L, -l, -F, -framework, -w
-func whitelist(pcFile: String, flags: (cFlags: [String], libs: [String])) throws {
-    // Returns an array of flags which doesn't match any filter.
-    func filter(flags: [String], filters: [String]) -> [String] {
-        var filtered = [String]()
+func whitelist(
+    pcFile: String,
+    flags: (cFlags: [String], libs: [String])
+) -> (cFlags: [String], libs: [String], unallowed: [String]) {
+    // Returns a tuple with the array of allowed flag and the array of unallowed flags.
+    func filter(flags: [String], filters: [String]) -> (allowed: [String], unallowed: [String]) {
+        var allowed = [String]()
+        var unallowed = [String]()
         var it = flags.makeIterator()
         while let flag = it.next() {
             guard let filter = filters.filter({ flag.hasPrefix($0) }).first else {
-                filtered += [flag]
+                unallowed += [flag]
                 continue
             }
 
           // Warning suppression flag has no arguments and is not suffixed.
           guard !flag.hasPrefix("-w") || flag == "-w" else {
-            filtered += [flag]
+            unallowed += [flag]
             continue
           }
 
@@ -158,14 +176,15 @@ func whitelist(pcFile: String, flags: (cFlags: [String], libs: [String])) throws
                    fatalError("Expected associated value")
                 }
             }
+            allowed += [flag]
         }
-        return filtered
-    }
-    let filtered = filter(flags: flags.cFlags, filters: ["-I", "-F"]) +
-      filter(flags: flags.libs, filters: ["-L", "-l", "-F", "-framework", "-w"])
-    guard filtered.isEmpty else {
-        throw PkgConfigError.nonWhitelistedFlags(filtered.joined(separator: ", "))
+        return (allowed, unallowed)
     }
+
+    let filteredCFlags = filter(flags: flags.cFlags, filters: ["-I", "-F"])
+    let filteredLibs = filter(flags: flags.libs, filters: ["-L", "-l", "-F", "-framework", "-w"])
+
+    return (filteredCFlags.allowed, filteredLibs.allowed, filteredCFlags.unallowed + filteredLibs.unallowed)
 }
 
 /// Remove the default flags which are already added by the compiler.

Tests/PackageLoadingTests/PkgConfigTests.swift

@@ -82,8 +82,8 @@ class PkgConfigTests: XCTestCase {
         try withCustomEnv(["PKG_CONFIG_PATH": inputsDir.asString]) {
             let result = pkgConfigArgs(for: SystemLibraryTarget(pkgConfig: "Bar"), diagnostics: diagnostics)!
             XCTAssertEqual(result.pkgConfigName, "Bar")
-            XCTAssertEqual(result.cFlags, [])
-            XCTAssertEqual(result.libs, [])
+            XCTAssertEqual(result.cFlags, ["-I/path/to/inc"])
+            XCTAssertEqual(result.libs, ["-L/usr/da/lib", "-lSystemModule", "-lok"])
             XCTAssertNil(result.provider)
             XCTAssertFalse(result.couldNotFindConfigFile)
             switch result.error {

Tests/PackageLoadingTests/WhitelistTests.swift

@@ -15,31 +15,21 @@ final class PkgConfigWhitelistTests: XCTestCase {
     func testSimpleFlags() {
         let cFlags = ["-I/usr/local/Cellar/gtk+3/3.18.9/include/gtk-3.0"]
         let libs = ["-L/usr/local/Cellar/gtk+3/3.18.9/lib", "-lgtk-3", "-w"]
-        do {
-            try whitelist(pcFile: "dummy", flags: (cFlags, libs))
-        } catch {
-            XCTFail("Unexpected failure \(error)")
-        }
+        XCTAssertTrue(whitelist(pcFile: "dummy", flags: (cFlags, libs)).unallowed.isEmpty)
     }
 
     func testFlagsWithInvalidFlags() {
         let cFlags = ["-I/usr/local/Cellar/gtk+3/3.18.9/include/gtk-3.0", "-L/hello"]
         let libs = ["-L/usr/local/Cellar/gtk+3/3.18.9/lib", "-lgtk-3", "-module-name", "name", "-werror"]
-        do {
-            try whitelist(pcFile: "dummy", flags: (cFlags, libs))
-        } catch {
-            XCTAssertEqual("\(error)", "non whitelisted flag(s): -L/hello, -module-name, name, -werror")
-        }
+        let unallowed = whitelist(pcFile: "dummy", flags: (cFlags, libs)).unallowed
+        XCTAssertEqual(unallowed, ["-L/hello", "-module-name", "name", "-werror"])
     }
 
     func testFlagsWithValueInNextFlag() {
         let cFlags = ["-I/usr/local", "-I", "/usr/local/Cellar/gtk+3/3.18.9/include/gtk-3.0", "-L/hello"]
         let libs = ["-L", "/usr/lib", "-L/usr/local/Cellar/gtk+3/3.18.9/lib", "-lgtk-3", "-module-name", "-lcool", "ok", "name"]
-        do {
-            try whitelist(pcFile: "dummy", flags: (cFlags, libs))
-        } catch {
-            XCTAssertEqual("\(error)", "non whitelisted flag(s): -L/hello, -module-name, ok, name")
-        }
+        let unallowed = whitelist(pcFile: "dummy", flags: (cFlags, libs)).unallowed
+        XCTAssertEqual(unallowed, ["-L/hello", "-module-name", "ok", "name"])
     }
 
     func testRemoveDefaultFlags() {