Skip to content

Conversation

@olilarkin
Copy link
Member

@olilarkin olilarkin commented Dec 19, 2025

Summary

  • Add CMake build system support for iPlug2OOS projects
  • Add GitHub Actions CI workflow for cross-platform CMake builds
  • Add build-cmake skill for Claude Code assistance

Changes

CMake Build System

  • Root CMakeLists.txt: Entry point that includes iPlug2 framework and adds TemplateProject
  • CMakePresets.json: Standardized presets aligned with iPlug2 framework conventions
    • macOS: macos-ninja, macos-make, macos-xcode, macos-xcode-universal
    • iOS: ios-xcode, ios-sim-xcode
    • visionOS: visionos-xcode, visionos-sim-xcode
    • Windows: windows-ninja, windows-vs2022, windows-vs2022-arm64ec
    • Web: wam (Emscripten/WebAssembly)
  • TemplateProject/CMakeLists.txt: Minimal CMake config using iplug_add_plugin()

CI Workflow (build-cmake.yml)

  • Default builds (on every push/PR): macOS Xcode + Windows VS2022
  • Optional builds via /ci commands or workflow_dispatch:
    • /ci ios - iOS Simulator build
    • /ci wam - Emscripten/WAM build
    • /ci universal - macOS Universal binary (arm64+x86_64)
    • /ci backends - IGraphics backend matrix (NanoVG/Skia variants)
    • /ci arm64ec - Windows ARM64EC build
    • /ci all-generators - Ninja + Make generators
    • /ci all - Everything
  • Artifact uploads for all build configurations
  • SDK caching for faster builds

Claude Code Integration

  • build-cmake skill: Documents preset-based workflow for building plugins

@claude
Copy link

claude bot commented Dec 19, 2025

Pull Request Review: Add CMake Build System

This is a well-structured PR that adds comprehensive CMake support to iPlug2OOS. The implementation is solid overall, with extensive CI testing. Here are my findings:


✅ Strengths

  1. Comprehensive CI Coverage: The GitHub Actions workflow tests multiple generators (Xcode, Ninja, Make), platforms (macOS, iOS, Emscripten), and graphics backends (NanoVG/Skia with different renderers).

  2. Good Documentation: The build-cmake skill provides clear, practical examples for users with different use cases.

  3. CMake Presets: Follows modern CMake best practices with proper preset inheritance and platform-specific configurations.

  4. Caching Strategy: The workflow uses GitHub Actions caching effectively for SDKs and Skia libraries.


⚠️ Issues & Recommendations

1. CMake Version Inconsistency (CRITICAL)

  • Issue: CMakeLists.txt:1 requires CMake 3.14, but CMakePresets.json:3-7 requires 3.21
  • Impact: Users with CMake 3.14-3.20 will fail when using presets
  • Recommendation: Bump CMakeLists.txt to cmake_minimum_required(VERSION 3.21) or lower the presets requirement

2. Deprecated Warnings Suppression

  • Issue: CMakeLists.txt:5-10 globally disables deprecated warnings
  • Impact: May hide important deprecation warnings in user code
  • Recommendation: Consider making this opt-in via a CMake option

3. Windows Jobs Commented Out

  • Issue: .github/workflows/test-cmake.yml:332-412 has Windows CI jobs disabled
  • Impact: No CI coverage for Windows builds
  • Recommendation: Either enable them or add a comment explaining why they're disabled

4. iOS Deployment Target Inconsistency

  • Issue: Skill doc says iOS 15.0, Presets use 14.0, Workflow uses 15.0
  • Impact: Confusion for users, potential compatibility issues
  • Recommendation: Standardize on 15.0 across all files

5. Hardcoded EMSDK Path in Presets

  • Issue: CMakePresets.json:120 uses $env{EMSDK} which may not be set
  • Impact: Web preset will fail if EMSDK environment variable is not set
  • Recommendation: Add a condition check or document the requirement

6. Missing Error Handling in Emscripten Patch

  • Issue: .github/workflows/test-cmake.yml:430-431 patches emcc.py without checking if it succeeds
  • Impact: Silent failures if the sed pattern doesn't match
  • Recommendation: Add error checking or at least a comment explaining why this patch is needed

🔒 Security

  • ✅ No security concerns identified
  • ✅ GitHub Actions uses pinned versions (@v4, @v5, @v14)
  • ✅ No secrets or credentials exposed

🧪 Test Coverage

Excellent coverage:

  • ✅ Multiple generators (Xcode, Ninja, Make)
  • ✅ Multiple platforms (macOS, iOS, Emscripten)
  • ✅ Graphics backend matrix testing
  • ✅ Universal binary verification
  • ✅ Artifact uploads for WAM builds
  • ❌ Windows testing disabled

