Add an integration with swift-api-digester #721

owenv · 2021-06-20T18:42:47Z

The goal here is to add support to the driver for generating an API/ABI baseline or comparing to one immediately after emitting a module. This means higher-level build systems like SwiftPM can share the implementation and each won't need to have their own support for generating swift-api-digester command lines to add breaking-change detection.

A few notes on the implementation:

Right now, all the flags related to this functionality have separate API and ABI variants. The makes it possible to generate or compare both types of baseline in a single driver invocation, but I'm not entirely sure it's worth the trouble
The baselines are treated as 'auxilary' outputs, similar to swiftsourceinfo files, so if an output path isn't specified they'll look for a Project/ directory if one is present. If this doesn't make sense, we could make them regular supplementary outputs.
Right now, these features only work if -emit-module is used to emit a top-level module. I implemented it this way because if the module is considered 'auxilary', the swiftmodule has a temporary filename and swift-api-digester won't be able to load it. This limitation could probably be lifted in the future.
I'm using .api.json and .abi.json as the baseline file types, but only because I couldn't think of anything better.
Currently this depends on https://github.com/apple/swift/pull/37991, and uses the new -disable-fail-on-error option so that when a breaking change is detected we don't mark the build as failed. I think this is the usually the right behavior, but we could make it configurable.

The new flags are:

-emit-(api|abi)-baseline, -emit-(api|abi)-baseline-path: emit a baseline JSON file for the module being emitted
-compare-to-(api|abi)-baseline-path: compare the API/ABI of the module being emitted to the baseline at
-serialize-(api|abi)-breaking-changes-path: serialize the errors emitted by swift-api-digester instead of printing them to stdout
-(api|abi)-breakage-allowlist-path: specify a list of breaking changes which should be ignored

This is just my first pass at the feature, so any and all feedback is welcome. Thanks to @nkcsgexi for the idea!

This adds support for generating an API and/or ABI baseline as part of a driver build plan if a module is being emitted. As a result, build systems and higher-level tools no longer need to interact with swift-api-digester directly.

nkcsgexi · 2021-06-23T16:28:30Z

Thank you for working on this @owenv ! I was on vacation in the previous several days and I'll start review this exciting PR shortly!

nkcsgexi

This is amazing! @owenv Some comments are inlined.

nkcsgexi · 2021-06-27T16:50:37Z

Sources/SwiftOptions/ExtraOptions.swift

@@ -18,6 +18,18 @@ extension Option {
  public static let noEmitModuleSeparately: Option = Option("-no-emit-module-separately", .flag, attributes: [.helpHidden], helpText: "Force using merge-module as the incremental build mode")
  public static let useFrontendParseableOutput: Option = Option("-use-frontend-parseable-output", .flag, attributes: [.helpHidden], helpText: "Emit parseable-output from swift-frontend jobs instead of from the driver")

+  // API digester operations
+  public static let emitAPIBaseline: Option = Option("-emit-api-baseline", .flag, attributes: [.noInteractive, .supplementaryOutput], helpText: "Emit an API baseline file for the module")


Instead of introducing abi versions of every flags, we probably just keep the api versions of all these flags and introduce a -abi flag to indicate ABI checking is actually intended. This is less error-prone and aligns with the tool's flags.

Ok, that sounds good to me! Instead of -abi, right now I'm using -digester-mode abi or -digester-mode api (with the default being api if it's unspecified), which I think makes it a little clearer what the impact of the setting is.

nkcsgexi · 2021-06-27T17:04:38Z

Sources/SwiftDriver/Jobs/Planning.swift

@@ -462,6 +463,24 @@ extension Driver {
    }
  }

