fix(plugins)!: make PluginKit ABI resilient and gate it in CI#1532
Conversation
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9b8bb6cde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paths: | ||
| - "Plugins/TableProPluginKit/**" | ||
| - "scripts/check-pluginkit-abi.sh" | ||
| - ".github/workflows/pluginkit-abi.yml" |
There was a problem hiding this comment.
The new gate only runs for PluginKit sources, the script, or this workflow, but the script builds TablePro.xcodeproj and the library-evolution setting that makes the ABI resilient lives in TablePro.xcodeproj/project.pbxproj. A PR that only changes that project file (for example disabling BUILD_LIBRARY_FOR_DISTRIBUTION, changing the PluginKit target settings, or otherwise affecting the generated .swiftinterface) would bypass this required ABI check entirely, leaving the baseline stale despite an ABI-affecting change.
Useful? React with 👍 / 👎.
…a committed baseline
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc323001e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,5 +1,6 @@ | |||
| import Foundation | |||
|
|
|||
| @frozen | |||
There was a problem hiding this comment.
This PR only adds the new @frozen ABI annotations under Plugins/TableProPluginKit, but the repository also has the mirrored package source at Packages/TableProCore/Sources/TableProPluginKit; I checked .github/workflows/contract-drift.yml, and every PR touching this path runs scripts/audit-refactor-health.sh --check. Running that script on this commit reports new unbaselined divergences for 15 PluginKit files, so this PR's contract-drift job will fail and SwiftPM clients keep a stale copy of the public API. Please apply the same public-interface changes to the package mirror or otherwise resolve the drift.
Useful? React with 👍 / 👎.
| @frozen | ||
| public enum InspectorColumnType: String, Sendable, Equatable, CaseIterable { |
There was a problem hiding this comment.
This change modifies the inspector-facing PluginKit ABI (InspectorColumnType is part of DocumentInspectorPlugin), but the commit only bumps currentPluginKitVersion; inspector-only bundles such as CSVInspectorPlugin declare TableProInspectorKitVersion instead and validateBundleVersions checks that against currentInspectorKitVersion, which remains 1. A user-installed inspector plugin built against the old TableProPluginKit can therefore pass validation even though driver/export/import plugins are rejected for the same ABI transition, leaving the stale inspector path exposed to the load-time mismatch this bump is meant to avoid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 528a85a405
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Breaking: bump currentPluginKitVersion + every plugin Info.plist TableProPluginKitVersion, | ||
| then run scripts/release-all-plugins.sh <newVersion>. | ||
| EOF | ||
| exit 1 |
There was a problem hiding this comment.
Because .github/workflows/pluginkit-abi.yml runs this script as the required PR check for PluginKit source changes, any legitimate additive public API change will produce a .swiftinterface diff and then hit this unconditional failure path. The message says additive changes need no version bump, but there is no success path after deciding they are additive, so future binary-compatible PluginKit additions will be blocked by CI even when the new versioning policy says they should pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c509300726
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| name: "TableProPluginKit", | ||
| dependencies: [], | ||
| path: "Sources/TableProPluginKit" | ||
| path: "Sources/TableProPluginKit", |
There was a problem hiding this comment.
This target now resolves through the new Sources/TableProPluginKit symlink to Plugins/TableProPluginKit, so future edits under Plugins/TableProPluginKit/** change the TableProPluginKit product that the mobile project consumes. I checked .github/workflows/ios-tests.yml, and its PR/push filters only include TableProMobile/** and Packages/TableProCore/**; a PluginKit change can therefore skip the iOS test workflow even though TableProMobile.xcodeproj depends on the TableProPluginKit product from ../Packages/TableProCore and mobile code imports it. Please add the PluginKit source path to that workflow's filters when consolidating the source tree.
Useful? React with 👍 / 👎.
…-evolution # Conflicts: # CHANGELOG.md # Packages/TableProCore/Sources/TableProPluginKit/ImportFormatPlugin.swift # Packages/TableProCore/Sources/TableProPluginKit/PluginImportDataSink.swift # Packages/TableProCore/Sources/TableProPluginKit/PluginImportProgress.swift # Packages/TableProCore/Sources/TableProPluginKit/PluginImportTypes.swift # Packages/TableProCore/Sources/TableProPluginKit~HEAD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7df9e264a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| final class PluginManager { | ||
| static let shared = PluginManager() | ||
| static let currentPluginKitVersion = 17 | ||
| static let currentPluginKitVersion = 18 |
There was a problem hiding this comment.
When this raises currentPluginKitVersion to 18, validateBundleVersions rejects any bundle whose TableProPluginKitVersion is lower than the current value; I checked Plugins/JSONImportPlugin/Info.plist, and it still declares 17 while the other bundled PluginKit plugins were bumped. Since built-in .tableplugin bundles are validated in discoverPlugin before being queued, the bundled JSON import plugin will be skipped as outdated after this change, removing JSON import support until that plist is updated as well.
Useful? React with 👍 / 👎.
1 participant
Problem
Every app release that changed the plugin protocol broke users' installed registry plugins with "could not be loaded" /
noCompatibleBinary. Around ten PRs patched the distribution layer without fixing it.Root cause
TableProPluginKit was not built with Library Evolution (
BUILD_LIBRARY_FOR_DISTRIBUTIONwas unset). Without it, protocol witness tables and public struct layouts use fixed compile-time offsets, so any protocol change breaks the ABI for already-built plugins. That forces a globalcurrentPluginKitVersionbump and a re-release of every registry plugin, and any gap between the app release and the registry catching up is the window that produced the error.Fix
Part 1, resilient ABI (
bf57b10b):BUILD_LIBRARY_FOR_DISTRIBUTION = YESon the TableProPluginKit target only.currentPluginKitVersion17 to 18 (enabling Library Evolution is itself a breaking change) plus every pluginInfo.plist.@frozen(committed layout, fast, exhaustive switches stay clean).PluginCapabilitystays non-frozen with@unknown defaultbecause it is a growing vocabulary. The driver protocols and transfer structs stay non-frozen so they can grow.After this, adding a protocol requirement that has a default implementation no longer breaks installed plugins and no longer needs a version bump: the Swift runtime fills the missing witness from the default. Only a genuinely breaking change bumps the version.
Part 2, CI ABI gate (
c9b8bb6c):scripts/check-pluginkit-abi.shbuilds TableProPluginKit and diffs its public interface against a committed baseline (Plugins/TableProPluginKit/ABI-Baseline.swiftinterface)..github/workflows/pluginkit-abi.ymlruns it on PRs that touch the framework, so an ABI change cannot merge unnoticed and a breaking one cannot merge without the version bump.Verification
Rollout (one time)
Enabling Library Evolution changes the ABI format, so the existing kit-17 registry plugins must be rebuilt once:
After that, additive protocol changes stop bumping the version and stop forcing re-releases.
Not included
The reconciliation and retention machinery is left as-is. With the additive-no-bump rule it now only fires on a rare breaking bump, so changing it would be high risk for little gain.