Merge pull request #1189 from hartbit/build-plan-refactor

Refactor SwiftTool building to accept a buildPlan

Author: hartbit

Sources/Commands/Options.swift

@@ -10,10 +10,14 @@
 
 import Basic
 import Utility
+import Build
 
 public class ToolOptions {
     /// Custom arguments to pass to C compiler, swift compiler and the linker.
     public var buildFlags = BuildFlags()
+    
+    /// Build configuration.
+    public var configuration: Build.Configuration = .debug
 
     /// The custom build directory, if provided.
     public var buildPath: AbsolutePath?

Sources/Commands/SwiftBuildTool.swift

@@ -33,10 +33,7 @@ public class SwiftBuildTool: SwiftTool<BuildToolOptions> {
             checkClangVersion()
           #endif
 
-            let graph = try loadPackageGraph()
-            // If we don't have any targets in root package, we're done.
-            guard !graph.rootPackages[0].targets.isEmpty else { break }
-            try build(graph: graph, includingTests: options.buildTests, config: options.config)
+            try build(includingTests: options.buildTests)
 
         case .version:
             print(Versioning.currentVersion.completeDisplayString)
@@ -48,11 +45,6 @@ public class SwiftBuildTool: SwiftTool<BuildToolOptions> {
             option: parser.add(option: "--build-tests", kind: Bool.self,
                 usage: "Build the both source and test targets"),
             to: { $0.buildTests = $1 })
-
-        binder.bind(
-            option: parser.add(option: "--configuration", shortName: "-c", kind: Build.Configuration.self,
-                usage: "Build with configuration (debug|release) [default: debug]"),
-            to: { $0.config = $1 })
     }
 
     private func checkClangVersion() {
@@ -82,9 +74,6 @@ public class BuildToolOptions: ToolOptions {
 
     /// If the test should be built.
     var buildTests = false
-
-    /// Build configuration.
-    var config: Build.Configuration = .debug
 }
 
 public enum BuildToolMode {
@@ -94,10 +83,3 @@ public enum BuildToolMode {
     /// Print the version.
     case version
 }
-
-extension Build.Configuration: StringEnumArgument {
-    public static var completion: ShellCompletion = .values([
-        (debug.rawValue, "build with DEBUG configuration"),
-        (release.rawValue, "build with RELEASE configuration"),
-    ])
-}

Sources/Commands/SwiftTestTool.swift

@@ -31,7 +31,6 @@ struct SpecifierDeprecatedDiagnostic: DiagnosticData {
 
 private enum TestError: Swift.Error {
     case invalidListTestJSONData
-    case multipleTestProducts
     case testsExecutableNotFound
 }
 
@@ -42,8 +41,6 @@ extension TestError: CustomStringConvertible {
             return "no tests found to execute, create a target in your `Tests' directory"
         case .invalidListTestJSONData:
             return "Invalid list test JSON structure."
-        case .multipleTestProducts:
-            return "cannot test packages with multiple test products defined"
         }
     }
 }
@@ -70,9 +67,6 @@ public class TestToolOptions: ToolOptions {
     /// If the test target should be built before testing.
     var shouldBuildTests = true
 
-    /// Build configuration.
-    var config: Build.Configuration = .debug
-
     /// If tests should run in parallel mode.
     var shouldRunInParallel = false
 
@@ -170,40 +164,25 @@ public class SwiftTestTool: SwiftTool<TestToolOptions> {
     ///
     /// - Returns: The path to the test binary.
     private func buildTestsIfNeeded(_ options: TestToolOptions) throws -> AbsolutePath {
-        let graph = try loadPackageGraph()
+        let buildPlan = try self.buildPlan()
         if options.shouldBuildTests {
-            try build(graph: graph, includingTests: true, config: options.config)
+            try build(plan: buildPlan, includingTests: true)
         }
 
-        // See the logic in `PackageLoading`'s `PackageExtensions.swift`.
-        //
-        // FIXME: We should also check if the package has any test
-        // targets, which isn't trivial (yet).
-        let testProducts = graph.products.filter({
-            if case .test = $0.type {
-                return true
-            } else {
-                return false
-            }
-        })
-        if testProducts.count == 0 {
+        guard let testProduct = buildPlan.buildProducts.first(where: { $0.product.type == .test }) else {
             throw TestError.testsExecutableNotFound
-        } else if testProducts.count > 1 {
-            throw TestError.multipleTestProducts
-        } else {
-            return buildPath
-                .appending(RelativePath(options.config.dirname))
-                .appending(component: testProducts[0].name + ".xctest")
         }
+
+        // Go up the folder hierarchy until we find the .xctest bundle.
+        return sequence(first: testProduct.binary, next: {
+            $0.isRoot ? nil : $0.parentDirectory
+        }).first(where: {
+            $0.basename.hasSuffix(".xctest")
+        })!
     }
 
     override class func defineArguments(parser: ArgumentParser, binder: ArgumentBinder<TestToolOptions>) {
 
-        binder.bind(
-            option: parser.add(option: "--configuration", shortName: "-c", kind: Build.Configuration.self,
-                usage: "Build with configuration (debug|release) [default: debug]"),
-            to: { $0.config = $1 })
-
         binder.bind(
             option: parser.add(option: "--skip-build", kind: Bool.self,
                 usage: "Skip building the test target"),

Sources/Commands/SwiftTool.swift

@@ -127,6 +127,12 @@ public class SwiftTool<Options: ToolOptions> {
                 usage: "Pass flag through to all linker invocations"),
             to: { $0.buildFlags = BuildFlags(xcc: $1, xswiftc: $2, xlinker: $3) })
 
+        binder.bind(
+            option: parser.add(
+                option: "--configuration", shortName: "-c", kind: Build.Configuration.self,
+                usage: "Build with configuration (debug|release) [default: debug]"),
+            to: { $0.configuration = $1 })
+        
         binder.bind(
             option: parser.add(
                 option: "--build-path", kind: PathArgument.self,
@@ -318,19 +324,20 @@ public class SwiftTool<Options: ToolOptions> {
     }
 
     /// Build the package graph using swift-build-tool.
-    func build(graph: PackageGraph, includingTests: Bool, config: Build.Configuration) throws {
-        // Create build parameters.
-        let buildParameters = BuildParameters(
-            dataPath: buildPath,
-            configuration: config,
-            toolchain: try getToolchain(),
-            flags: options.buildFlags
-        )
-        let yaml = buildPath.appending(component: config.dirname + ".yaml")
-        // Create build plan.
-        let buildPlan = try BuildPlan(buildParameters: buildParameters, graph: graph, delegate: self)
+    func build(includingTests: Bool) throws {
+        try build(plan: buildPlan(), includingTests: includingTests)
+    }
+    
+    /// Build the package graph using swift-build-tool.
+    func build(plan: BuildPlan, includingTests: Bool) throws {
+        guard !plan.graph.rootPackages[0].targets.isEmpty else {
+            warning(message: "no targets to build in package")
+            return
+        }
+
+        let yaml = buildPath.appending(component: plan.buildParameters.configuration.dirname + ".yaml")
         // Generate llbuild manifest.
-        let llbuild = LLbuildManifestGenerator(buildPlan)
+        let llbuild = LLbuildManifestGenerator(plan)
         try llbuild.generateManifest(at: yaml)
         assert(isFile(yaml), "llbuild manifest not present: \(yaml.asString)")
 
@@ -366,6 +373,18 @@ public class SwiftTool<Options: ToolOptions> {
         }
     }
 
+    /// Generates a BuildPlan based on the tool's options.
+    func buildPlan() throws -> BuildPlan {
+        return try BuildPlan(
+            buildParameters: BuildParameters(
+                dataPath: buildPath,
+                configuration: options.configuration,
+                toolchain: try getToolchain(),
+                flags: options.buildFlags),
+            graph: try loadPackageGraph(),
+            delegate: self)
+    }
+
     /// Lazily compute the destination toolchain.
     private lazy var _destinationToolchain: Result<UserToolchain, AnyError> = {
         // Create custom toolchain if present.
@@ -456,3 +475,10 @@ private func sandboxProfile(allowedDirectories: [AbsolutePath]) -> String {
     stream <<< ")" <<< "\n"
     return stream.bytes.asString!
 }
+
+extension Build.Configuration: StringEnumArgument {
+    public static var completion: ShellCompletion = .values([
+        (debug.rawValue, "build with DEBUG configuration"),
+        (release.rawValue, "build with RELEASE configuration"),
+    ])
+}