COR-173 - Move prebuilt binaries to release assets - #102
Closed
dbezverkhnii wants to merge 3 commits into
Closed
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The SwiftPM test target currently references non-existent test source files (and there are additional correctness concerns in the new helper/queue logic).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates MailCore2’s Windows distribution flow to publish/download prebuilt C/C++ binaries as GitHub release assets keyed by a source digest, and adds an IMAP-side escape hatch to interrupt the currently running command to avoid teardown being blocked on socket timeouts.
Changes:
- Introduces a digest-based Windows prebuilt pipeline (pins file, publish/download/check scripts) and a Linux PR check that fails when the corresponding prebuilt asset is missing.
- Adds
interruptCurrentCommand()support across the C++ core, async IMAP operations, C wrapper, and Swift bindings, plus a Darwin-only unit test covering running vs queued operations. - Updates documentation (
README.md,AGENTS.md) and ignores SwiftPM local state.
File summaries
| File | Description |
|---|---|
| windows-build-pins.json | Adds pinned dependency/toolchain versions used in the Windows prebuilt digest. |
| unittest/IMAPInterruptCurrentCommandTests.swift | Adds Darwin-only tests validating interrupt behavior. |
| src/swift/imap/IMAPBaseOperation.swift | Exposes interruptCurrentCommand() in Swift bindings. |
| src/include/MailCore/MCOperationQueue.h | Adds queue API/state to interrupt the currently running operation. |
| src/include/MailCore/MCOperation.h | Adds base Operation::interrupt() virtual hook. |
| src/include/MailCore/MCIMAPSession.h | Declares IMAP session command interruption API. |
| src/include/MailCore/MCIMAPOperation.h | Declares IMAP operation interrupt APIs. |
| src/include/MailCore/MCIMAPAsyncConnection.h | Declares async connection interrupt entrypoint. |
| src/include/MailCore/CIMAPBaseOperation.h | Exposes interrupt API in the C wrapper header. |
| src/core/imap/MCIMAPSession.h | Mirrors IMAP session header changes for core build. |
| src/core/imap/MCIMAPSession.cpp | Implements stream cancellation for interrupting a blocked command. |
| src/core/basetypes/MCOperationQueue.h | Mirrors queue header changes for core build. |
| src/core/basetypes/MCOperationQueue.cpp | Implements running-op tracking and interruption in the queue. |
| src/core/basetypes/MCOperation.h | Mirrors base Operation interrupt hook for core build. |
| src/core/basetypes/MCOperation.cpp | Adds default no-op Operation::interrupt() implementation. |
| src/c/imap/CIMAPBaseOperation.h | Mirrors C wrapper header update for C build. |
| src/c/imap/CIMAPBaseOperation.cpp | Implements synthesized C wrapper interrupt function. |
| src/async/imap/MCIMAPOperation.h | Mirrors async IMAP op header changes for async build. |
| src/async/imap/MCIMAPOperation.cpp | Implements async IMAP op interruption by canceling the stream. |
| src/async/imap/MCIMAPAsyncConnection.h | Mirrors async connection header update for async build. |
| src/async/imap/MCIMAPAsyncConnection.cpp | Routes interrupt requests through the queue’s running-op check. |
| README.md | Documents the new digest-based Windows prebuilt workflow. |
| Package.swift | Updates SwiftPM test target source list to include new tests. |
| build-windows-5.10/Publish-Mailcore2Prebuilt.ps1 | Adds the end-to-end publish script (build, verify, upload). |
| build-windows-5.10/Prebuilt-Common.ps1 | Centralizes digest/release/asset helper logic shared by scripts. |
| build-windows-5.10/Get-Mailcore2.ps1 | Switches download to digest-named GitHub release assets with digest verification. |
| build-windows-5.10/Check-PrebuiltPublished.ps1 | Adds PR check script that fails when the needed prebuilt asset is missing. |
| build-windows-5.10/Build-Mailcore2.ps1 | Supports building with a bundled dependency archive and pinned dependency commits. |
| build-windows-5.10/Build-Helpers.ps1 | Adds public stand-ins for internal RD PowerShell modules. |
| AGENTS.md | Adds agent-facing operational guidance for the new prebuilt pipeline. |
| .gitignore | Ignores SwiftPM local state files. |
| .github/workflows/pull-request-check.yml | Adds a Linux job to check prebuilt asset publication for the merge commit. |
Review details
- Files reviewed: 31/32 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+71
to
+84
| function Initialize-Toolchain { | ||
| $pinsPath = Join-Path (Split-Path $PSScriptRoot) "windows-build-pins.json" | ||
| $toolchain = (Get-Content -LiteralPath $pinsPath -Raw | ConvertFrom-Json).toolchain | ||
|
|
||
| $swiftRoot = Join-Path $env:LOCALAPPDATA "Programs\Swift" | ||
| $swiftBin = Get-ChildItem -LiteralPath (Join-Path $swiftRoot "Toolchains") -Filter clang-cl.exe -Recurse -File | | ||
| Select-Object -First 1 -ExpandProperty DirectoryName | ||
| if (-not $swiftBin) { throw "Swift clang-cl.exe not found under $swiftRoot" } | ||
| $msvcBin = "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\$($toolchain.msvcToolset)\bin\Hostx64\x64" | ||
| $windowsSdkBin = "C:\Program Files (x86)\Windows Kits\10\bin\$($toolchain.windowsSdk)\x64" | ||
| $cmakeBin = "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin" | ||
| $ninjaBin = "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja" | ||
| $env:Path = "$swiftBin;$msvcBin;$windowsSdkBin;$cmakeBin;$ninjaBin;$env:Path" | ||
| } |
Comment on lines
154
to
158
| op->retain()->autorelease(); | ||
|
|
||
| MCB_LOCK(&mLock); | ||
| mRunningOperation = NULL; | ||
| mOperations->removeObjectAtIndex(0); |
Comment on lines
+111
to
+115
| `build-windows-5.10\Build-Helpers.ps1` provides the subset of the internal `RDBuildCMake` / `RDBuildMSVC` | ||
| / `RDDependency` modules that the C/C++ build uses, and `Prebuilt-Common.ps1` dot-sources it after | ||
| importing those modules so it wins even where they are installed — the pinned toolchain has to | ||
| be the one that actually runs. A plain clone plus the pinned toolchain is enough to build and | ||
| publish the prebuilt. |
mailcore2-windows-0a7de371ed29.zip, already on the release, carries an MCIMAPAsyncConnection.h from the COR-180 branch: it declares setReserved, isReserved, setAutomaticDisconnectDelay and automaticDisconnectDelay, none of which are in the mailcore2.dll beside it. It is the only one of the 132 shipped headers that public-headers.cmake does not declare. src/CMakeLists.txt stages the public headers into the CMake binary directory with file(COPY) and installs that directory whole. file(COPY) adds but never prunes, so the header stayed behind from a build of another branch and was packaged. Publish cleaned the install tree and the staging copy but not the build directory, so: - the build directory is now cleaned too, which is what actually stops it; - the archive check no longer requires MCIMAPAsyncConnection.h. That line came over from COR-180 with the release-assets work, but this branch does not make the header public and spark2 never did - so the check was passing only by finding the stale copy. It now requires MailCore.h, which public-headers.cmake has always declared. The digest does not cover the publish script, so the archive keeps its name: re-publish 0a7de371ed29 with -Force to replace the contaminated one. Reported by Cursor Bugbot on the closed PR #101; the header it named there was the C one, which this branch does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Package.swift still listed IMAPConnectionLeaseTests.swift and SilentTCPEndpoint.swift for MailCoreTests. Both belong to the connection-lease work, not to this branch, and neither file is in the tree. SwiftPM reports them as warnings rather than errors, which is why the macOS and Android checks stayed green, but they are dead references either way. Removed; `swift package describe` is now clean. AGENTS.md said Prebuilt-Common.ps1 dot-sources Build-Helpers.ps1 after importing the RD modules so it wins even where they are installed. It does not: it loads one or the other. That sentence described a change that was made and then reverted, and it is exactly the sort of thing someone would rely on while diagnosing a toolchain problem. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 725185d. Configure here.
| Copy-Item -Path "$TidyDependencyPath\include" -Destination "$ExternalsPath\include\tidy" -Recurse -Force -ErrorAction Stop -PassThru | Write-Host | ||
| Copy-Item -Path "$TidyDependencyPath\rdtidy.lib" -Destination "$ExternalsPath\lib64" -Force -ErrorAction Stop -PassThru | Write-Host | ||
| Copy-Item -Path "$TidyDependencyPath\include\*" -Destination "$ExternalsPath\include\tidy" -Recurse -Force -ErrorAction Stop -PassThru | Write-Host | ||
| Copy-Item -Path "$TidyDependencyPath\lib\rdtidy.lib" -Destination "$ExternalsPath\lib64" -Force -ErrorAction Stop -PassThru | Write-Host |
There was a problem hiding this comment.
Tidy headers copy destination missing
High Severity
The tidy headers copy now uses a wildcard into Externals\include\tidy, but that directory is never created. On a clean machine the copy fails, so Publish-Mailcore2Prebuilt.ps1 cannot finish the Windows archive.
Reviewed by Cursor Bugbot for commit 725185d. Configure here.
| To test unreleased C++ changes, add `-BuildMailcore2` to the `Build-SwiftMailcore.ps1` | ||
| invocation in `Build-SparkCore.ps1` — mailcore2 is then compiled from the pinned checkout | ||
| instead of downloading the archive. It needs no credentials: the dependency archive is fetched | ||
| automatically. Slower (~10-15 min extra), for test builds only; remove it before release. |
There was a problem hiding this comment.
Source build skips dependency archive
Medium Severity
-BuildMailcore2 is documented to fetch mailcore2-windows-deps automatically with no credentials, but Build-SwiftMailcore.ps1 still calls Build-Mailcore2.ps1 without -PrebuiltDependenciesArchive, so that path still needs SPARK_PREBUILT_KEY and a C:\Library layout.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 725185d. Configure here.
| $windowsSdkBin = "C:\Program Files (x86)\Windows Kits\10\bin\$($toolchain.windowsSdk)\x64" | ||
| $cmakeBin = "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin" | ||
| $ninjaBin = "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja" | ||
| $env:Path = "$swiftBin;$msvcBin;$windowsSdkBin;$cmakeBin;$ninjaBin;$env:Path" |
There was a problem hiding this comment.
Pinned Swift toolchain not selected
Medium Severity
Initialize-Toolchain puts the first clang-cl.exe under Toolchains on PATH, not the Swift version from windows-build-pins.json. A machine with more than one toolchain can publish binaries that do not match the pinned compiler.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 725185d. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Medium Risk
Windows consumer builds depend on correct digest/release assets and manual publishes; IMAP interrupt intentionally leaves the session unusable until reconnect—misuse could break live commands.
Overview
Windows C/C++ prebuilts move from numbered S3 zips (
mailcore2-all-<N>) to public GitHub release assets onwindows-prebuilt, namedmailcore2-windows-<digest>.zipwhere the digest hashes git tree entries forsrc(excludingsrc/swift),CMakeLists.txt, and newwindows-build-pins.json.Get-Mailcore2.ps1downloads by digest (noSPARK_PREBUILT_KEY);Publish-Mailcore2Prebuilt.ps1builds, verifies, and uploads viagh. Shared logic lives inPrebuilt-Common.ps1;Build-Helpers.ps1can stand in for internal RD modules so publishing works from a plain clone.A PR check (
mailcore2 - Windows prebuilton Ubuntu) runsCheck-PrebuiltPublished.ps1against the merge commit so missing uploads fail early. README and new AGENTS.md document the flow;.gitignoreadds SwiftPM local state.IMAP teardown: operations can call
interruptCurrentCommand()to cancel only the in-flight command on their connection (OperationQueue::interruptRunningOperation,IMAPSession::interruptCurrentCommand/mailstream_cancel), exposed through C/Swift and covered byIMAPInterruptCurrentCommandTests.Build-Mailcore2.ps1also gains optional bundled deps archive, pinned git deps, and extra runtime DLLs in the install tree when publishing.Reviewed by Cursor Bugbot for commit 725185d. Bugbot is set up for automated code reviews on this repo. Configure here.