Fix experimental-api-diff command and add a test (#3359)

* Fix experimental-api-diff command

* [APIDiff] Load the current version of the package directly instead of dumping JSON first

Author: owenv

Sources/Build/BuildPlan.swift

@@ -1893,11 +1893,22 @@ public class BuildPlan {
         }
     }
 
-    public func createAPIDigesterArgs() -> [String] {
+    public func createAPIToolCommonArgs(includeLibrarySearchPaths: Bool) -> [String] {
         let buildPath = buildParameters.buildPath.pathString
         var arguments = ["-I", buildPath]
 
-        arguments += buildParameters.toolchain.extraSwiftCFlags
+        var extraSwiftCFlags = buildParameters.toolchain.extraSwiftCFlags
+        if !includeLibrarySearchPaths {
+            for index in extraSwiftCFlags.indices.dropLast().reversed() {
+                if extraSwiftCFlags[index] == "-L" {
+                    // Remove the flag
+                    extraSwiftCFlags.remove(at: index)
+                    // Remove the argument
+                    extraSwiftCFlags.remove(at: index)
+                }
+            }
+        }
+        arguments += extraSwiftCFlags
 
         // Add the search path to the directory containing the modulemap file.
         for target in targets {

Sources/Commands/APIDigester.swift

@@ -124,7 +124,7 @@ struct APIDigesterBaselineDumper {
         try apiDigesterTool.dumpSDKJSON(
             at: sdkJSON,
             modules: graph.apiDigesterModules,
-            additionalArgs: buildOp.buildPlan!.createAPIDigesterArgs()
+            additionalArgs: buildOp.buildPlan!.createAPIToolCommonArgs(includeLibrarySearchPaths: false)
         )
 
         return sdkJSON
@@ -159,16 +159,18 @@ public struct SwiftAPIDigester {
     }
 
     public func diagnoseSDK(
-        currentSDKJSON: AbsolutePath,
-        baselineSDKJSON: AbsolutePath
+        baselineSDKJSON: AbsolutePath,
+        apiToolArgs: [String],
+        modules: [String]
     ) throws {
-        let args = [
+        var args = [
             "-diagnose-sdk",
-            "--input-paths",
-            baselineSDKJSON.pathString,
-            "-input-paths",
-            currentSDKJSON.pathString,
+            "-baseline-path", baselineSDKJSON.pathString,
         ]
+        args.append(contentsOf: apiToolArgs)
+        for module in modules {
+            args.append(contentsOf: ["-module", module])
+        }
 
         try runTool(args)
     }

Sources/Commands/SwiftPackageTool.swift

@@ -294,11 +294,8 @@ extension SwiftPackageTool {
         @OptionGroup()
         var swiftOptions: SwiftToolOptions
 
-        @Argument(help: "The baseline treeish")
+        @Argument(help: "The baseline treeish to compare to (e.g. a commit hash, branch name, tag, etc.)")
         var treeish: String
-        
-        @Flag(help: "Invert the baseline which is helpful for determining API additions")
-        var invertBaseline: Bool = false
 
         func run(_ swiftTool: SwiftTool) throws {
             let apiDigesterPath = try swiftTool.getToolchain().getSwiftAPIDigester()
@@ -310,23 +307,12 @@ extension SwiftPackageTool {
             let buildOp = try swiftTool.createBuildOperation(cacheBuildManifest: false)
             try buildOp.build()
 
-            // Dump JSON for the current package.
-            let buildParameters = buildOp.buildParameters
-            let currentSDKJSON = buildParameters.apiDiff.appending(component: "current.json")
-            let packageGraph = try buildOp.getPackageGraph()
-
-            try apiDigesterTool.dumpSDKJSON(
-                at: currentSDKJSON,
-                modules: packageGraph.apiDigesterModules,
-                additionalArgs: buildOp.buildPlan!.createAPIDigesterArgs()
-            )
-
             // Dump JSON for the baseline package.
             let workspace = try swiftTool.getActiveWorkspace()
             let baselineDumper = try APIDigesterBaselineDumper(
                 baselineTreeish: treeish,
                 packageRoot: swiftTool.getPackageRoot(),
-                buildParameters: buildParameters,
+                buildParameters: buildOp.buildParameters,
                 manifestLoader: workspace.manifestLoader,
                 repositoryManager: workspace.repositoryManager,
                 apiDigesterTool: apiDigesterTool,
@@ -336,8 +322,9 @@ extension SwiftPackageTool {
 
             // Run the diagnose tool which will print the diff.
             try apiDigesterTool.diagnoseSDK(
-                currentSDKJSON: invertBaseline ? baselineSDKJSON : currentSDKJSON,
-                baselineSDKJSON: invertBaseline ? currentSDKJSON : baselineSDKJSON
+                baselineSDKJSON: baselineSDKJSON,
+                apiToolArgs: buildOp.buildPlan!.createAPIToolCommonArgs(includeLibrarySearchPaths: false),
+                modules: try buildOp.getPackageGraph().apiDigesterModules
             )
         }
     }

Sources/Commands/SymbolGraphExtract.swift

@@ -42,8 +42,7 @@ public struct SymbolGraphExtract {
             args += ["-module-name", target.c99name]
             args += buildParameters.targetTripleArgs(for: target)
 
-            // FIXME: We should rename this to common Swift tools args or something.
-            args += buildPlan.createAPIDigesterArgs()
+            args += buildPlan.createAPIToolCommonArgs(includeLibrarySearchPaths: true)
             args += ["-module-cache-path", buildParameters.moduleCache.pathString]
 
             args += ["-output-dir", symbolGraphDirectory.pathString]

Tests/CommandsTests/PackageToolTests.swift

@@ -982,6 +982,25 @@ final class PackageToolTests: XCTestCase {
       #endif
     }
 
+    func testAPIDiff() throws {
+        #if os(macOS)
+        guard (try? Resources.default.toolchain.getSwiftAPIDigester()) != nil else {
+            throw XCTSkip("swift-api-digester not available")
+        }
+        fixture(name: "DependencyResolution/External/Simple") { prefix in
+            let packageRoot = prefix.appending(component: "Foo")
+            // Overwrite the existing decl.
+            try localFileSystem.writeFileContents(packageRoot.appending(component: "Foo.swift")) {
+                $0 <<< "public let foo = 42"
+            }
+            let (_, stderr) = try execute(["experimental-api-diff", "1.2.3"], packagePath: packageRoot)
+            XCTAssertTrue(stderr.contains("Func foo() has been removed"))
+        }
+        #else
+        throw XCTSkip("Test unsupported on current platform")
+        #endif
+    }
+
     func testArchiveSource() throws {
         fixture(name: "DependencyResolution/External/Simple") { prefix in
             let packageRoot = prefix.appending(component: "Bar")