Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully integrates the watchOS companion app into the iOS project and automates its enablement for CI/CD builds. A new enable_watchos.sh script is introduced to modify the Xcode project file and Info.plist, and this script is correctly integrated into the Codemagic workflows. The WATCHOS_README.md has been updated to reflect these changes, providing clear instructions for both local development and understanding the CI/CD process. The changes are well-structured and align with best practices for managing Xcode project configurations in a CI/CD environment.
| CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES = YES; | ||
| CODE_SIGN_ENTITLEMENTS = "Runner/RunnerDebug-dev.entitlements"; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
Setting DEVELOPMENT_TEAM to an empty string here and in other build configurations relies heavily on the CI/CD system (Codemagic) to correctly inject the team ID during the build process. While this is a common practice, it's a critical configuration point. If the Codemagic setup for ios_signing were to change or be misconfigured, this could lead to build failures related to code signing. Consider adding a comment in the project.pbxproj itself or ensuring robust validation in the CI/CD to catch such issues early.
| CLANG_ALLOW_NON_MODULAR_INCLUDES_IN_FRAMEWORK_MODULES = YES; | ||
| CODE_SIGN_ENTITLEMENTS = "Runner/RunnerProfile-dev.entitlements"; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
Similar to the previous comment, setting DEVELOPMENT_TEAM to an empty string here for the Profile-dev configuration introduces a dependency on the CI/CD system to correctly handle code signing. Ensure that the Codemagic configuration is robust enough to always provide the correct development team for this build type.
| CODE_SIGN_IDENTITY = "Apple Development"; | ||
| CODE_SIGN_STYLE = Automatic; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
The DEVELOPMENT_TEAM is set to an empty string for the Release-dev configuration. This continues the pattern of relying on CI/CD for code signing. This is acceptable as long as the CI/CD environment is guaranteed to inject the correct team ID.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
The DEVELOPMENT_TEAM is set to an empty string for the Debug configuration of the omiWatchApp target. This is a critical setting for watchOS apps, as they have specific signing requirements. It's crucial that Codemagic correctly overrides this empty value with the appropriate development team ID to prevent build failures.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
For the Debug-prod configuration of omiWatchApp, CODE_SIGN_IDENTITY has been removed and DEVELOPMENT_TEAM set to an empty string. This makes the build entirely dependent on the CI/CD system's ability to inject correct signing identities and team IDs. This is a high-risk change if the CI/CD configuration is not perfectly aligned to handle these missing values, potentially leading to signing errors during the build.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
Similar to the Debug-prod configuration, the Profile-dev configuration for omiWatchApp also has CODE_SIGN_IDENTITY removed and DEVELOPMENT_TEAM emptied. This further emphasizes the reliance on CI/CD for proper code signing. Any discrepancy in the CI/CD's signing process for watchOS targets could cause build failures.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
The Release-dev configuration for omiWatchApp follows the same pattern of removing CODE_SIGN_IDENTITY and emptying DEVELOPMENT_TEAM. This consistent approach across watchOS build configurations highlights the critical dependency on the CI/CD system to manage code signing. Ensure that the Codemagic setup is thoroughly tested for all watchOS build types.
| CODE_SIGN_STYLE = Automatic; | ||
| CURRENT_PROJECT_VERSION = 1; | ||
| DEVELOPMENT_TEAM = 9536L8KLMP; | ||
| DEVELOPMENT_TEAM = ""; |
There was a problem hiding this comment.
The DEVELOPMENT_TEAM is set to an empty string for the Debug configuration of the main Runner target. This is consistent with the other changes but still represents a critical dependency on the CI/CD system to correctly provision the development team for code signing.
|
@mdmohsin7 There hasn’t been any activity on this draft for 3+ days; to keep the repo current, please close it for now and reopen when it’s ready. by AI for @beastoin |
|
Hey @mdmohsin7 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
2 participants
No description provided.