📊 Summary

Overall Assessment: Strong PR with minor issues to address

Critical Issues: 1 (CMake version mismatch)
Important Issues: 4 (deprecated warnings, Windows CI, iOS version inconsistency, EMSDK path)

Recommendation: Approve with requested changes for the CMake version inconsistency and iOS deployment target standardization.


🔧 Suggested Action Items

Must fix before merge:

  • Resolve CMake version requirement mismatch
  • Standardize iOS deployment target to 15.0

Should fix before merge:

  • Either enable Windows CI or document why it's disabled
  • Make deprecated warnings suppression optional
  • Document required iPlug2 submodule changes

Great work on this comprehensive CMake integration! The testing infrastructure is particularly impressive.

@claude
Copy link

claude bot commented Dec 20, 2025

PR Review: CMake Build System Support

Thank you for this comprehensive CMake implementation. Overall, this is a well-structured addition that follows modern CMake best practices.

Overall Assessment

Grade: B+ (Very Good with room for improvement)

The implementation provides excellent cross-platform build support with a sophisticated preset system and comprehensive CI/CD pipeline.


Critical Issues

1. WAM Build Not Using Defined Preset

Location: .github/workflows/build-cmake.yml:701

The WAM CI build bypasses the wam preset defined in CMakePresets.json. It uses manual emcmake cmake instead of cmake --preset=wam. This creates maintenance issues as the preset configuration isn't validated in CI.

Recommendation: Use the preset to ensure consistency.

2. Emscripten Patch Needs Documentation

Location: .github/workflows/build-cmake.yml:682-684

The workflow modifies Emscripten source files to disable identifier validation, which is a security concern with no explanation.

Recommendations:

  • Add detailed comment explaining why this patch is necessary
  • Investigate root cause (likely invalid EXPORT_NAME setting)
  • Consider using build flags instead of source modification

3. Invalid Backend Combinations in CI Matrix

Location: .github/workflows/build-cmake.yml:474-491

The CI tests SKIA/GL2 combination, but documentation states Skia only supports GL3, METAL, and CPU.


Medium Priority Issues

4. Hardcoded Deployment Targets

iOS 15.0 is relatively recent and not documented. Consider making configurable.

5. Missing Environment Variable Validation

WAM preset depends on EMSDK variables without validation or helpful errors.

6. Code Signing Disabled for iOS/visionOS

Should be documented for users who need device deployment.


Strengths

  1. Excellent CMake Structure - Modern CMake 3.25+ features, proper preset inheritance
  2. Comprehensive Platform Coverage - macOS, Windows, iOS, visionOS, WebAssembly
  3. Sophisticated CI/CD - Smart caching, comment-triggered builds, concurrency control
  4. Quality Documentation - Comprehensive skill guide with clear examples
  5. Security Best Practices - Latest Actions versions, proper token scoping

Recommendations

Before Merge:

  1. Fix WAM preset usage in CI
  2. Document Emscripten patch
  3. Fix or document backend matrix combinations

Follow-up PRs:

  • Add inline comments to CMakeLists.txt
  • Make deployment targets configurable
  • Add troubleshooting guide
  • Add environment variable validation

Conclusion

This is high-quality work that significantly improves the build system. The issues are mostly documentation and consistency concerns. I recommend addressing the 3 critical issues before merge, then creating follow-up issues for medium-priority improvements.

Great job on this comprehensive CMake implementation!

@claude
Copy link

claude bot commented Dec 20, 2025

PR Review: Add CMake Build System

Great work on adding CMake support to iPlug2OOS! This is a significant enhancement that will improve the development experience. Here's my detailed review:

✅ Strengths

1. Excellent Documentation

  • The build-cmake skill documentation (.claude/skills/build-cmake/SKILL.md:1-175) is comprehensive and well-structured
  • Clear examples for all platforms and presets
  • Good coverage of graphics backend options and build targets

2. Well-Structured CMake Configuration

  • Clean separation of concerns with root CMakeLists.txt, presets, and project-specific configs
  • CMakeLists.txt:1-9 follows best practices with proper version requirement and modular structure
  • TemplateProject/CMakeLists.txt:1-18 demonstrates minimal, clear usage of iplug_add_plugin()

3. Comprehensive CI/CD

  • Extensive platform coverage (macOS, Windows, iOS, visionOS, ARM64EC, WAM)
  • Smart default builds with optional expansions via /ci commands
  • Good use of caching for SDKs and Skia libraries to improve build times
  • Proper concurrency control to prevent resource conflicts

