Plugin: Stop building with -enable-testing even with release config

kateinoigakukun · kateinoigakukun · commit bede6186db29 · 2024-10-05T23:53:00.000+09:00
diff --git a/Sources/Build/BuildDescription/ProductBuildDescription.swift b/Sources/Build/BuildDescription/ProductBuildDescription.swift
@@ -205,7 +205,7 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
             return []
         case .test:
             // Test products are bundle when using Objective-C, executable when using test entry point.
-            switch self.buildParameters.testingParameters.testProductStyle {
+            switch self.buildParameters.testProductStyle {
             case .loadableBundle:
                 args += ["-Xlinker", "-bundle"]
             case .entryPointExecutable:
diff --git a/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift b/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift
@@ -955,7 +955,7 @@ public final class SwiftModuleBuildDescription {
             // test targets must be built with -enable-testing
             // since its required for test discovery (the non objective-c reflection kind)
             return ["-enable-testing"]
-        } else if self.buildParameters.testingParameters.enableTestability {
+        } else if self.buildParameters.enableTestability {
             return ["-enable-testing"]
         } else {
             return []
diff --git a/Sources/Build/BuildPlan/BuildPlan+Test.swift b/Sources/Build/BuildPlan/BuildPlan+Test.swift
@@ -43,7 +43,7 @@ extension BuildPlan {
     ) throws -> [(product: ResolvedProduct, discoveryTargetBuildDescription: SwiftModuleBuildDescription?, entryPointTargetBuildDescription: SwiftModuleBuildDescription)] {
         var explicitlyEnabledDiscovery = false
         var explicitlySpecifiedPath: AbsolutePath?
-        if case let .entryPointExecutable(caseExplicitlyEnabledDiscovery, caseExplicitlySpecifiedPath) = destinationBuildParameters.testingParameters.testProductStyle {
+        if case let .entryPointExecutable(caseExplicitlyEnabledDiscovery, caseExplicitlySpecifiedPath) = destinationBuildParameters.testProductStyle {
             explicitlyEnabledDiscovery = caseExplicitlyEnabledDiscovery
             explicitlySpecifiedPath = caseExplicitlySpecifiedPath
         }
diff --git a/Sources/Build/LLBuildCommands.swift b/Sources/Build/LLBuildCommands.swift
@@ -103,7 +103,7 @@ final class TestDiscoveryCommand: CustomLLBuildCommand, TestBuildCommand {
     private func execute(fileSystem: Basics.FileSystem, tool: TestDiscoveryTool) throws {
         let outputs = tool.outputs.compactMap { try? AbsolutePath(validating: $0.name) }
 
-        if case .loadableBundle = context.productsBuildParameters.testingParameters.testProductStyle {
+        if case .loadableBundle = context.productsBuildParameters.testProductStyle {
             // When building an XCTest bundle, test discovery is handled by the
             // test harness process (i.e. this is the Darwin path.)
             for file in outputs {
@@ -222,7 +222,7 @@ final class TestEntryPointCommand: CustomLLBuildCommand, TestBuildCommand {
             testObservabilitySetup = ""
         }
 
-        let isXCTMainAvailable: String = switch buildParameters.testingParameters.testProductStyle {
+        let isXCTMainAvailable: String = switch buildParameters.testProductStyle {
         case .entryPointExecutable:
             "canImport(XCTest)"
         case .loadableBundle:
diff --git a/Sources/Commands/Utilities/PluginDelegate.swift b/Sources/Commands/Utilities/PluginDelegate.swift
@@ -227,7 +227,7 @@ final class PluginDelegate: PluginInvocationDelegate {
         // which ones they are until we've built them and can examine the binaries.
         let toolchain = try swiftCommandState.getHostToolchain()
         var toolsBuildParameters = try swiftCommandState.toolsBuildParameters
-        toolsBuildParameters.testingParameters.enableTestability = true
+        toolsBuildParameters.testingParameters.explicitlyEnabledTestability = true
         toolsBuildParameters.testingParameters.enableCodeCoverage = parameters.enableCodeCoverage
         let buildSystem = try await swiftCommandState.createBuildSystem(
             traitConfiguration: .init(),
diff --git a/Sources/Commands/Utilities/TestingSupport.swift b/Sources/Commands/Utilities/TestingSupport.swift
@@ -250,27 +250,10 @@ extension SwiftCommandState {
         experimentalTestOutput: Bool
     ) -> BuildParameters {
         var parameters = parameters
-
-        var explicitlyEnabledDiscovery = false
-        var explicitlySpecifiedPath: AbsolutePath?
-        if case let .entryPointExecutable(
-            explicitlyEnabledDiscoveryValue,
-            explicitlySpecifiedPathValue
-        ) = parameters.testingParameters.testProductStyle {
-            explicitlyEnabledDiscovery = explicitlyEnabledDiscoveryValue
-            explicitlySpecifiedPath = explicitlySpecifiedPathValue
-        }
-        parameters.testingParameters = .init(
-            configuration: parameters.configuration,
-            targetTriple: parameters.triple,
-            forceTestDiscovery: explicitlyEnabledDiscovery,
-            testEntryPointPath: explicitlySpecifiedPath
-        )
-
         parameters.testingParameters.enableCodeCoverage = enableCodeCoverage
         // for test commands, we normally enable building with testability
         // but we let users override this with a flag
-        parameters.testingParameters.enableTestability = enableTestability ?? true
+        parameters.testingParameters.explicitlyEnabledTestability = enableTestability ?? true
         parameters.shouldSkipBuilding = shouldSkipBuilding
         parameters.testingParameters.experimentalTestOutput = experimentalTestOutput
         return parameters
diff --git a/Sources/CoreCommands/BuildSystemSupport.swift b/Sources/CoreCommands/BuildSystemSupport.swift
@@ -36,7 +36,7 @@ private struct NativeBuildSystemFactory: BuildSystemFactory {
         observabilityScope: ObservabilityScope?
     ) async throws -> any BuildSystem {
         _ = try await swiftCommandState.getRootPackageInformation()
-        let testEntryPointPath = productsBuildParameters?.testingParameters.testProductStyle.explicitlySpecifiedEntryPointPath
+        let testEntryPointPath = productsBuildParameters?.testProductStyle.explicitlySpecifiedEntryPointPath
         return try BuildOperation(
             productsBuildParameters: try productsBuildParameters ?? self.swiftCommandState.productsBuildParameters,
             toolsBuildParameters: try toolsBuildParameters ?? self.swiftCommandState.toolsBuildParameters,
diff --git a/Sources/CoreCommands/SwiftCommandState.swift b/Sources/CoreCommands/SwiftCommandState.swift
@@ -826,8 +826,6 @@ public final class SwiftCommandState {
                 isVerbose: self.logLevel <= .info
             ),
             testingParameters: .init(
-                configuration: options.build.configuration ?? self.preferredBuildConfiguration,
-                targetTriple: triple,
                 forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery
                 testEntryPointPath: options.build.testEntryPointPath
             )
diff --git a/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift b/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift
@@ -77,18 +77,20 @@ extension BuildParameters {
         /// Whether to enable code coverage.
         public var enableCodeCoverage: Bool
 
-        /// Whether building for testability is enabled.
-        public var enableTestability: Bool
+        /// Whether building for testability is explicitly enabled or disabled.
+        package var explicitlyEnabledTestability: Bool?
 
         /// Whether or not to enable the experimental test output mode.
         public var experimentalTestOutput: Bool
 
-        /// The style of test product to produce.
-        public var testProductStyle: TestProductStyle
+        /// Whether to force test discovery.
+        fileprivate var explicitlyEnabledDiscovery: Bool
+
+        /// The path to the test entry point file, if one was specified explicitly
+        /// via `--experimental-test-entry-point-path <file>`.
+        fileprivate var explicitlySpecifiedPath: AbsolutePath?
 
         public init(
-            configuration: BuildConfiguration,
-            targetTriple: Triple,
             enableCodeCoverage: Bool = false,
             enableTestability: Bool? = nil,
             experimentalTestOutput: Bool = false,
@@ -97,18 +99,29 @@ extension BuildParameters {
         ) {
             self.enableCodeCoverage = enableCodeCoverage
             self.experimentalTestOutput = experimentalTestOutput
-            // decide on testability based on debug/release config
-            // the goals of this being based on the build configuration is
-            // that `swift build` followed by a `swift test` will need to do minimal rebuilding
-            // given that the default configuration for `swift build` is debug
-            // and that `swift test` normally requires building with testable enabled.
-            // when building and testing in release mode, one can use the '--disable-testable-imports' flag
-            // to disable testability in `swift test`, but that requires that the tests do not use the testable imports feature
-            self.enableTestability =  enableTestability ?? (.debug == configuration)
-            self.testProductStyle = targetTriple.isDarwin() ? .loadableBundle : .entryPointExecutable(
-                explicitlyEnabledDiscovery: forceTestDiscovery,
-                explicitlySpecifiedPath: testEntryPointPath
-            )
+            self.explicitlyEnabledTestability = enableTestability
+            self.explicitlyEnabledDiscovery = forceTestDiscovery
+            self.explicitlySpecifiedPath = testEntryPointPath
         }
     }
+
+    /// Whether building for testability is enabled.
+    public var enableTestability: Bool {
+        // decide on testability based on debug/release config
+        // the goals of this being based on the build configuration is
+        // that `swift build` followed by a `swift test` will need to do minimal rebuilding
+        // given that the default configuration for `swift build` is debug
+        // and that `swift test` normally requires building with testable enabled.
+        // when building and testing in release mode, one can use the '--disable-testable-imports' flag
+        // to disable testability in `swift test`, but that requires that the tests do not use the testable imports feature
+        self.testingParameters.explicitlyEnabledTestability ?? (self.configuration == .debug)
+    }
+
+    /// The style of test product to produce.
+    public var testProductStyle: TestProductStyle {
+        return triple.isDarwin() ? .loadableBundle : .entryPointExecutable(
+            explicitlyEnabledDiscovery: testingParameters.explicitlyEnabledDiscovery,
+            explicitlySpecifiedPath: testingParameters.explicitlySpecifiedPath
+        )
+    }
 }
diff --git a/Sources/SPMBuildCore/BuildParameters/BuildParameters.swift b/Sources/SPMBuildCore/BuildParameters/BuildParameters.swift
@@ -163,7 +163,7 @@ public struct BuildParameters: Encodable {
         driverParameters: Driver = .init(),
         linkingParameters: Linking = .init(),
         outputParameters: Output = .init(),
-        testingParameters: Testing? = nil
+        testingParameters: Testing = .init()
     ) throws {
         let triple = try triple ?? .getHostTriple(usingSwiftCompiler: toolchain.swiftCompilerPath)
         self.debuggingParameters = debuggingParameters ?? .init(
@@ -218,7 +218,7 @@ public struct BuildParameters: Encodable {
         self.driverParameters = driverParameters
         self.linkingParameters = linkingParameters
         self.outputParameters = outputParameters
-        self.testingParameters = testingParameters ?? .init(configuration: configuration, targetTriple: triple)
+        self.testingParameters = testingParameters
     }
 
     /// The path to the build directory (inside the data directory).
diff --git a/Sources/XCBuildSupport/XcodeBuildSystem.swift b/Sources/XCBuildSupport/XcodeBuildSystem.swift
@@ -371,7 +371,7 @@ extension PIFBuilderParameters {
         self.init(
             triple: buildParameters.triple,
             isPackageAccessModifierSupported: buildParameters.driverParameters.isPackageAccessModifierSupported,
-            enableTestability: buildParameters.testingParameters.enableTestability,
+            enableTestability: buildParameters.enableTestability,
             shouldCreateDylibForDynamicProducts: buildParameters.shouldCreateDylibForDynamicProducts,
             toolchainLibDir: (try? buildParameters.toolchain.toolchainLibDir) ?? .root,
             pkgConfigDirectories: buildParameters.pkgConfigDirectories,
diff --git a/Tests/SPMBuildCoreTests/BuildParametersTests.swift b/Tests/SPMBuildCoreTests/BuildParametersTests.swift
@@ -0,0 +1,31 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift open source project
+//
+// Copyright (c) 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See http://swift.org/LICENSE.txt for license information
+// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+@testable import SPMBuildCore
+import Basics
+import struct PackageModel.BuildEnvironment
+import _InternalTestSupport
+import XCTest
+
+final class BuildParametersTests: XCTestCase {
+    func testConfigrationDependentProperties() throws {
+        // Ensure that properties that depend on the "configuration" property are
+        // correctly updated after modifying the configuration.
+        var parameters = mockBuildParameters(
+            destination: .host,
+            environment: BuildEnvironment(platform: .linux, configuration: .debug)
+        )
+        XCTAssertEqual(parameters.enableTestability, true)
+        parameters.configuration = .release
+        XCTAssertEqual(parameters.enableTestability, false)
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ extension BuildPlan {`
`43`	`43`	`) throws -> [(product: ResolvedProduct, discoveryTargetBuildDescription: SwiftModuleBuildDescription?, entryPointTargetBuildDescription: SwiftModuleBuildDescription)] {`
`44`	`44`	`var explicitlyEnabledDiscovery = false`
`45`	`45`	`var explicitlySpecifiedPath: AbsolutePath?`
`46`		`- if case let .entryPointExecutable(caseExplicitlyEnabledDiscovery, caseExplicitlySpecifiedPath) = destinationBuildParameters.testingParameters.testProductStyle {`
	`46`	`+ if case let .entryPointExecutable(caseExplicitlyEnabledDiscovery, caseExplicitlySpecifiedPath) = destinationBuildParameters.testProductStyle {`
`47`	`47`	`explicitlyEnabledDiscovery = caseExplicitlyEnabledDiscovery`
`48`	`48`	`explicitlySpecifiedPath = caseExplicitlySpecifiedPath`
`49`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -826,8 +826,6 @@ public final class SwiftCommandState {`
`826`	`826`	`isVerbose: self.logLevel <= .info`
`827`	`827`	`),`
`828`	`828`	`testingParameters: .init(`
`829`		`- configuration: options.build.configuration ?? self.preferredBuildConfiguration,`
`830`		`- targetTriple: triple,`
`831`	`829`	`forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery`
`832`	`830`	`testEntryPointPath: options.build.testEntryPointPath`
`833`	`831`	`)`