fix: CI#3206
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements disk space optimizations across CI workflows to address disk space constraints in GitHub Actions runners. The changes focus on aggressive cleanup strategies, updated build environments, and resource management improvements.
Key Changes:
- Added disk space cleanup steps to free up storage on Ubuntu and Rocky Linux runners by removing unnecessary system files, build artifacts, and caches
- Updated macOS runner from macos-13 to macos-14 and upgraded GCC from version 10 to version 13
- Modified Git fetch depth and added binutils package installation for binary stripping operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| working-directory: ${{ github.workspace }}/build | ||
| run: | | ||
| rm -rf Testing CMakeFiles CMakeCache.txt | ||
| find . -name "*.o" -o -name "*.a" -delete |
There was a problem hiding this comment.
The pattern find . -name "*.o" -o -name "*.a" -delete may not work as expected. When using -o (OR) with -delete, the find command can exhibit unexpected behavior because -delete has lower precedence. Use -o within parentheses or use separate find commands for each file type to ensure both patterns are properly deleted.
| find . -name "*.o" -o -name "*.a" -delete | |
| find . \( -name "*.o" -o -name "*.a" \) -delete |
| cmake -B build -DCMAKE_C_COMPILER=/usr/local/opt/gcc@10/bin/gcc-10 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache | ||
| GCC_PREFIX=$(brew --prefix gcc@13) | ||
| export CC=$GCC_PREFIX/bin/gcc-13 | ||
| cmake -B build -DCMAKE_C_COMPILER=$GCC_PREFIX/bin/gcc-13 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache |
There was a problem hiding this comment.
The CMAKE configuration only specifies CMAKE_C_COMPILER but not CMAKE_CXX_COMPILER. For a C++ project (indicated by DCMAKE_CXX_FLAGS_DEBUG), both compilers should be explicitly set to ensure consistent toolchain usage. Add -DCMAKE_CXX_COMPILER=$GCC_PREFIX/bin/g++-13 to match the C compiler specification.
| cmake -B build -DCMAKE_C_COMPILER=$GCC_PREFIX/bin/gcc-13 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache | |
| cmake -B build -DCMAKE_C_COMPILER=$GCC_PREFIX/bin/gcc-13 -DCMAKE_CXX_COMPILER=$GCC_PREFIX/bin/g++-13 -DUSE_PIKA_TOOLS=ON -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} -DCMAKE_CXX_FLAGS_DEBUG=-fsanitize=address -D CMAKE_C_COMPILER_LAUNCHER=ccache -D CMAKE_CXX_COMPILER_LAUNCHER=ccache |
| go clean -cache -testcache | ||
| rm -rf "$HOME/.cache/go-build" "$HOME/go/pkg/mod" |
There was a problem hiding this comment.
The Go cache cleanup occurs before the "Run Go E2E Tests" step (which starts at line 243). This means the E2E tests will have to rebuild the Go cache from scratch, significantly slowing down the test execution. Move this cleanup to after all Go-related tests are complete, or remove it entirely if disk space permits.
| @@ -256,6 +286,7 @@ jobs: | |||
| cp deps/lib/libz.1.dylib . | |||
| cp deps/lib/libz.1.dylib tests/integration/ | |||
| rm -rf ./buildtrees | |||
There was a problem hiding this comment.
The sed command modifies test files to replace 'exec leaks' with 'exec echo "0 leaks"', effectively disabling memory leak detection in tests. While this may be intentional to avoid false positives or macOS-specific issues, consider documenting why leak detection is being disabled, as this could hide real memory leaks in the codebase.
| rm -rf ./buildtrees | |
| rm -rf ./buildtrees | |
| # Disable memory leak detection on macOS tests by replacing 'exec leaks' with a no-op. | |
| # This is done to avoid persistent false positives or compatibility issues with the 'leaks' tool on macOS CI. | |
| # See https://github.com/pikadb/pika/issues/XXXX for more details. (Replace with actual issue if available.) |
| find ./build -name "*.a" -type f -delete | ||
|
|
||
| echo "Stripping debug symbols from binaries..." | ||
| find ./build -type f -executable -not -name "*.sh" -exec strip --strip-debug {} \; || true |
There was a problem hiding this comment.
The strip command is used here, but it may fail if binutils is not properly installed or if the package name differs on Rocky Linux 9. The || true suffix masks potential failures. Consider verifying that the strip command is available after installing binutils, or use a conditional check before running strip to ensure proper error handling.
| find ./build -type f -executable -not -name "*.sh" -exec strip --strip-debug {} \; || true | |
| if command -v strip >/dev/null 2>&1; then | |
| find ./build -type f -executable -not -name "*.sh" -exec strip --strip-debug {} \; | |
| else | |
| echo "Warning: 'strip' command not found. Skipping stripping debug symbols." | |
| fi |
| rm -rf .git | ||
| rm -rf include | ||
| rm -rf docs |
There was a problem hiding this comment.
The cleanup step removes the .git directory and essential project directories (include, docs) which could break subsequent steps that need to access the git history or project files. These deletions happen before the Test and Unit Test steps, which may depend on these files. Consider moving these aggressive cleanup operations to after all tests and operations are complete, or verify that they won't break subsequent steps.
No description provided.