[APIDiff] Improve API digester integration & polish of experimental-api-diff #3485

owenv · 2021-05-08T22:07:52Z

Improve the swift-api-digester integration of the experimental-api-diff command by taking advantage of recent compiler improvements, and fix various bugs.

Motivation:

Make the experimental-api-diff command suitable for use in a production environment on macOS

Modifications:

Parse serialized diagnostic output from the API digester to differentiate breaking changes from other diagnostics
Improve output formatting
Generate and compare baselines on a per-module basis
Only include modules in the analysis if they're vended by library products
Remove some option-parsing hacks that aren't required in newer toolchains
Improve test coverage for the feature

Result:

After this change, experimental-api-diff should be much easier to use and mostly production-ready on macOS. Further changes will be required to get it working well on Linux & Windows because of SDK layout differences.

After this change, the command will only work with a swift-api-digester from the 5/5/2021 nightly toolchain or later. The tests already needed to be skipped on the self-hosted bots since Xcode toolchains won't include the tool until Swift 5.5, so I don't expect this to be an issue.

…iate breaking changes from other diags

…asier to understand

…ducts

…line

… opaque args

owenv · 2021-05-08T22:08:03Z

@swift-ci please smoke test

owenv · 2021-05-08T22:11:44Z

swiftlang/swift-tools-support-core#213
@swift-ci please smoke test

owenv · 2021-05-08T22:15:25Z

Hmm, looks like the self-hosted bots don't support cross-repo testing

owenv · 2021-05-09T19:05:34Z

swiftlang/swift-tools-support-core#213
@swift-ci please smoke test

owenv · 2021-05-15T18:27:39Z

@swift-ci please smoke test

elsh · 2021-05-16T06:44:00Z

Sources/Build/BuildPlan.swift

