Repository navigation
Conversation
No CI job tested the CocoaPods opt-out: after the SwiftPM build, nothing ran `spm deinit` or looked at the files it leaves. The template app job now records the Podfile and ios/.gitignore checksums before `spm add`, runs `spm deinit` after the app upload, and checks that the Podfile and .gitignore are unchanged, the marker and ReactNativeSPM/ are gone, and the Xcode project and shared scheme have no SwiftPM references left. The check reads files only; `deinit` does not run `pod install`.
116499b to
7ffb988
Compare
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for adding this, it's the check I was asking for on #59742. 🙏
My main concern is where it runs. e2e-ios-templateapp.yml exists for the Maestro tests, and it's a bad place for a check that fails the same way every time:
- The
testjob hasneeds: build. If the deinit check fails in either flavor, the Maestro tests are skipped for both. A bug in the SwiftPM tooling would then hide the template E2E results. test-all.ymlreruns the whole workflow up to twice when it fails (test_e2e_ios_templateapp_retry_1/_retry_2). Those retries are for flaky Maestro runs. A deinit failure won't go away on retry, so we'd pay for three template builds onmacos-26-largebefore we see it.- The failure gets reported through
recordJobFailure/reportTestResultsas an E2E failure, which is misleading.
Could we move the check to test-ios-spm-newapp.yml, or to the end of the SwiftPM path in the test-ios-helloworld action? Both already run spm add --deintegrate, they don't retry, and they fail where people look for SwiftPM problems. It's also enough to run it in one flavor: deinit doesn't depend on the flavor.
Separately, and not something this PR has to do: the iOS template app still builds inside the retried E2E workflow, so every retry rebuilds it. #59135 (Maestro on remote simulators) doesn't change that: test-all.yml doesn't pass artifact-run-id, and test still needs build. #59646 also changes this file and keeps the build in the retry loop on purpose. Building the template in its own job, as RNTester and the Android template already do, would make it cleaner to add checks like this one to that job later.
One more question: going back to CocoaPods means spm deinit and then pod install. The CocoaPods jobs from #59742 only build apps that were never converted, so no job runs the full round trip. Could we add pod install plus a Debug workspace build after deinit, in one cell? A follow-up PR is fine too.
| name: RNTestProject-${{ matrix.flavor }} | ||
| overwrite: true | ||
| path: /tmp/RNTestProject/Build/Products/${{ matrix.flavor }}-iphonesimulator/RNTestProject.app | ||
| - name: Take the app back to CocoaPods |
There was a problem hiding this comment.
I'd move this step and the check after it out of this workflow. If either one fails, the test job (needs: build) gets skipped, so we lose the Maestro results for both flavors. test-all.yml then reruns the whole workflow twice, rebuilding the app each time, for a failure that won't change. test-ios-spm-newapp.yml or the SwiftPM path of the test-ios-helloworld action would be a better place: both already run spm add --deintegrate and don't retry.
| - name: Record the Podfile and .gitignore checksums | ||
| working-directory: /tmp/RNTestProject/ios | ||
| run: | | ||
| echo "PODFILE_SHA256=$(shasum -a 256 Podfile | cut -d ' ' -f 1)" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
nit: this step computes the sha256 and the absent value inline, while assert-spm-deinit.sh has sha256_or_absent for the same thing. They can drift apart. Also, if Podfile were missing, $(shasum … Podfile) would write an empty PODFILE_SHA256 without failing the step, and the script would then fail later with an unclear "Podfile changed". What about a record mode in the script, so both steps use one implementation?
| if [ ! -f "$PBXPROJ" ]; then | ||
| fail "$PBXPROJ is missing." | ||
| else | ||
| for INJECTED in \ |
There was a problem hiding this comment.
I understand why this isn't a byte-for-byte comparison of the pbxproj: deinit deliberately doesn't restore some of what add removes, like the dangling JavaScriptCore.framework reference and the empty Pods group (see spm-pbxproj.js:736). But a list of strings only catches leftovers someone thought to add to it. Could we copy ios/ before spm add and print a diff -r against it after deinit? Just for information, not as a check. When this fails, the diff would show what was left behind.
| # This source code is licensed under the MIT license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| # Checks that `npx react-native spm deinit` left an iOS app with only its |
There was a problem hiding this comment.
nit: the script fails on any packageReferences / packageProductDependencies in the pbxproj, so it assumes the app had no Swift packages before spm add. That's true for the template, but the header says it checks "an iOS app". Can we say that in the comment, so nobody reuses the script on an app that already has Swift packages?
Summary
npx react-native spm deinittakes an app from SwiftPM back to CocoaPods. No CI job tests this today. The unit tests mockpodandxcodebuild, and no job runsdeinit. A later change could break the way back without any failing check.This PR adds a file check to the template app build job (
e2e-ios-templateapp.yml, Debug and Release):spm add, the job records the sha256 ofios/Podfileandios/.gitignore, orabsentwhen.gitignoreis missing.npx react-native spm deinit. The uploaded app is not affected..github/workflow-scripts/assert-spm-deinit.shthen checks:.gitignoreis byte-identical, or gone if it was absent before;.spm-injected.jsonmarker andios/ReactNativeSPM/are gone;project.pbxprojhas no SwiftPM package references, product dependencies,ReactNativeSPMpaths, or SwiftPM build phases;The check reads files only.
deinitdoes not runpod install, so the steps take seconds. CocoaPods references are not checked, becausepod installrestores those, notdeinit.Changelog:
[INTERNAL] [ADDED] - CI check that
spm deinittakes the template app back to CocoaPodsTest Plan:
I ran the script locally on copies of
private/helloworld/ioswith a test Podfile:deinit: the script fails with one error for each SwiftPM change still in place.deinitthat I changed to keep the marker, a copy that keeps one.gitignoreline, and a copy that keeps the scheme pre-action: each fails on exactly that check.deinit, with.gitignoreabsent beforeaddand with an existing.gitignore: the script passes, and.gitignoreis byte-identical.bash -npasses on the script, and the workflow YAML parses.Not run: shellcheck (not installed locally), and the job itself. CI on this PR runs it.