-
Notifications
You must be signed in to change notification settings - Fork 240
build(ios,macos): update pod file locks to match pubspec #2594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request makes configuration updates for Google services and Firebase across multiple platforms. It adds the Google services plugin declarations to both Android build files and introduces empty configuration arrays for iOS build phases. Additionally, the Firebase options handling is modified to support Windows by returning appropriate options instead of throwing an error, and a comment in a web file has been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Options as DefaultFirebaseOptions.currentPlatform
App->>Options: Request Firebase options
alt Platform is Windows
Options-->>App: Return Windows Firebase options
else Other Platforms
Options-->>App: Throw UnsupportedError
end
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockmacos/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
android/app/build.gradle(1 hunks)android/settings.gradle(1 hunks)ios/Runner.xcodeproj/project.pbxproj(2 hunks)lib/firebase_options.dart(1 hunks)web/kdf/res/kdf_wrapper.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Desktop (macos)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (5)
web/kdf/res/kdf_wrapper.dart (1)
1-3: Documentation update for KDF wrapperThe comment has been expanded to suggest migrating the KDF JavaScript bootstrapper to Dart and compiling it to JavaScript. This is a good note for future development planning.
android/app/build.gradle (1)
3-5: Firebase Google Services plugin added correctlyThe Google Services plugin has been properly added within the FlutterFire Configuration block. This is necessary for Firebase integration in Android.
android/settings.gradle (1)
23-25: Google Services plugin version configured correctlyThe Google Services plugin (version 4.3.15) has been properly added at the project level within the FlutterFire Configuration block. This matches the plugin declaration in app/build.gradle.
ios/Runner.xcodeproj/project.pbxproj (2)
275-282: Clarify the addition of empty inputPaths and outputPaths in [CP] Copy Pods ResourcesThe addition of empty
inputPathsandoutputPathsarrays provides explicit placeholders for potential future entries. This is acceptable as long as it is intentional. However, please verify that leaving these arrays empty does not trigger any build-phase warnings or unexpected behavior in Xcode.
311-318: Validate empty arrays for [CP] Embed Pods Frameworks build phaseSimilar to the previous section, empty
inputPathsandoutputPathsarrays have been added in the [CP] Embed Pods Frameworks build phase. This maintains consistency and reserves the structure for future modifications. Please ensure that these empty arrays meet the intended configuration and will not cause any issues in your build process.
|
Visit the preview URL for this PR (updated for commit 9f5ab99): https://walletrc--pull-2594-merge-0l35uqu0.web.app (expires Mon, 14 Apr 2025 20:52:02 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
097ac33 to
403d2e9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/actions/releases/setup-android/action.yml (2)
35-35: Remove Trailing SpacesLine 35 contains trailing spaces (as flagged by YAMLlint). Removing these unnecessary spaces will help maintain clean YAML formatting and avoid potential linting errors.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 35-35: trailing spaces
(trailing-spaces)
36-41: Review Google Play Services Import Commenting StepThe new step to comment out the Google Play Services import in android/app/build.gradle is a practical workaround for the Firebase build error when no valid
google-services.jsonis present. However, thesedcommand uses a generic pattern that might unintentionally match or miss variations in the gradle file. To increase robustness, consider anchoring the regex to the start of the line and allowing flexible spacing. For example:- sed -i 's/id .com\.google\.gms\.google-services./\/\/ id .com\.google\.gms\.google-services./' android/app/build.gradle + sed -i 's/^\(id \+[com\.google\.gms\.google-services].*\)$/\/\/ \1/' android/app/build.gradleThis suggestion is optional if your current command reliably achieves the desired result in your target environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/komodo_ui_kit/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
.github/actions/releases/setup-android/action.yml(1 hunks)lib/firebase_options.dart(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/actions/releases/setup-android/action.yml
[error] 35-35: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (2)
lib/firebase_options.dart (2)
30-30: Windows platform support properly implementedThe code now correctly returns the Windows Firebase options instead of throwing an UnsupportedError. This change enables Firebase functionality on Windows, consistent with how other supported platforms are handled.
79-87: Windows Firebase configuration properly addedThe Windows FirebaseOptions constant is now properly defined with all required fields, completing the Firebase support for Windows platform. The structure follows the same pattern as other platform configurations, including the appropriate fields needed for Windows.
| # "Fixes firebase build error when no valid google-services.json is present" | ||
| - name: Comment out Google Play Services import | ||
| shell: bash | ||
| run: | | ||
| sed -i 's/id .com\.google\.gms\.google-services./\/\/ id .com\.google\.gms\.google-services./' android/app/build.gradle | ||
| echo "Google Play Services import commented out" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release builds should fail if Firebase is not configured correctly. I will fix the root cause shortly. This also happens with macOS or iOS (can't recall which it was).
Please also create tasks for (normal priority):
Ensure this behaviour is consistent across platforms.
Amend our Firebase local setup documentation to include instructions and/or a script in our CLI package script to "de-Google" the builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
macOS builds succeed without Firebase configs, but the app crashes on startup with a Firebase-related runtime exception.
Android builds are the only builds failing because of firebase configs currently, and because of the introduction of the com.google.gms.google-services import in this PR. The new import was added by flutterfire_cli.
All platforms, except for macOS, worked fine without valid firebase configs present - the runtime error appears to only be fatal on macOS. Linux is also not yet supported by flutterfire_cli/firebase.
defaults to building web first before iOS, ensuring that the required assets are downloaded
Summary by CodeRabbit
New Features
Chores