+  private mutating func addAPIDigesterJobs(addJob: (Job) -> Void) throws {


ABI checking is only relevant when -enable-library-evolution and -emit-module-interface are specified. Could you check these conditions before proceeding and diagnose as well?

👍 makes sense, done!

nkcsgexi · 2021-06-27T17:07:33Z

Sources/SwiftDriver/Jobs/APIDigesterJobs.swift

+      kind: mode.baselineGenerationJobKind,
+      tool: .absolute(try toolchain.getToolPath(.swiftAPIDigester)),
+      commandLine: commandLine,
+      inputs: [.init(file: modulePath, type: .swiftModule)],


For -abi, we should also require the interface file as the input. Please also add digester flag -use-interface-for-module CurrentMainModule when generating ABI baselines.

👍 makes sense, done!

Remove duplicated API/ABI digester flags Fixes for ABI baselines and comparisons

owenv · 2021-06-28T00:30:33Z

@nkcsgexi thanks for the feedback! I hadn't spent as much time thinking about the ABI-checking use case, so this is really helpful.

nkcsgexi · 2021-06-28T02:20:41Z

Sources/SwiftDriver/Driver/Driver.swift

+  ) throws -> VirtualPath.Handle? {
+    // Only emit a baseline if at least of the arguments was provided.
+    guard parsedOptions.hasArgument(.emitDigesterBaseline, .emitDigesterBaselinePath) else { return nil }
+    return try computeModuleAuxiliaryOutputPath(&parsedOptions,


Thanks for fixing the previous round of comments! Here will give us a location next to .swiftmodule file. Can we change it so that we emit next to .swiftsourceinfo file instead? .swiftsourceinfo is in the Project directory if you use XCBuild, which is a sub-directory reserved for local artifacts.

I think this is already using the same logic as .swiftsourceinfo, they both call computeModuleAuxiliaryOutputPath and should use the Project folder if it's present. The only difference is that source info is opt-out and baseline generation is opt-in. I have a couple basic tests for this in testBaselineOutputPath which roughly correspond to the ones in testSourceInfoFileEmitOption. It's possible I made a mistake somewhere though.

ah, you are right. I meant the build system has already been passing down the path of .swiftsourceinfo via output file map. We could just use that to decide where we should put the baseline files. I presume we could add a special case in here: https://github.com/apple/swift-driver/blob/main/Sources/SwiftDriver/Driver/OutputFileMap.swift#L49

Oh I see, that makes sense! I added the new rule and a couple tests involving output file maps

nkcsgexi

This looks great! @owenv

nkcsgexi · 2021-06-29T16:24:33Z

@swift-ci please test

owenv · 2021-06-30T03:01:19Z

Thanks! Once I merge swiftlang/swift#37991 and it makes it into a nightly toolchain the tests here should pass

nkcsgexi · 2021-06-30T16:49:53Z

Sounds good!

owenv · 2021-07-03T17:59:29Z

@swift-ci please test

owenv · 2021-07-03T22:51:33Z

The tests which run jobs are now checking for flag availability so they won't fail when run with an older toolchain. I verified in swiftlang/swift#38251 that they run & pass with new toolchains, so this is good to go.

Initial API/ABI baseline generation support

f26cb05

This adds support for generating an API and/or ABI baseline as part of a driver build plan if a module is being emitted. As a result, build systems and higher-level tools no longer need to interact with swift-api-digester directly.

owenv requested review from nkcsgexi and artemcm June 20, 2021 18:42

nkcsgexi reviewed Jun 27, 2021

View reviewed changes

Add support for API digester baseline comparison jobs

eac3fd5

Remove duplicated API/ABI digester flags Fixes for ABI baselines and comparisons

owenv force-pushed the api-digester-integration branch from c52bfba to eac3fd5 Compare June 28, 2021 00:25

owenv mentioned this pull request Jun 28, 2021

[APIDigester] Add a -disable-fail-on-error option swiftlang/swift#37991

Merged

nkcsgexi reviewed Jun 28, 2021

View reviewed changes

Infer baseline output paths from swiftsourceinfo output file map entries

6206c02

nkcsgexi approved these changes Jun 29, 2021

View reviewed changes

Skip execution tests with older toolchains

4ec1161

owenv mentioned this pull request Jul 3, 2021

[DNM] Cross-repo testing swiftlang/swift#38251

Closed

owenv merged commit 4f4e853 into swiftlang:main Jul 3, 2021

artemcm mentioned this pull request Jul 6, 2021

Revert "Add an integration with swift-api-digester" #744

Merged

nkcsgexi mentioned this pull request Sep 8, 2021

[5.5] Cherry-pick changes about ABI checking integration #827

Merged

Add an integration with swift-api-digester #721

Add an integration with swift-api-digester #721

Uh oh!

Conversation

owenv commented Jun 20, 2021

Uh oh!

nkcsgexi commented Jun 23, 2021

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owenv commented Jun 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

nkcsgexi commented Jun 29, 2021

Uh oh!

owenv commented Jun 30, 2021

Uh oh!

nkcsgexi commented Jun 30, 2021

Uh oh!

owenv commented Jul 3, 2021

Uh oh!

owenv commented Jul 3, 2021

Uh oh!

Uh oh!