Skip to content

Conversation

@olilarkin
Copy link
Member

No description provided.

@olilarkin olilarkin mentioned this pull request Mar 2, 2025
@olilarkin olilarkin changed the title Update the macOS github actions workflow Update the macOS GHA workflow Mar 2, 2025
@olilarkin
Copy link
Member Author

@claude

@claude
Copy link

claude bot commented Dec 9, 2025

Claude finished @olilarkin's task —— View job


PR Review: Update the macOS GHA workflow

I've reviewed the changes in this PR. Here's my comprehensive analysis:

Tasks

  • Read and analyze changed files
  • Review new macOS workflow file
  • Review modifications to existing files
  • Provide comprehensive feedback

Summary

This PR introduces a dedicated macOS GitHub Actions workflow with comprehensive caching, SDK management, and validation testing. The changes improve CI efficiency and testing coverage for macOS builds.

Overall Assessment

Strong improvements - This is a well-structured workflow with good caching strategy and comprehensive testing. A few minor suggestions below.


Detailed Review

1. New Workflow: .github/workflows/build-mac.yml

Strengths:

  • Excellent caching strategy for dependencies, VST3 SDK, CLAP SDK, and pluginval
  • Proper separation of build and test jobs for parallelization
  • Comprehensive validation using both VST3 validator, pluginval, and auval
  • Good use of cache keys with version/hash dependencies
  • Uploads both artifacts and test results

Suggestions:

Line 26: Consider updating to actions/checkout@v4 for consistency with build-native.yml:29

- uses: actions/checkout@v4  # instead of v3.3.0

Lines 32, 48, 63, 118, 133: Consider updating to actions/cache@v4 for consistency with artifact actions

- uses: actions/cache@v4  # instead of v3

Line 82: The base64 decode and unzip could fail silently. Consider adding error handling:

echo $VST2_SDK | base64 -d > tmp/tmp.zip || exit 1
unzip tmp/tmp.zip -d tmp || exit 1

Line 129: Typo in unzip command - missing .zip extension:

unzip pluginval.zip  # instead of: unzip pluginval

Lines 147, 155: Exit codes are good, but consider if you want to continue on pluginval failure or stop immediately. Current behavior stops on first failure which is probably desired.

Lines 162-165: The commented RTSafe test - consider adding a TODO or explanation of why it's disabled, or remove if not needed.


2. Modified: .github/workflows/build-native.yml

Changes:

  • Updated actions/checkout from v3.3.0 to v4 (line 29)
  • Updated actions/upload-artifact from v3 to v4 (lines 64, 115)

These are good dependency updates for consistency.


3. Modified: .gitignore

Changes:

  • Added build-mac/, build-win/, build-web/ to ignore list
  • Improved formatting/organization

Clean and appropriate changes.


4. Modified: TemplateProject/scripts/makedist-mac.sh

Key Changes:

  • Line 158: Improved xcodebuild error handling with PIPESTATUS check
  • Lines 365-380: New "out" folder preparation for CI artifact collection
  • Organized dSYM and auval packaging

Strengths:

  • Better error handling and exit codes for CI
  • Clear separation of CI outputs into build-mac/out folder
  • Proper packaging of debug symbols and validation tools

Minor note:

  • Line 158: The xcpretty formatting is nice but the commented && exit ${PIPESTATUS[0]} suggests this may have been intentional. Current implementation is better.

5. Modified: Xcode Projects

macOS project (TemplateProject-macOS.xcodeproj/project.pbxproj):

  • Removed BuildIndependentTargetsInParallel = YES setting (as mentioned in commit 2aefb3d)
  • Changed All target dependency from APP to AUv3App
  • Reordered dependencies

iOS project (TemplateProject-iOS.xcodeproj/project.pbxproj):

  • Removed BuildIndependentTargetsInParallel = YES setting

Rationale: The removal of BuildIndependentTargetsInParallel suggests it was causing issues. The switch from APP to AUv3App in the All target aligns with commit 0de216e and likely reflects the actual testing/validation needs.


Recommendations

  1. Update action versions in build-mac.yml for consistency (checkout@v4, cache@v4)
  2. Fix the unzip typo on line 129 of build-mac.yml
  3. Add error handling to VST2 SDK extraction (line 82)
  4. Consider documenting why RTSafe auval test is commented out, or remove if not needed

Testing Suggestions

Before merging, verify:

  • Cache restoration works correctly across runs
  • VST2_SDK secret is properly configured in repository settings
  • All validation tests pass (pluginval, VST3 validator, auval)
  • Artifact uploads contain expected files
  • The unzip command fix works correctly (currently has bug)

Overall, this is a solid improvement to the CI infrastructure. The new workflow is well-designed with excellent caching and comprehensive testing. The minor issues noted above are easy fixes.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants