From 0d6fa537529d1e6058e89ee6a371802b3752d415 Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Fri, 27 Sep 2024 13:05:34 -0700 Subject: [PATCH] WIP: Implement SE-0455: SwiftPM @testable build setting This introduces a new `enableTesting` API in SwiftSetting which allows targets to explicitly control whether testability is enabled, as some projects do not want to use the @testable feature. This can also improve debug build performance as significantly fewer symbols will be exported from the binary in the case where @testable is disabled. The default is equivalent to `enableTesting(true, .when(configuration: .debug))`, so there is no behavior change from today without explicitly adopting this new API. The --enable-testable-imports/--disable-testable-imports command line flags now acts as overrides -- if specified, they will override any build settings configured at the target level, and if unspecified, the target-level settings will be respected. --- .../SwiftModuleBuildDescription.swift | 24 ++++++++++++++++--- Sources/Commands/SwiftTestCommand.swift | 4 ++-- .../PackageDescription/BuildSettings.swift | 16 +++++++++++++ .../PackageLoading/ManifestJSONParser.swift | 14 +++++++++++ Sources/PackageLoading/PackageBuilder.swift | 10 ++++++++ Sources/PackageModel/BuildSettings.swift | 2 ++ .../TargetBuildSettingDescription.swift | 4 +++- .../ManifestSourceGeneration.swift | 8 +++++++ .../BuildParameters+Testing.swift | 12 ---------- Sources/XCBuildSupport/PIFBuilder.swift | 8 ------- Sources/XCBuildSupport/XcodeBuildSystem.swift | 1 - 11 files changed, 76 insertions(+), 27 deletions(-) diff --git a/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift b/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift index 94e902be0e4..7253427febc 100644 --- a/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift +++ b/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift @@ -979,6 +979,8 @@ public final class SwiftModuleBuildDescription { /// Testing arguments according to the build configuration. private var testingArguments: [String] { + let enableTesting: Bool + if self.isTestTarget { // test targets must be built with -enable-testing // since its required for test discovery (the non objective-c reflection kind) @@ -991,11 +993,27 @@ public final class SwiftModuleBuildDescription { result += ["-Xfrontend", "-enable-cross-import-overlays"] return result - } else if self.buildParameters.enableTestability { - return ["-enable-testing"] + } else if let enableTestability = self.buildParameters.testingParameters.explicitlyEnabledTestability { + // Let the command line flag override + enableTesting = enableTestability } else { - return [] + // Use the target settings + let enableTestabilitySetting = self.buildParameters.createScope(for: self.target).evaluate(.ENABLE_TESTABILITY) + if !enableTestabilitySetting.isEmpty { + enableTesting = enableTestabilitySetting.contains(where: { $0 == "YES" }) + } else { + // By default, 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` requires building with testable enabled if @testable is being used. + // 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 + enableTesting = self.buildParameters.configuration == .debug + } } + + return enableTesting ? ["-enable-testing"] : [] } /// Module cache arguments. diff --git a/Sources/Commands/SwiftTestCommand.swift b/Sources/Commands/SwiftTestCommand.swift index 1fc6276b4f4..e1dde2f2b03 100644 --- a/Sources/Commands/SwiftTestCommand.swift +++ b/Sources/Commands/SwiftTestCommand.swift @@ -189,8 +189,8 @@ struct TestCommandOptions: ParsableArguments { var xUnitOutput: AbsolutePath? /// Generate LinuxMain entries and exit. - @Flag(name: .customLong("testable-imports"), inversion: .prefixedEnableDisable, help: "Enable or disable testable imports. Enabled by default.") - var enableTestableImports: Bool = true + @Flag(name: .customLong("testable-imports"), inversion: .prefixedEnableDisable, help: "Enable or disable testable imports. Based on target settings by default.") + var enableTestableImports: Bool? /// Whether to enable code coverage. @Flag(name: .customLong("code-coverage"), diff --git a/Sources/PackageDescription/BuildSettings.swift b/Sources/PackageDescription/BuildSettings.swift index 070900cc8b3..b6a52dca154 100644 --- a/Sources/PackageDescription/BuildSettings.swift +++ b/Sources/PackageDescription/BuildSettings.swift @@ -440,6 +440,22 @@ public struct SwiftSetting: Sendable { return SwiftSetting( name: "swiftLanguageMode", value: [.init(describing: mode)], condition: condition) } + + /// Whether `@testable` is enabled by passing the `-enable-testing` to the Swift compiler. + /// + /// - Since: First available in PackageDescription 6.2. + /// + /// - Parameters: + /// - enable: Whether to enable `@testable`. + /// - condition: A condition that restricts the application of the build setting. + @available(_PackageDescription, introduced: 6.2) + public static func enableTestableImport( + _ enable: Bool, + _ condition: BuildSettingCondition? = nil + ) -> SwiftSetting { + return SwiftSetting( + name: "enableTestableImport", value: [.init(describing: enable)], condition: condition) + } } /// A linker build setting. diff --git a/Sources/PackageLoading/ManifestJSONParser.swift b/Sources/PackageLoading/ManifestJSONParser.swift index c65d298ddc3..3c37cd2c9e5 100644 --- a/Sources/PackageLoading/ManifestJSONParser.swift +++ b/Sources/PackageLoading/ManifestJSONParser.swift @@ -554,6 +554,20 @@ extension TargetBuildSettingDescription.Kind { } return .swiftLanguageMode(version) + case "enableTestableImport": + guard let rawVersion = values.first else { + throw InternalError("invalid (empty) build settings value") + } + + if values.count > 1 { + throw InternalError("invalid build settings value") + } + + guard let value = Bool(rawVersion) else { + throw InternalError("invalid boolean value: \(rawVersion)") + } + + return .enableTestableImport(value) default: throw InternalError("invalid build setting \(name)") } diff --git a/Sources/PackageLoading/PackageBuilder.swift b/Sources/PackageLoading/PackageBuilder.swift index 235f9287b30..2eba28b23f6 100644 --- a/Sources/PackageLoading/PackageBuilder.swift +++ b/Sources/PackageLoading/PackageBuilder.swift @@ -1016,6 +1016,12 @@ public final class PackageBuilder { } } + for setting in manifestTarget.settings { + if case let .enableTestableImport(enable) = setting.kind, enable, setting.condition?.config == "release" { + self.observabilityScope.emit(warning: "'\(potentialModule.name)' should not enable `@testable import` when building in release mode") + } + } + // Create and return the right kind of target depending on what kind of sources we found. if sources.hasSwiftSources { return try SwiftModule( @@ -1223,6 +1229,10 @@ public final class PackageBuilder { } values = [version.rawValue] + + case .enableTestableImport(let enable): + decl = .ENABLE_TESTABILITY + values = enable ? ["YES"] : ["NO"] } // Create an assignment for this setting. diff --git a/Sources/PackageModel/BuildSettings.swift b/Sources/PackageModel/BuildSettings.swift index 3320bdf6307..66b8bbab345 100644 --- a/Sources/PackageModel/BuildSettings.swift +++ b/Sources/PackageModel/BuildSettings.swift @@ -14,6 +14,8 @@ public enum BuildSettings { /// Build settings declarations. public struct Declaration: Hashable { + public static let ENABLE_TESTABILITY: Declaration = .init("ENABLE_TESTABILITY") + // Swift. public static let SWIFT_ACTIVE_COMPILATION_CONDITIONS: Declaration = .init("SWIFT_ACTIVE_COMPILATION_CONDITIONS") diff --git a/Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift b/Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift index 807d8e804da..dcf867cd438 100644 --- a/Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift +++ b/Sources/PackageModel/Manifest/TargetBuildSettingDescription.swift @@ -41,13 +41,15 @@ public enum TargetBuildSettingDescription { case swiftLanguageMode(SwiftLanguageVersion) + case enableTestableImport(Bool) + public var isUnsafeFlags: Bool { switch self { case .unsafeFlags(let flags): // If `.unsafeFlags` is used, but doesn't specify any flags, we treat it the same way as not specifying it. return !flags.isEmpty case .headerSearchPath, .define, .linkedLibrary, .linkedFramework, .interoperabilityMode, - .enableUpcomingFeature, .enableExperimentalFeature, .swiftLanguageMode: + .enableUpcomingFeature, .enableExperimentalFeature, .swiftLanguageMode, .enableTestableImport: return false } } diff --git a/Sources/PackageModel/ManifestSourceGeneration.swift b/Sources/PackageModel/ManifestSourceGeneration.swift index e6860202f9c..2f044545584 100644 --- a/Sources/PackageModel/ManifestSourceGeneration.swift +++ b/Sources/PackageModel/ManifestSourceGeneration.swift @@ -534,6 +534,12 @@ fileprivate extension SourceCodeFragment { params.append(SourceCodeFragment(from: condition)) } self.init(enum: setting.kind.name, subnodes: params) + case .enableTestableImport(let enable): + params.append(SourceCodeFragment(boolean: enable)) + if let condition = setting.condition { + params.append(SourceCodeFragment(from: condition)) + } + self.init(enum: setting.kind.name, subnodes: params) } } } @@ -688,6 +694,8 @@ extension TargetBuildSettingDescription.Kind { return "enableExperimentalFeature" case .swiftLanguageMode: return "swiftLanguageMode" + case .enableTestableImport: + return "enableTestableImport" } } } diff --git a/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift b/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift index 357c7de25e8..f66ac517987 100644 --- a/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift +++ b/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift @@ -105,18 +105,6 @@ extension BuildParameters { } } - /// 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( diff --git a/Sources/XCBuildSupport/PIFBuilder.swift b/Sources/XCBuildSupport/PIFBuilder.swift index 2dae7094dcf..d5130ba6e42 100644 --- a/Sources/XCBuildSupport/PIFBuilder.swift +++ b/Sources/XCBuildSupport/PIFBuilder.swift @@ -29,9 +29,6 @@ struct PIFBuilderParameters { /// Whether the toolchain supports `-package-name` option. let isPackageAccessModifierSupported: Bool - /// Whether or not build for testability is enabled. - let enableTestability: Bool - /// Whether to create dylibs for dynamic library products. let shouldCreateDylibForDynamicProducts: Bool @@ -343,7 +340,6 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder { debugSettings[.GCC_OPTIMIZATION_LEVEL] = "0" debugSettings[.ONLY_ACTIVE_ARCH] = "YES" debugSettings[.SWIFT_OPTIMIZATION_LEVEL] = "-Onone" - debugSettings[.ENABLE_TESTABILITY] = "YES" debugSettings[.SWIFT_ACTIVE_COMPILATION_CONDITIONS, default: []].append("DEBUG") debugSettings[.GCC_PREPROCESSOR_DEFINITIONS, default: ["$(inherited)"]].append("DEBUG=1") addBuildConfiguration(name: "Debug", settings: debugSettings) @@ -354,10 +350,6 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder { releaseSettings[.GCC_OPTIMIZATION_LEVEL] = "s" releaseSettings[.SWIFT_OPTIMIZATION_LEVEL] = "-Owholemodule" - if parameters.enableTestability { - releaseSettings[.ENABLE_TESTABILITY] = "YES" - } - addBuildConfiguration(name: "Release", settings: releaseSettings) for product in package.products.sorted(by: { $0.name < $1.name }) { diff --git a/Sources/XCBuildSupport/XcodeBuildSystem.swift b/Sources/XCBuildSupport/XcodeBuildSystem.swift index b5a8d46320d..cb3430d57dc 100644 --- a/Sources/XCBuildSupport/XcodeBuildSystem.swift +++ b/Sources/XCBuildSupport/XcodeBuildSystem.swift @@ -371,7 +371,6 @@ extension PIFBuilderParameters { self.init( triple: buildParameters.triple, isPackageAccessModifierSupported: buildParameters.driverParameters.isPackageAccessModifierSupported, - enableTestability: buildParameters.enableTestability, shouldCreateDylibForDynamicProducts: buildParameters.shouldCreateDylibForDynamicProducts, toolchainLibDir: (try? buildParameters.toolchain.toolchainLibDir) ?? .root, pkgConfigDirectories: buildParameters.pkgConfigDirectories,