@@ -1919,14 +1919,7 @@ public class BuildPlan {
        // Add search paths from the system library targets.
        for target in graph.reachableTargets {
            if let systemLib = target.underlyingTarget as? SystemLibraryTarget {
-                for flag in self.pkgConfig(for: systemLib).cFlags {


Are these changes unnecessary now?

Yeah, as of swiftlang/swift#37206 this is unnecessary because the API digester and compiler are using the same option definitions

elsh · 2021-05-16T06:50:14Z

Sources/Commands/APIDigester.swift

+        var otherDiagnostics: [SerializedDiagnostics.Diagnostic]
+
+        /// `true` if the comparison succeeded and no breaking changes were found, otherwise `false`.
+        var isSuccessful: Bool {


Maybe rename 'hasNoAPIBreakingChanges' (or 'hasAPIBreakingChanges' that returns the opposite value)? ComparisonResult.isSuccessful doesn't necessarily convey what's successful.

sounds good to me, done!

elsh · 2021-05-16T06:53:27Z

Sources/Commands/SwiftPackageTool.swift

+            let baselineDir = try baselineDumper.emitAPIBaseline()
+
+            var succeeded = true
+            for module in try buildOp.getPackageGraph().apiDigesterModules {


Could you provide a reason for doing this per module? This triggers a process launch for each, which can cause issues such as a (noticeable) perf regression even when there's a minor non-api breaking change.

Right now the API digester assumes the baseline was generated from the same set of modules it's loading, so generating a baseline from multiple and then comparing that to one module at a time returns a bunch of false positives. I also didn't want to load every module at once because currently the source module for each breaking change isn't serialized, so there isn't a reliable way to sort the changes by product/target. Future compiler improvements might let us rollback this part of the change

Seems like making changes in the detection logic itself in the frontend should be the way to go (cc @nkcsgexi). Having to iterate over each module and launching the process for each won't scale. Is there at least a way to find depending modules that are affected by a diff so only those can be iterated over?

Eliminating false positives at all from the API digester tool is a very challenging and long-term task. Why do you think it won't scale for iterating over each module? Build system/swift-driver iterates each modules and build them.

@nkcsgexi Build system performs several things including dependency scanning and caching/cache lookups so incremental builds are faster. The api-digester doesn't do any of that and this change launches a frontend process for each module even when there's a very minor change at the top module. This won't be a problem if there are a few modules but projects containing over 100 modules are not uncommon.

…iftAPIDigester.ComparisonResult.hasNoAPIBreakingChanges

nkcsgexi

Thank you for working on this, @owenv ! I'm so excited that we can start to adopt the serialized diagnostics.

elsh

@abertelrud @tomerd Could you weigh in? See the discussions re: perf.

elsh · 2021-05-21T06:29:54Z

@owenv Since changing the detection logic itself is supposedly a difficult task, could you add support to allow an input module list (e.g. --module-list) and iterate over that specified list if provided? This way users have an option to run the api digester with a few specific modules that they know are likely affected by their changes vs unnecessarily having to compare with all modules.

Also would be great if you could add an explanation for the experimental-api-diff subcommand and its default behavior (iterating over all modules).

owenv · 2021-05-22T02:41:25Z

@elsh Sure, I've added support for filtering the comparison to a subset of the package. I chose --product and --target because I thought those would be more intuitive when working on a package, but I'm open to switching to --module or --module-list if you think that would be better. I've also updated the help text and parallelized the api-digester invocations.

owenv · 2021-05-22T02:41:32Z

@swift-ci please smoke test

abertelrud · 2021-05-24T21:01:01Z

@elsh Sure, I've added support for filtering the comparison to a subset of the package. I chose --product and --target because I thought those would be more intuitive when working on a package, but I'm open to switching to --module or --module-list if you think that would be better. I've also updated the help text and parallelized the api-digester invocations.

Target and product seem reasonable to me, since that's the terminology used for packages.

elsh · 2021-05-25T09:38:21Z

Sources/Commands/SwiftPackageTool.swift

+            The experimental-api-diff command can be used to compare the Swift API of \
+            a package to a baseline revision, diagnosing any breaking changes which have \
+            been introduced. By default, it compares every Swift module from the baseline \
+            revision which is part of a library product. If this behavior is undesirable, \


Please add a comment on the performance, for example, "This can result in a slow performance if there are a large number of modules. If this behavior is ..."

👍 I reworded this to add the extra note. I also ran a couple of benchmarks locally, and it looks like performance probably won't be a major concern for any package with < 100-200 targets. I've been primarily testing with swift-nio, which has about a dozen, and the diff operation takes under 2 seconds using a release w/ asserts frontend

elsh · 2021-05-25T09:39:14Z

Sources/Commands/SwiftPackageTool.swift


        @OptionGroup(_hiddenFromHelp: true)
        var swiftOptions: SwiftToolOptions

        @Argument(help: "The baseline treeish to compare to (e.g. a commit hash, branch name, tag, etc.)")
        var treeish: String

+        @Option(name: .customLong("product"), help: "One or more products to include in the diff")


Let's use a plural "products" since there can be multiple. Also expand the description: "One or more products to be included for comparison/diagnosis. If set, only the specified products will be compared."

👍 changed, I also switched the parsing strategy so these can be passed as --products P1 P2 instead of --product P1 --product P2.

Sources/Commands/SwiftPackageTool.swift

elsh · 2021-05-25T10:16:50Z

Sources/Commands/APIDigester.swift

+        let group = DispatchGroup()
+        let errors = ThreadSafeArrayStore<Swift.Error>()
+        for module in modulesToDiff {
+            DispatchQueue.sharedConcurrent.async(group: group) {


Thanks for adding this. Could you add support to control concurrency as well (DispatchSemaphore)? Will need to add a "-j" input option to the cmdline. This can be a separate PR though.

I ended up making this change in this PR since the command already accepted -j to control the parallelism of the build operation. Now both stages should respect the option consistently

Thanks for adding semaphore.

elsh · 2021-05-25T10:42:14Z

@elsh Sure, I've added support for filtering the comparison to a subset of the package. I chose --product and --target because I thought those would be more intuitive when working on a package, but I'm open to switching to --module or --module-list if you think that would be better. I've also updated the help text and parallelized the api-digester invocations.

Thanks for making the changes. The flag naming makes sense. Left a few comments.

owenv · 2021-05-26T04:57:19Z

@elsh thanks for the review!

owenv · 2021-05-26T04:57:27Z

@swift-ci please smoke test

Sources/Commands/SwiftPackageTool.swift

owenv · 2021-05-27T05:19:47Z

@swift-ci please smoke test

owenv added 13 commits May 7, 2021 23:12

[APIDiff] Parse serialized diagnostics from API digester to different…

fd3e92d

…iate breaking changes from other diags

[APIDiff] Refactoring and documentation to make the diffing process e…

8d6b0f6

…asier to understand

[APIDiff] Diff and print results from one module at a time

80b8864

[APIDiff] Improve test coverage

1f5a34b

[APIDiff] Use per-module baselines

125818f

[APIDiff] Only diagnose API breakage in modules vended by library pro…

ba89eaa

…ducts

[APIDiff] Add a test case involving a C-family target

3a421a4

[APIDiff] Print a message if a module has no breaking changes

5c1e2db

[APIDiff] Skip comparison of modules which aren't present in the base…

fbeb00a

…line

[APIDiff] Refactor API digester tool to accept a BuildPlan instead of…

1407c66

… opaque args

[APIDiff] Remove an option-parsing hack that's no longer needed

7bbf999

[APIDiff] Improve failure diagnostics

3557105

[APIDiff] Remove an obsolete test

5415614

owenv requested review from abertelrud, friedbunny, neonichu and tomerd as code owners May 8, 2021 22:07

[APIDiff] Adapt to a TSC API change

2738aa0

miggs597 assigned elsh and abertelrud May 10, 2021

elsh requested changes May 16, 2021

View reviewed changes

[APIDiff] Rename SwiftAPIDigester.ComparisonResult.isSuccessful -> Sw…

2457b7d

…iftAPIDigester.ComparisonResult.hasNoAPIBreakingChanges

nkcsgexi approved these changes May 20, 2021

View reviewed changes

elsh requested changes May 21, 2021

View reviewed changes

owenv added 4 commits May 21, 2021 17:56

[APIDiff] Allow restricting the diff to specific products and/or targets

81b4d2d

[APIDiff] Improve subcommand help

59f13f9

[APIDiff] Parallelize diff operation

cd88df0

[APIDiff] Parallelize baseline generation

3f07326

elsh reviewed May 25, 2021

View reviewed changes

Sources/Commands/SwiftPackageTool.swift Outdated Show resolved Hide resolved

elsh reviewed May 25, 2021

View reviewed changes

owenv added 2 commits May 25, 2021 21:12

[APIDiff] Clarify filtering options

e45725d

[APIDiff] Control paralellism of api-digester invocations with -j

9167479

elsh reviewed May 26, 2021

View reviewed changes

Sources/Commands/SwiftPackageTool.swift Outdated Show resolved Hide resolved

elsh approved these changes May 26, 2021

View reviewed changes

Update help text

cc0bdf4

owenv force-pushed the productionize-macos-api-diff branch from 3831194 to cc0bdf4 Compare May 27, 2021 05:19

elsh merged commit e1a732a into swiftlang:main May 27, 2021

hassila mentioned this pull request Dec 27, 2021

Fix formatting issue in Readme hassila/swift-plugin#1

Merged

swift-ci mentioned this pull request Jul 27, 2021

[SR-10540] SwiftPM: integration with swift-api-digester would be really cool #4716

Closed

[APIDiff] Improve API digester integration & polish of experimental-api-diff #3485

[APIDiff] Improve API digester integration & polish of experimental-api-diff #3485

Uh oh!

Conversation

owenv commented May 8, 2021

Motivation:

Modifications:

Result:

Uh oh!

owenv commented May 8, 2021

Uh oh!

owenv commented May 8, 2021

Uh oh!

owenv commented May 8, 2021

Uh oh!

owenv commented May 9, 2021

Uh oh!

owenv commented May 15, 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!

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!

elsh May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

elsh left a comment

Choose a reason for hiding this comment

Uh oh!

elsh commented May 21, 2021

Uh oh!

owenv commented May 22, 2021

Uh oh!

owenv commented May 22, 2021

Uh oh!

abertelrud commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsh May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

elsh commented May 25, 2021

Uh oh!

owenv commented May 26, 2021

Uh oh!

owenv commented May 26, 2021

Uh oh!

Uh oh!

owenv commented May 27, 2021

Uh oh!

Uh oh!

elsh May 21, 2021 •

edited

Loading

elsh May 25, 2021 •

edited

Loading