4. Cross-Platform Support

  • All major platforms covered with appropriate presets
  • Universal binary support for macOS (arm64 + x86_64)
  • ARM64EC support for Windows (forward-looking)

🔍 Issues & Suggestions

1. GitHub Actions Workflow Truncation (Critical)

  • The build-cmake.yml file appears truncated in the diff at line 720
  • The comment mentions a patch for emcc.py but the code is cut off
  • Action Required: Verify the complete workflow file is present and functional

2. CMake Version Requirement

  • CMakeLists.txt:1 and CMakePresets.json:3-6 require CMake 3.25 (released Dec 2022)
  • This is relatively recent and might cause issues for users on older systems
  • Suggestion: Document minimum CMake version in README or add a check with helpful error message
  • Question: Is 3.25 actually required, or could a lower version work? Consider testing with 3.20 or 3.21

3. Error Handling in CI

  • The workflow uses test -d checks to verify bundles exist, but errors might be silent
  • Suggestion: Add more verbose error messages in verification steps

4. Security: Command Injection Risk (Medium)

  • build-cmake.yml:81 sanitizes comment body: tr -cd '[:alnum:][:space:]/=,_-'
  • However, the sanitized comment is still evaluated in bash conditionals
  • Current Risk: Low (sanitization is reasonable)
  • Best Practice: Consider using GitHub Actions expressions instead of bash string matching

5. Preset Configuration Considerations

  • CMakePresets.json uses environment variables for Emscripten (lines 174-176)
  • Suggestion: Add validation in CI to ensure EMSDK variables are set before attempting WAM builds

6. iOS Deployment Target

  • CMakePresets.json:85,101 sets iOS deployment to 15.0 (iOS 15)
  • This is quite recent (2021) and excludes older devices
  • Question: Is iOS 15 actually required? Document the rationale or consider lowering if possible

7. Windows ARM64EC Build

  • ARM64EC support is excellent forward-thinking
  • Suggestion: Add a note in documentation about ARM64EC being experimental and its use cases

8. Missing Test Coverage

  • The CI builds and verifies artifacts exist, but doesn't run any tests
  • Suggestion: Consider adding basic smoke tests (e.g., plugin validation, instantiation tests)

9. Artifact Retention

  • No retention policy specified for uploaded artifacts
  • Suggestion: Add retention-days to artifact uploads to manage storage costs (e.g., 7 days)

📋 Minor Issues

10. .vscode/settings.json

  • Only shows +3/-1 in diff, but content not visible
  • Suggestion: Ensure settings are project-appropriate and don't include personal preferences

11. Documentation Gaps

  • build-cmake skill mentions run-ios-simulator skill (line 56) but doesn't explain integration
  • Suggestion: Add cross-references to other skills or workflows

12. Cache Key Stability

  • SDK cache keys use file hash which is good
  • Risk: If SDKs themselves update but script doesn't, cache won't invalidate
  • Suggestion: Consider including SDK version in cache key if available

🎯 Performance Considerations

13. Parallel Builds

  • macOS Make preset correctly uses parallel jobs
  • Ninja automatically parallelizes
  • Optimization: Consider setting CMAKE_BUILD_PARALLEL_LEVEL for consistency

14. Build Matrix Strategy

  • fail-fast: false is used appropriately for backend matrix builds
  • This allows all combinations to complete even if one fails - good for CI

🔐 Security Assessment

  • No credential exposure detected
  • No arbitrary code execution vulnerabilities
  • Comment sanitization is reasonable
  • Overall: Security posture is good

📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐ Clean, well-structured
Documentation ⭐⭐⭐⭐⭐ Excellent skill documentation
CI/CD ⭐⭐⭐⭐ Comprehensive, needs file verification
Security ⭐⭐⭐⭐ Good practices, minor suggestions
Test Coverage ⭐⭐⭐ Builds verified, but no functional tests

✅ Recommended Actions Before Merge

  1. Critical: Verify build-cmake.yml is complete (appears truncated)
  2. Document CMake 3.25 requirement and rationale
  3. Add artifact retention policies
  4. Consider adding basic smoke tests
  5. Verify the iPlug2 submodule update is intentional (shows +1/-1)

🎉 Overall Assessment

This is a high-quality PR that adds substantial value to the project. The CMake integration is well-designed, the CI workflow is comprehensive, and the documentation is excellent. The issues identified are mostly minor refinements. With the critical items addressed (especially the workflow file completeness), this should be good to merge.

Great work! 🚀

@olilarkin olilarkin merged commit 27dc207 into master Dec 20, 2025
27 checks passed
@olilarkin olilarkin deleted the cmake-land branch December 20, 2025 11:10
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