Cumulative updates includes the iOS build support #8
Conversation
kurone-kito
added
bug
WalkthroughThe pull request introduces modifications across several workflow files in the GitHub Actions setup, enhancing CI processes for Unity packages and release management. A new workflow for handling pushes to non-main branches is added, alongside updates to existing workflows for release processes and deployment configurations. Additionally, various configuration files, including Changes
Possibly related PRs
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
README.md (2)
5-13: Enhance the new section with an introduction and more specific details.The new section "Difference from original template" provides valuable information about the enhancements made to the project. To improve it further:
- Consider adding a brief introductory sentence to provide context for the list of differences.
- The last bullet point "and more..." is vague. Consider either removing it or replacing it with specific improvements that haven't been mentioned.
Here's a suggested revision:
## 💭 Difference from original template +This template builds upon the original VPM project template, incorporating several enhancements to improve development workflow and code quality: + - Added some linters e.g. CSpell, EditorConfig, Markdownlint - Added some actions e.g. CodeQL, CodeRabbit.AI, Dependabot - Auto invoke the VPM registry update when the package is updated - Improved a deployment process with GPG signing - Improved a GitHub Actions for testing and building - Improved the git attributes and ignore files -and more... +- Enhanced support for iOS buildsThis revision provides more context and replaces the vague "and more..." with a specific improvement mentioned in the PR objectives.
Line range hint
15-54: Consider updating the "Setting up the Automation" sectionGiven the new features and improvements mentioned in the "Difference from original template" section, it might be beneficial to update the "Setting up the Automation" section. This would ensure that users have the most up-to-date information on how to leverage the new automation features, such as the automatic VPM registry update and improved GitHub Actions for testing and building.
Consider adding information about:
- How to set up and use the new linters (CSpell, EditorConfig, Markdownlint).
- How to leverage the new actions (CodeQL, CodeRabbit.AI, Dependabot).
- Any steps required to enable the automatic VPM registry update.
- How to use the improved GitHub Actions for testing and building.
This will provide a more comprehensive guide for users to take full advantage of the template's new features.
.github/workflows/release.yml (1)
Line range hint
1-105: Suggestions for workflow enhancementsWhile the workflow is well-structured and functional, consider the following improvements:
- Use variables for file extensions (e.g.,
ZIP_EXT=".zip") to make the workflow more maintainable and flexible.- Optimize the GPG signing process by creating a reusable step or job for signing multiple files.
- Enhance error handling and logging throughout the workflow to make troubleshooting easier.
- Consider adding a step to validate the created artifacts before release.
These enhancements could improve the workflow's maintainability, efficiency, and reliability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- .github/workflows/build-listing.yml (1 hunks)
- .github/workflows/push.yml (1 hunks)
- .github/workflows/release.yml (1 hunks)
- .tool-versions (1 hunks)
- Assets/XR/XRGeneralSettings.asset (2 hunks)
- Packages/vpm-manifest.json (1 hunks)
- ProjectSettings/ProjectSettings.asset (4 hunks)
- README.md (1 hunks)
- global.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .tool-versions
- global.json
🧰 Additional context used
🪛 actionlint
.github/workflows/push.yml
22-22: shellcheck reported issue in this script: SC2086:info:1:70: Double quote to prevent globbing and word splitting
(shellcheck)
22-22: shellcheck reported issue in this script: SC2086:info:2:56: Double quote to prevent globbing and word splitting
(shellcheck)
51-51: shellcheck reported issue in this script: SC2086:info:1:100: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (17)
Packages/vpm-manifest.json (4)
29-29: Update of com.vrchat.base version in its own entryThe version of
com.vrchat.basehas been updated to 3.7.1 in its own entry in thelockedsection, maintaining consistency with its reference incom.vrchat.worlds.
23-23: Version updates in locked section for VRChat packagesThe versions of
com.vrchat.worldsand its dependencycom.vrchat.basehave been updated to 3.7.1 in thelockedsection. This ensures consistency with the maindependenciessection.Let's verify if all VRChat-related packages are using the same version:
#!/bin/bash # Description: Verify consistency of VRChat package versions # Test: Extract and compare versions of VRChat packages jq -r '.locked | to_entries[] | select(.key | startswith("com.vrchat")) | "\(.key): \(.value.version)"' Packages/vpm-manifest.jsonAlso applies to: 25-25
18-21: Addition of dev.onevr.vrworldtoolkit to locked sectionThe
dev.onevr.vrworldtoolkitdependency (version 3.2.0) has been added to thelockedsection. This ensures that this specific version is used consistently across different environments.Let's verify if this package is compatible with the current VRChat SDK version:
9-10: Dependency version update for com.vrchat.worldsThe version of
com.vrchat.worldshas been updated from 3.7.0 to 3.7.1. This is a minor version update, which typically includes bug fixes and small improvements.To ensure compatibility, let's check for any breaking changes or important notes in the release documentation:
.github/workflows/push.yml (4)
1-8: LGTM: Workflow trigger and permissions are well-configured.The workflow trigger is correctly set to run on all branches except 'main', which is suitable for a CI workflow. The read-only permissions for contents follow the principle of least privilege, enhancing security.
19-20: LGTM: Repository checkout step is up-to-date.The use of
actions/checkout@v4ensures you're using the latest version of the checkout action.
35-40: Consider implications of not cleaning the checkout directory.While checking out the automation repository without cleaning (
clean: false) allows for faster checkouts, it may lead to potential conflicts if there are leftover files from previous runs.Ensure this approach doesn't cause issues in your specific use case. If you're certain that no conflicts will arise, you can ignore this comment.
43-49: LGTM: Caching configuration is well-structured.The caching step is using the latest version of the
actions/cacheaction and has a well-structured key based on the OS and relevant file hashes. This should provide effective caching for improved workflow performance..github/workflows/build-listing.yml (1)
50-51: Approve the new step with suggestions for improvementThe addition of the step to deploy the
global.jsonfile is a good improvement. However, consider the following suggestions:
- Add a comment explaining the purpose of this step for better maintainability.
- Implement error handling and verification to ensure the file exists and is copied successfully.
Here's a suggested improvement:
- name: Deploy the global.json file - run: cp ${{ github.workspace }}/global.json ${{ env.pathToCi }}/ + run: | + # Ensure global.json is available for subsequent steps + if [ -f "${{ github.workspace }}/global.json" ]; then + cp "${{ github.workspace }}/global.json" "${{ env.pathToCi }}/" + echo "global.json deployed successfully" + else + echo "Error: global.json not found in the workspace" + exit 1 + fiLet's verify the impact of this change on subsequent steps:
✅ Verification successful
Verification Successful
The use of
global.jsonin the cache key (hashFiles('**/global.json', '**/*.csproj')) confirms that deploying theglobal.jsonfile impacts the caching mechanism as intended. The review comment is accurate, and the suggested improvements are appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if global.json is used in subsequent steps # Test: Search for global.json usage in the workflow file rg --type yaml 'global\.json' .github/workflows/build-listing.ymlLength of output: 264
Assets/XR/XRGeneralSettings.asset (3)
45-45: LGTM! iPhone Settings updated correctly.The iPhone Settings are now properly linked to the new iPhone Providers, ensuring the correct XR configuration for iOS.
To ensure consistency, let's verify that other platform settings are similarly configured:
#!/bin/bash # Check if all platform settings reference their respective providers rg --type yaml "m_LoaderManagerInstance: \{fileID: [^\}]+\}"
Line range hint
1-108: LGTM! XRGeneralSettings structure is complete and consistent.The XRGeneralSettings asset now includes proper configuration for Standalone, Android, and iPhone platforms. The structure is consistent, and the new iPhone settings integrate well with the existing setup.
To ensure all necessary platforms are covered, let's verify the supported platforms in the project settings:
#!/bin/bash # Check the list of supported platforms in ProjectSettings rg --type yaml "BuildTargetPlatform:" ProjectSettings/ProjectSettings.asset
17-32: LGTM! New iPhone Providers added.The addition of iPhone Providers is a good step towards iOS support for XR in the project. The settings for automatic loading and running are safely set to false by default.
Consider if specific XR loaders for iOS need to be added to the empty Loaders array. You may want to verify if any iOS-specific XR SDKs or plugins are required for your project.
.github/workflows/release.yml (1)
63-63: Correct file extension separator addedThe addition of the period before the file extension in the
zipFilevariable assignment is correct and important. This change ensures that the zip file will have the proper.zipextension, aligning with standard file naming conventions and the naming pattern used for other files in this workflow. This correction prevents potential issues in subsequent steps that rely on the${{ env.zipFile }}variable, particularly during the file creation and release processes.ProjectSettings/ProjectSettings.asset (4)
380-475: Ensure all required iOS icon sizes are includedThe added icon configurations for the
iPhonebuild target include various sizes for both iPhone and iPad devices. Please verify that all mandatory icon sizes required by Apple's App Store guidelines are included, and that the specified dimensions are correct.
510-510: Review enabling Graphics Jobs on iOSEnabling Graphics Jobs (
m_GraphicsJobs: 1) for the iOS build target can improve performance but may lead to instability on certain devices. Ensure thorough testing is performed across all target iOS devices to confirm compatibility and stability.
839-839: Confirm scripting define symbols for iOSThe scripting define symbols for
iPhoneare set toUDONSHARP;UDON;VRC_SDK_VRCSDK3. Please verify that these symbols are appropriate for the iOS platform and that they do not introduce dependencies or code that is incompatible with iOS builds.
874-874: Validate API compatibility level for iOSThe
apiCompatibilityLevelPerPlatformforiPhoneis set to3, which corresponds to .NET Standard 2.0. Confirm that all code and third-party libraries used in the project are compatible with this API level to prevent potential runtime issues.
| build: | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| listPublishDirectory: Website | ||
| packagePath: Packages/${{ vars.PACKAGE_NAME }} | ||
| pathToCi: ci | ||
| unityPackage: | ||
| zipFile: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a single step for environment variable setup.
While the current setup is functional, it would be more maintainable to declare all environment variables in a single step. This approach would centralize configuration and make it easier to manage variables.
Consider refactoring the environment variable setup as follows:
env:
listPublishDirectory: Website
packagePath: Packages/${{ vars.PACKAGE_NAME }}
pathToCi: ci
unityPackage: ${{ vars.PACKAGE_NAME }}-feature.unitypackage
zipFile: ${{ vars.PACKAGE_NAME }}-feature.zipThis change eliminates the need for the separate step to configure environment variables (current lines 21-24).
| - name: Zip the Package for testing | ||
| working-directory: "${{ env.packagePath }}" | ||
| run: zip -r "${{ github.workspace }}/${{ env.zipFile }}" . |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve zip command with consistent environment variable usage.
The zip command could be improved by using environment variables more consistently.
Consider updating the zip command as follows:
- name: Zip the Package for testing
run: zip -r "${{ github.workspace }}/${{ env.zipFile }}" .
working-directory: ${{ env.packagePath }}This change ensures consistent use of environment variables and improves readability.
| - name: Build a list of .meta files for future use | ||
| run: find "${{ env.packagePath }}/" -name \*.meta >> metaList |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of .meta file list creation.
The current method of creating the .meta file list could be made more robust.
Consider updating the step as follows:
- name: Build a list of .meta files for future use
run: find "${{ env.packagePath }}" -name '*.meta' -type f > metaListThis change ensures only files (not directories) are included and uses single quotes to prevent potential issues with shell expansion.
| - name: Deploy the global.json file | ||
| run: cp ${{ github.workspace }}/global.json ${{ env.pathToCi }}/ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use environment variables consistently in global.json deployment.
The global.json deployment step uses a hardcoded path, which is inconsistent with the use of environment variables elsewhere in the workflow.
Consider updating the step as follows:
- name: Deploy the global.json file
run: cp "${{ github.workspace }}/global.json" "${{ env.pathToCi }}/"This change ensures consistent use of environment variables and improves maintainability.
| - name: Build Package Version Listing with Nuke | ||
| run: ${{ env.pathToCi }}/build.cmd BuildRepoListing --root ${{ env.pathToCi }} --list-publish-directory $GITHUB_WORKSPACE/${{ env.listPublishDirectory }} --current-package-name ${{ vars.PACKAGE_NAME }} | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Address potential shell quoting issues in the build command.
The build command may have shell quoting issues that could lead to unexpected behavior.
Consider updating the build step as follows to ensure proper quoting:
- name: Build Package Version Listing with Nuke
run: '${{ env.pathToCi }}/build.cmd BuildRepoListing --root "${{ env.pathToCi }}" --list-publish-directory "$GITHUB_WORKSPACE/${{ env.listPublishDirectory }}" --current-package-name "${{ vars.PACKAGE_NAME }}"'
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}This change ensures that all arguments are properly quoted, preventing potential issues with spaces or special characters in paths or names.
🧰 Tools
🪛 actionlint
51-51: shellcheck reported issue in this script: SC2086:info:1:100: Double quote to prevent globbing and word splitting
(shellcheck)
Features
CI/CD
Documentation
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependency Updates
Bug Fixes