Use macos-latest #690
Use macos-latest #690
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes made in the CI workflow configuration involve the introduction of an environment variable Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci v2.yml (1)
27-27: Document runner version strategy in workflowConsider adding a comment in the workflow file documenting the runner version strategy and the rationale for using
macos-latest. This will help future maintainers understand the decision and manage version-related issues effectively.Add a comment like this at the top of the file:
name: CI v2 +# Runner Version Strategy: +# - Using macos-latest to automatically stay on supported versions +# - Monitor runner deprecation announcements: https://github.com/actions/runner-images/issuesAlso applies to: 140-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/ci v2.yml(2 hunks)
🔇 Additional comments (2)
.github/workflows/ci v2.yml (2)
140-140: Ensure cache compatibility across macOS versions
The change to macos-latest in the tests job should be compatible with the cached dependencies. Monitor the cache hit rates and build times to ensure there are no negative impacts from version mismatches.
✅ Verification successful
After analyzing the workflow file, I can provide the final response:
Cache configuration is properly handled for macOS
The cache configuration in the workflow is well-designed and compatible with macOS version changes:
- The cache key includes the OS-specific name (
ci-macos), build type, and content hashes ofconanfile.pyandCMakePresets.json - The cache paths correctly include both
~/.conan2and/Users/runner/.conan2/ - The restore-keys fallback is properly configured
- The cache is used consistently across both the
cache_depsandtestsjobs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any cache-related issues in recent workflow runs
gh api graphql -f query='
{
repository(owner: "victimsnino", name: "reactive-pp") {
workflowRuns(first: 10, orderBy: {field: CREATED_AT, direction: DESC}) {
nodes {
workflow {
name
}
jobs(first: 10) {
nodes {
name
steps(first: 20) {
nodes {
name
conclusion
}
}
}
}
}
}
}
}'
Length of output: 770
Script:
#!/bin/bash
# Let's check the cache configuration in the workflow file and any related cache settings
cat .github/workflows/ci\ v2.yml
Length of output: 8396
27-27: Consider the trade-offs of using macos-latest
While using macos-latest ensures you're always on a supported runner version, it may impact build reproducibility as the runner version can change automatically. Consider documenting this decision and monitoring for potential issues during version transitions.
✅ Verification successful
Let me try a different approach to verify the macOS runner configuration and its potential impact.
Based on the search results, I can now generate the final response since I have enough context about the macOS runner usage in the workflow:
Switching to macos-latest is a reasonable choice
The change from macos-12 to macos-latest appears safe for this workflow because:
- The macOS runner is used in standard ways for caching dependencies and running tests
- No macOS-specific conditional logic or special handling was found
- The workflow doesn't rely on version-specific macOS features
The automatic version updates from macos-latest should not impact the build stability as the workflow uses standard CI operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check current macos-latest version and upcoming deprecations
gh api \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
/repos/actions/runner-images/contents/images/macos/macos-latest.md | jq -r '.content' | base64 -d
# Check if there are any existing issues related to macos runner version
gh api graphql -f query='
{
search(query: "repo:victimsnino/reactive-pp is:issue macos runner", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
state
}
}
}
}'
Length of output: 557
Script:
#!/bin/bash
# Check the workflow file for any specific macOS dependencies or configurations
rg -A 5 "macos" .github/workflows/ci\ v2.yml
# Check if there are any macOS-specific build or test configurations
rg -l "darwin|macos|osx" --type yaml
# Look for any conditional logic based on runner OS
rg "if: runner.os == 'macOS'" --type yaml
Length of output: 734
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/analyzers.yml (1)
113-114: LGTM! Consider matching cache fallback strategy.The multi-line YAML format is correct, but for consistency and better cache utilization, consider matching the fallback strategy suggested for the
pvsjob.Consider expanding the restore-keys:
restore-keys: | deps-ci-ubuntu-clang-Release-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }} + deps-ci-ubuntu-clang-Release-${{ hashFiles('**/conanfile.py') }}- + deps-ci-ubuntu-clang-Release-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/analyzers.yml(2 hunks).github/workflows/ci v2.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci v2.yml
55-55: undefined variable "CACHE_INDEX". available variables are "env", "github", "inputs", "job", "matrix", "needs", "runner", "secrets", "steps", "strategy", "vars"
(expression)
🔇 Additional comments (3)
.github/workflows/analyzers.yml (1)
41-41: Verify cache key structure and fallback strategy.
The cache key now includes hashes of both dependency files, which improves specificity but might reduce cache hits. Consider adding fallback keys for partial matches.
Consider adding additional restore-keys for better cache utilization:
- restore-keys: deps-ci-ubuntu-clang-Release-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}
+ restore-keys: |
+ deps-ci-ubuntu-clang-Release-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}
+ deps-ci-ubuntu-clang-Release-${{ hashFiles('**/conanfile.py') }}-
+ deps-ci-ubuntu-clang-Release-This provides a fallback strategy when exact matches aren't found.
.github/workflows/ci v2.yml (2)
114-114: LGTM! Cache configuration improvements
The changes to cache keys and restore-keys are well thought out:
- Including dependency file hashes ensures cache accuracy
- Consistent pattern across jobs improves maintainability
- Cache index allows for controlled cache invalidation when needed
Also applies to: 191-191
29-29: Consider documenting macOS version requirements
While using macos-latest reduces maintenance overhead, it may introduce unexpected behavior when the runner version changes. Consider:
- Documenting minimum macOS version requirements
- Adding CI checks to verify compatibility across different macOS versions
Run this script to check the current macOS version and available runners:
Also applies to: 140-140
✅ Verification successful
Using macos-latest is appropriate for this project
Based on the codebase analysis, using macos-latest is suitable because:
- The project already handles cross-platform compatibility through CMake configurations
- No strict macOS version requirements are specified in the codebase
- The only macOS-specific requirement mentioned in Readme.md is "Apple Clang 14", which is available across recent macOS versions
The change from macos-12 to macos-latest is safe and reduces maintenance overhead without introducing significant risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check current macOS version and available runners
# Get current macOS version from latest runner
gh api repos/actions/runner-images/contents/images/macos/macos-latest.json | jq -r '.content' | base64 -d | jq -r '.os_version'
Length of output: 247
Script:
#!/bin/bash
# Let's check if there are any specific macOS version requirements or dependencies in the codebase
# Search for macOS version related patterns in the codebase
rg -i "macos|darwin" -g "!{.git,node_modules}" -A 2
# Look for any package.json or similar configuration files that might specify OS requirements
fd "package.json|requirements.txt|Cargo.toml|*.podspec" --type f --exec cat {}
Length of output: 2722
.github/workflows/ci v2.yml
Outdated
| env: | ||
| CACHE_INDEX: 1 |
There was a problem hiding this comment.
Fix environment variable reference syntax
The CACHE_INDEX environment variable is incorrectly referenced in the cache key. GitHub Actions requires environment variables to be accessed through the env context.
Apply this change to fix the syntax:
- key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{CACHE_INDEX}}
+ key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{ env.CACHE_INDEX }}Also applies to: 55-55
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci v2.yml (1)
29-29: Consider implications of using macos-latestWhile using
macos-latestkeeps the runner up-to-date, it might introduce unexpected behavior when GitHub updates the macOS version. Consider:
- Documenting the minimum required macOS version
- Testing the workflow when GitHub announces new macOS images
Also applies to: 142-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/analyzers.yml(5 hunks).github/workflows/ci v2.yml(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/analyzers.yml
🔇 Additional comments (2)
.github/workflows/ci v2.yml (2)
55-55:
Fix environment variable reference syntax in cache keys
The CACHE_INDEX environment variable is incorrectly referenced in the cache keys. GitHub Actions requires environment variables to be accessed through the env context.
Apply this change to all cache keys:
-key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{CACHE_INDEX}}
+key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{ env.CACHE_INDEX }}Also applies to: 116-116, 194-194
19-20: Ensure consistent environment variable syntax usage
The environment variable declaration is correct, but ensure it's consistently referenced using ${{ env.CACHE_INDEX }} syntax throughout the workflow.
✅ Verification successful
Environment variable syntax is correctly used throughout the workflow
The environment variable CACHE_INDEX is properly referenced using the correct syntax ${{env.CACHE_INDEX}} consistently across all its usages in cache keys and cache-key-prefix configurations. No syntax issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for incorrect env var references
rg -l '\$\{\{CACHE_INDEX\}\}' || rg -l '\$CACHE_INDEX'
Length of output: 55
Script:
#!/bin/bash
# Check for all references to CACHE_INDEX in the workflow file
rg -A 2 -B 2 'CACHE_INDEX' '.github/workflows/ci v2.yml'
Length of output: 1304
.github/workflows/ci v2.yml
Outdated
| lookup-only: true | ||
|
|
||
| - name: Install Qt | ||
| if: steps.check_cache.outputs.cache-hit != 'true' || github.event_name == 'schedule' | ||
| uses: jurplel/install-qt-action@v4 | ||
| with: | ||
| cache: true | ||
| cache-key-prefix: ${{env.CACHE_INDEX}} |
There was a problem hiding this comment.
Fix environment variable reference syntax in cache key prefixes
The CACHE_INDEX environment variable is incorrectly referenced in the Qt installation cache key prefixes.
Apply this change to all Qt installation steps:
-cache-key-prefix: ${{CACHE_INDEX}}
+cache-key-prefix: ${{ env.CACHE_INDEX }}Also applies to: 105-105, 183-183, 263-263
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/analyzers.yml(5 hunks).github/workflows/ci v2.yml(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/analyzers.yml
🔇 Additional comments (3)
.github/workflows/ci v2.yml (3)
19-20: LGTM: Good cache versioning strategy
Adding a cache version index is a good practice to invalidate old caches when making significant changes to the build process or dependencies.
55-55:
Fix environment variable reference syntax in cache keys
The CACHE_INDEX environment variable is incorrectly referenced. GitHub Actions requires environment variables to be accessed through the env context.
Apply this change to all cache key definitions:
-key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{CACHE_INDEX}}
+key: deps-${{ matrix.config.name }}-${{ matrix.build_type.config }}-${{ hashFiles('**/conanfile.py') }}-${{ hashFiles('**/CMakePresets.json') }}-${{ env.CACHE_INDEX }}Also applies to: 116-116, 194-194
63-63:
Fix environment variable reference syntax in Qt installation cache key prefixes
The CACHE_INDEX environment variable is incorrectly referenced in the Qt installation cache key prefixes.
Apply this change to all Qt installation steps:
-cache-key-prefix: ${{CACHE_INDEX}}
+cache-key-prefix: ${{ env.CACHE_INDEX }}Also applies to: 105-105, 183-183, 263-263
| @@ -24,7 +26,7 @@ jobs: | |||
| config: [{name: ci-ubuntu-gcc, os: ubuntu-latest}, | |||
| {name: ci-ubuntu-clang, os: ubuntu-latest}, | |||
| {name: ci-windows, os: windows-latest}, | |||
| {name: ci-macos, os: macos-12}] | |||
| {name: ci-macos, os: macos-latest}] | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the output, I can generate the final response as we have sufficient information about the available macOS runners. The script results show that GitHub Actions currently supports macOS versions 12 through 15, including ARM64 variants for versions 13-15.
Specify macOS version 14 instead of using latest
Using macos-latest may lead to unexpected behavior when GitHub automatically updates the runner. Based on the available runners, it's recommended to use macos-14 as it represents a stable, recent version while maintaining predictability in your CI pipeline.
- Update line 29: Replace
macos-latestwithmacos-14 - Update line 142: Replace
macos-latestwithmacos-14
🔗 Analysis chain
Consider using a specific macOS version instead of latest
While macos-latest automatically provides the most recent stable runner, it may introduce unexpected behavior when GitHub updates the runner version. Consider using a specific version (e.g., macos-13) for better stability and predictability.
Run this script to check the current macOS version and available options:
Also applies to: 142-142
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check current macOS runner version and available options
# Get current macos-latest version
gh api \
-H "Accept: application/vnd.github+json" \
/repos/actions/runner-images/contents/images/macos/macos-latest.json | jq -r '.content' | base64 -d | jq '.os_version'
# List available macOS runner versions
gh api \
-H "Accept: application/vnd.github+json" \
/repos/actions/runner-images/contents/images/macos | jq -r '.[].name' | grep macos
Length of output: 570
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
conanfile.py (1)
50-52: Consider version range for Qt dependencyThe Qt requirement is pinned to exactly 5.15.14. Consider using a version range to allow for patch updates.
- self.requires("qt/5.15.14", transitive_libs=True) + self.requires("qt/[>=5.15.14 <5.16]", transitive_libs=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/analyzers.yml(3 hunks).github/workflows/ci v2.yml(6 hunks)cmake/variables.cmake(1 hunks)conanfile.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/analyzers.yml
- .github/workflows/ci v2.yml
🔇 Additional comments (3)
conanfile.py (2)
15-16: LGTM: Qt option follows existing pattern
The new with_qt option is consistently implemented following the existing pattern of optional features.
24-26: Consider version constraints for Qt shared library setting
While the Qt integration looks good, the qt:shared setting might need version-specific handling.
Consider documenting the rationale for forcing shared library mode in the comments, as this might affect downstream projects.
✅ Verification successful
Qt shared library setting is correctly configured
The qt:shared setting is properly configured in the conanfile.py. The file shows that Qt is only included when with_qt option is enabled, and when included, it uses Qt version 5.15.14 with transitive libraries. The shared library setting is defined as a default option, which is a standard and supported configuration for Qt in Conan.
- The shared library setting is defined in the default options section, which is the correct location
- Qt dependency is properly versioned (5.15.14) and includes transitive libraries
- The configuration follows Conan's best practices for Qt integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Qt version 5.15.14 supports shared library option consistently
rg -A 5 "qt.*5\.15\.14.*shared" || rg -A 5 "qt.*shared.*5\.15\.14"
Length of output: 67
Script:
#!/bin/bash
# Let's first check the conanfile.py content to understand the Qt dependency configuration
cat conanfile.py
# Also search for any Qt-related settings or requirements
rg -l "requires.*qt" || rg -l "qt.*requires"
Length of output: 1667
cmake/variables.cmake (1)
111-113: LGTM: Qt integration follows existing patterns
The Qt integration in CMake is consistently implemented, following the same pattern as other optional features like SFML and GRPC.
However, let's verify the integration between CMake and Conan settings:
✅ Verification successful
Qt integration is properly configured and consistent
The verification confirms that the Qt integration is correctly implemented and consistent across the codebase:
- CMake option
RPP_BUILD_QT_CODEis properly defined with clear documentation in BUILDING.md - Conan option
with_qtis correctly defined in conanfile.py with proper Qt dependency - The CMake-to-Conan bridge in variables.cmake correctly sets the Conan option when Qt is enabled
- Dependencies.cmake properly handles Qt package discovery
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency between CMake and Conan Qt settings
echo "Checking CMake-Conan Qt integration consistency..."
# Look for Qt-related settings in CMake files
echo "CMake Qt settings:"
rg -l "RPP_BUILD_QT_CODE"
# Look for corresponding Conan settings
echo "Conan Qt settings:"
rg -l "with_qt.*True"
Length of output: 462
Script:
#!/bin/bash
# Let's check the actual implementation details
# Check conanfile.py Qt options
echo "=== Conan Qt option definition ==="
rg "with_qt" conanfile.py -B 2 -A 2
# Check CMake Qt option definition
echo -e "\n=== CMake Qt option definition ==="
rg "RPP_BUILD_QT_CODE" cmake/variables.cmake -B 2 -A 2
# Check how Qt is used in dependencies
echo -e "\n=== Qt usage in dependencies.cmake ==="
rg "RPP_BUILD_QT_CODE" cmake/dependencies.cmake -B 2 -A 2
# Check documentation
echo -e "\n=== Qt build documentation ==="
rg "RPP_BUILD_QT_CODE" BUILDING.md -B 2 -A 2
Length of output: 2340
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cmake/dependencies.cmake (2)
51-57: Improve Qt version detection error handling.While the Qt version detection logic is good, consider these improvements:
- Add REQUIRED to Qt6 find_package to fail fast if specified Qt6 is required
- Add explicit error message when neither Qt5 nor Qt6 is found
- find_package(Qt6 COMPONENTS Widgets Core Gui QUIET) + find_package(Qt6 COMPONENTS Widgets Core Gui ${RPP_QT6_REQUIRED}) if (Qt6_FOUND) SET(RPP_QT_TARGET Qt6) else() message("-- RPP: Can't find Qt6, searching for Qt5...") - find_package(Qt5 REQUIRED COMPONENTS Widgets Core Gui) - SET(RPP_QT_TARGET Qt5) + find_package(Qt5 COMPONENTS Widgets Core Gui) + if (Qt5_FOUND) + SET(RPP_QT_TARGET Qt5) + else() + message(FATAL_ERROR "Neither Qt6 nor Qt5 was found. Please install Qt development files.") + endif() endif()
Line range hint
61-69: Refactor Windows-specific DLL copying for better maintainability.The current implementation repeats similar commands for each Qt component. Consider creating a list of components and using a foreach loop.
if (WIN32) - add_custom_command (TARGET ${TARGET} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different $<TARGET_FILE:${RPP_QT_TARGET}::Core> $<TARGET_FILE_DIR:${TARGET}>) - add_custom_command (TARGET ${TARGET} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different $<TARGET_FILE:${RPP_QT_TARGET}::Widgets> $<TARGET_FILE_DIR:${TARGET}>) - add_custom_command (TARGET ${TARGET} POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy_if_different $<TARGET_FILE:${RPP_QT_TARGET}::Gui> $<TARGET_FILE_DIR:${TARGET}>) + set(QT_COMPONENTS Core Widgets Gui) + foreach(COMPONENT ${QT_COMPONENTS}) + add_custom_command( + TARGET ${TARGET} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + $<TARGET_FILE:${RPP_QT_TARGET}::${COMPONENT}> + $<TARGET_FILE_DIR:${TARGET}> + ) + endforeach() endif()
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci v2.yml (1)
86-88: Consider standardizing Python version across jobsThe workflow uses different Python version specifications across jobs:
- Most jobs use '3.6.x - 3.11.x'
- The docs job uses a fixed "3.8"
Consider standardizing the Python version across all jobs for better consistency and maintainability.
Also applies to: 162-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/analyzers.yml(5 hunks).github/workflows/ci v2.yml(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/analyzers.yml
🔇 Additional comments (3)
.github/workflows/ci v2.yml (3)
19-20: LGTM: Environment variable addition for cache management
The addition of the CACHE_INDEX environment variable at the workflow level is a good practice for cache management and invalidation.
29-29: Use specific macOS version instead of latest
Using macos-latest may lead to unexpected behavior when GitHub automatically updates the runner. It's recommended to use macos-14 for better stability and predictability in your CI pipeline.
Also applies to: 125-125
55-55: Fix environment variable reference syntax in cache keys
The CACHE_INDEX environment variable is incorrectly referenced in the cache keys. GitHub Actions requires environment variables to be accessed through the env context.
Also applies to: 99-99, 175-175
Summary by CodeRabbit
CACHE_INDEX.