Implement diagnose-api-breaking-changes support with --build-system swiftbuild #8857
Conversation
owenv
requested review from
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
|
@swift-ci test |
| // The opt-in/opt-out mechanism doesn't seem to be used. | ||
| // It is kept around for abundance of caution. If "why is it needed" | ||
| // not identified by March 2025, then it is OK to remove it. | ||
| try XCTSkipIf( |
There was a problem hiding this comment.
issue (possibly-blocking): The converted test does not appear to add support for skipping tests if SWIFTPM_TEST_API_DIFF_OUTPUT environment is set to 0. Do we still need to support this?
There was a problem hiding this comment.
I'm fairly confident this was only needed several years ago before we had a fully featured api digester tool available everywhere
There was a problem hiding this comment.
Before then these tests were skipped entirely for many years, I believe unnecessarily, so I don't think we need this control any longer
| } | ||
| } | ||
|
|
||
| private func generateAPIBaselineUsingIntegratedAPIDigesterSupport(_ swiftCommandState: SwiftCommandState, baselineRevision: Revision, baselineDir: Basics.AbsolutePath, modulesNeedingBaselines: Set<String>) async throws -> Set<String> { |
There was a problem hiding this comment.
suggestion: Consider created automated tests that validate this function in isolation with happy path, and also fault injection, and more.
| @@ -173,39 +194,180 @@ struct APIDiff: AsyncSwiftCommand { | |||
| } | |||
| } | |||
|
|
|||
| private func determineModulesToDiff(packageGraph: ModulesGraph, observabilityScope: ObservabilityScope) throws -> Set<String> { | |||
| private func runWithIntegratedAPIDigesterSupport(_ swiftCommandState: SwiftCommandState, baselineRevision: Revision, baselineDir: Basics.AbsolutePath, modulesToDiff: Set<String>) async throws { | |||
There was a problem hiding this comment.
suggestion: Consider created automated tests that validate this function in isolation with happy path, and also fault injection, and more.
| } | ||
| } | ||
|
|
||
| private func generateAPIBaselineUsingIntegratedAPIDigesterSupport(_ swiftCommandState: SwiftCommandState, baselineRevision: Revision, baselineDir: Basics.AbsolutePath, modulesNeedingBaselines: Set<String>) async throws -> Set<String> { |
There was a problem hiding this comment.
suggestion: consider type aliasing, or creating more specific type, for the return value to aid in code readability. For example, instead of String, create a type alias which represents what the string is. e.g.: typealias ModuleName = String
| @@ -173,39 +194,180 @@ struct APIDiff: AsyncSwiftCommand { | |||
| } | |||
| } | |||
|
|
|||
| private func determineModulesToDiff(packageGraph: ModulesGraph, observabilityScope: ObservabilityScope) throws -> Set<String> { | |||
| private func runWithIntegratedAPIDigesterSupport(_ swiftCommandState: SwiftCommandState, baselineRevision: Revision, baselineDir: Basics.AbsolutePath, modulesToDiff: Set<String>) async throws { | |||
There was a problem hiding this comment.
suggestion: consider type aliasing, or creating more specific type, for the modulesToDiff argument to aid in code readability. For example, instead of String, create a type alias which represents what the string is. e.g.: typealias ModuleName = String, and then modulesToDiff: Set<ModuleName>
| @@ -687,6 +723,32 @@ public final class SwiftBuildSystem: SPMBuildCore.BuildSystem { | |||
| return settings | |||
| } | |||
|
|
|||
| private static func constructAPIDigesterSettingsOverrides(from digesterMode: BuildParameters.APIDigesterMode?) -> [String: String] { | |||
There was a problem hiding this comment.
suggestion: although this function is straightforward, consider writing automated tests to values the expected return value.
|
Looks like we need to fix looking up the api digester relative to the compiler that was used |
owenv
force-pushed
the
owenv/digester
branch
2 times, most recently
from
90a764e to
7da6b4f
Compare
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test Windows |
1 similar comment
|
@swift-ci test Windows |
depends on swiftlang/swift-build#601
closes #8847
Swift Build has builtin support for generating and comparing to API/ABI baselines. Wire this up to the diagnose-api-breaking-changes command. Notably, we now have a mechanism for running a Swift Build based build and collecting serialized diagnostics, which should be reusable in the implementation of the migrate command.