fix: SPM correct platform for iOS #38
Conversation
WalkthroughA new iOS version extraction function is added to enhance parsing of deployment targets from Xcode projects, while the SPM utility is updated to use this function with conditional version string formatting for Package.swift generation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/ios/common.ts(1 hunks)cli/src/util/spm.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/src/ios/common.ts (1)
cli/src/definitions.ts (1)
Config(141-148)
cli/src/util/spm.ts (1)
cli/src/ios/common.ts (1)
getMajorMinoriOSVersion(117-141)
🪛 GitHub Actions: CI
cli/src/ios/common.ts
[warning] 1-1: Code style issues found in cli/src/ios/common.ts. Run Prettier with --write to fix.
🔇 Additional comments (1)
cli/src/util/spm.ts (1)
11-11: LGTM: Import updated correctly.The import change aligns with the new function that extracts patch versions from the deployment target.
| export function getMajorMinoriOSVersion(config: Config): string { | ||
| const pbx = readFileSync(join(config.ios.nativeXcodeProjDirAbs, 'project.pbxproj'), 'utf-8'); | ||
| const searchString = 'IPHONEOS_DEPLOYMENT_TARGET = '; | ||
| const startIndex = pbx.indexOf(searchString); | ||
| if (startIndex === -1) { | ||
| return ''; | ||
| } | ||
| const valueStart = startIndex + searchString.length; | ||
| // Extract until semicolon or newline (typical end of value in pbxproj) | ||
| const endIndex = pbx.indexOf(';', valueStart); | ||
| const newlineIndex = pbx.indexOf('\n', valueStart); | ||
| const actualEnd = endIndex !== -1 && newlineIndex !== -1 | ||
| ? Math.min(endIndex, newlineIndex) | ||
| : endIndex !== -1 | ||
| ? endIndex | ||
| : newlineIndex !== -1 | ||
| ? newlineIndex | ||
| : pbx.length; | ||
| let iosVersion = pbx.substring(valueStart, actualEnd).trim(); | ||
| // Remove trailing .0 if present | ||
| if (iosVersion.endsWith('.0')) { | ||
| iosVersion = iosVersion.slice(0, -2); | ||
| } | ||
| return iosVersion; | ||
| } |
There was a problem hiding this comment.
Review the empty string return behavior and address the pipeline failure.
The function logic for extracting the iOS deployment target is sound, but there are a few concerns:
-
Empty string return (Line 122): Returning an empty string when
IPHONEOS_DEPLOYMENT_TARGETis not found will cause invalid Swift code generation inspm.ts(see my comment on that file). Consider either throwing an error or coordinating with the caller to handle this case. -
Pipeline failure: Prettier reports code style issues. Please run
prettier --write cli/src/ios/common.tsto fix formatting.
Optional refactor to reduce duplication:
The logic overlaps significantly with getMajoriOSVersion (Lines 107-115). Consider refactoring to share the file reading and search logic:
export function getMajoriOSVersion(config: Config): string {
const fullVersion = getMajorMinoriOSVersion(config);
return fullVersion.split('.')[0] || fullVersion;
}This would maintain backward compatibility while eliminating duplication.
🤖 Prompt for AI Agents
In cli/src/ios/common.ts around lines 117 to 141, the function currently returns
an empty string when IPHONEOS_DEPLOYMENT_TARGET is not found which leads to
invalid Swift code generation downstream; change this to throw a descriptive
Error (e.g., `throw new Error("IPHONEOS_DEPLOYMENT_TARGET not found in
project.pbxproj")`) so callers must handle it; after making that change, run
`prettier --write cli/src/ios/common.ts` to fix formatting; optionally refactor
getMajoriOSVersion to call getMajorMinoriOSVersion and derive the major part to
remove duplication.
| if (iosVersion.endsWith('.0')) { | ||
| iosVersion = iosVersion.slice(0, -2); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "common.ts" -path "*ios*" | head -5Repository: Cap-go/capacitor-plus
Length of output: 88
🏁 Script executed:
head -150 ./cli/src/ios/common.ts | tail -20Repository: Cap-go/capacitor-plus
Length of output: 733
🏁 Script executed:
sed -n '100,160p' ./cli/src/ios/common.tsRepository: Cap-go/capacitor-plus
Length of output: 1730
🏁 Script executed:
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "getMajorMinoriOSVersion\|IPHONEOS_DEPLOYMENT_TARGET" 2>/dev/nullRepository: Cap-go/capacitor-plus
Length of output: 49
🏁 Script executed:
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" -o -name "*.spec.js" \) | head -20Repository: Cap-go/capacitor-plus
Length of output: 456
🏁 Script executed:
grep -n "getMajorMinoriOSVersion\|getMajoriOSVersion" ./cli/test/*.spec.ts 2>/dev/nullRepository: Cap-go/capacitor-plus
Length of output: 47
🏁 Script executed:
rg "getMajorMinoriOSVersion|getMajoriOSVersion" --type tsRepository: Cap-go/capacitor-plus
Length of output: 705
🏁 Script executed:
sed -n '1,50p' ./cli/src/util/spm.tsRepository: Cap-go/capacitor-plus
Length of output: 1939
🏁 Script executed:
grep -n -A 5 "getMajorMinoriOSVersion" ./cli/src/util/spm.tsRepository: Cap-go/capacitor-plus
Length of output: 552
🏁 Script executed:
sed -n '90,130p' ./cli/src/util/spm.tsRepository: Cap-go/capacitor-plus
Length of output: 1627
🏁 Script executed:
grep -r "getMajorMinoriOSVersion\|IPHONEOS_DEPLOYMENT_TARGET" ./cli/test/ --include="*.ts" --include="*.js"Repository: Cap-go/capacitor-plus
Length of output: 47
🏁 Script executed:
sed -n '100,110p' ./cli/src/util/spm.tsRepository: Cap-go/capacitor-plus
Length of output: 508
Trailing .0 removal logic is correct; add test coverage for version formats.
The logic properly handles iOS version formatting for SPM platform syntax:
15.0→15(uses.v15enum syntax)15.5→15.5(uses"15.5"string syntax)15.5.0→15.5(uses"15.5"string syntax)
This function lacks test coverage. Add tests to the test suite verifying these version formats are correctly extracted and normalized:
describe('getMajorMinoriOSVersion', () => {
it('should remove trailing .0 from versions', () => {
// Test with mocked pbxproj containing "IPHONEOS_DEPLOYMENT_TARGET = 15.0;"
// Expect: "15"
});
it('should preserve decimal versions', () => {
// Test with "IPHONEOS_DEPLOYMENT_TARGET = 15.5;"
// Expect: "15.5"
});
it('should handle three-part versions', () => {
// Test with "IPHONEOS_DEPLOYMENT_TARGET = 15.5.0;"
// Expect: "15.5"
});
});🤖 Prompt for AI Agents
In cli/src/ios/common.ts around lines 137 to 139, add unit tests covering
getMajorMinoriOSVersion behavior: create three tests that mock pbxproj content
with IPHONEOS_DEPLOYMENT_TARGET set to "15.0", "15.5", and "15.5.0"
respectively, assert the function returns "15", "15.5", and "15.5" (ensuring
trailing ".0" is removed and decimals preserved); place tests in the existing
iOS/common test file or create a new spec, mock or stub the file reading/parse
function used by getMajorMinoriOSVersion, and include assertions for each
expected normalized output.
| const iosVersion = getMajorMinoriOSVersion(config); | ||
|
|
||
| let packageSwiftText = `// swift-tools-version: 5.9 | ||
| import PackageDescription | ||
| // DO NOT MODIFY THIS FILE - managed by Capacitor CLI commands | ||
| let package = Package( | ||
| name: "CapApp-SPM", | ||
| platforms: [.iOS(.v${iosVersion})], | ||
| platforms: [.iOS(${iosVersion.includes('.') ? `"${iosVersion}"` : `.v${iosVersion}`})], |
There was a problem hiding this comment.
Handle empty version string to prevent invalid Swift code generation.
If getMajorMinoriOSVersion returns an empty string (when IPHONEOS_DEPLOYMENT_TARGET is not found in the pbxproj), Line 103 will generate invalid Swift code: .iOS(.v). This would cause build failures.
Consider adding a fallback or validation:
const iosVersion = getMajorMinoriOSVersion(config);
+ if (!iosVersion) {
+ fatal('Unable to determine iOS deployment target from project.pbxproj');
+ }
let packageSwiftText = `// swift-tools-version: 5.9Alternatively, provide a sensible default:
- const iosVersion = getMajorMinoriOSVersion(config);
+ const iosVersion = getMajorMinoriOSVersion(config) || '13';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const iosVersion = getMajorMinoriOSVersion(config); | |
| let packageSwiftText = `// swift-tools-version: 5.9 | |
| import PackageDescription | |
| // DO NOT MODIFY THIS FILE - managed by Capacitor CLI commands | |
| let package = Package( | |
| name: "CapApp-SPM", | |
| platforms: [.iOS(.v${iosVersion})], | |
| platforms: [.iOS(${iosVersion.includes('.') ? `"${iosVersion}"` : `.v${iosVersion}`})], | |
| const iosVersion = getMajorMinoriOSVersion(config) || '13'; | |
| let packageSwiftText = `// swift-tools-version: 5.9 | |
| import PackageDescription | |
| // DO NOT MODIFY THIS FILE - managed by Capacitor CLI commands | |
| let package = Package( | |
| name: "CapApp-SPM", | |
| platforms: [.iOS(${iosVersion.includes('.') ? `"${iosVersion}"` : `.v${iosVersion}`})], |
🤖 Prompt for AI Agents
In cli/src/util/spm.ts around lines 95 to 103, the computed iosVersion may be an
empty string which causes invalid Swift `.iOS(.v)` output; add a
validation/fallback so that if iosVersion is falsy use a sensible default (e.g.
"15.0" or major 15) before building packageSwiftText, and then keep the existing
conditional formatting: if the version contains a dot emit the quoted string
`"15.0"`, otherwise emit `.v15`; ensure the fallback is applied so the generated
Swift is always valid.
|
backported in ionic-team#8284 |
The problem
Currently, our Capacitor updater requires iOS 15.5 as the minimum iOS deployment target. Unfortunately, when using SPM, the Capacitor CLI will always default to iOS 15, regardless of whether iOS 15.5 is set in Xcode build settings. This is problematic and causes build issues.
The problem was first reported here: https://discord.com/channels/912707985829163099/1450088141997215744
Motivation
The Capacitor CLI should work with our own plugin when using SPM. This is a high priority issue as this is currently preventing some users from using Capgo. We must fix it as soon as possible
Business impact
HIGH ❗️
This issue is currently preventing some users from using Capgo, which could negatively impact our MMR. This MUST ne resolved as sonn as possible
Documentation used
https://developer.apple.com/documentation/packagedescription/supportedplatform/ios(_:)-83bbf
I use
.iOS("15.5")when a dot version is detected,iOS(.v15)if no dot version is detected. This is required by AppleTesting
I tested manually. If the test app is required, I will provide a zip of it.
Lint
I did not lint. This project differs from a typical Capgo project and the lint command did not work.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.