Optimize cross platform check#3890
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Pull request overview
This PR optimizes the cross-platform build check workflow by introducing conditional build strategies based on the event type. For pull requests, it runs only a quick syntax check (cargo check) to minimize disk usage, while full debug builds are reserved for the main branch. The changes also add disk space monitoring, build failure diagnostics, and attempt to optimize caching configuration.
Key Changes
- Conditional build strategy:
cargo checkfor PRs vs full build for main branch - Added disk space monitoring before/after builds
- Configured cargo environment variables to limit build jobs and improve network retry behavior
| # Cleanup on success to save disk space for other jobs | ||
| - name: Cleanup build artifacts (Main Branch Only) | ||
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' | ||
| run: | | ||
| echo "Cleaning up build artifacts to save disk space..." | ||
| cargo clean | ||
| echo "=== Cleanup completed ===" | ||
| df -h |
There was a problem hiding this comment.
Running cargo clean after a successful build defeats the purpose of the rust-cache configuration. The cache is meant to speed up subsequent builds, but cleaning removes all build artifacts that would be cached. This step should be removed or reconsidered, as the cache action already handles optimal cache management.
| # Cleanup on success to save disk space for other jobs | |
| - name: Cleanup build artifacts (Main Branch Only) | |
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' | |
| run: | | |
| echo "Cleaning up build artifacts to save disk space..." | |
| cargo clean | |
| echo "=== Cleanup completed ===" | |
| df -h | |
| # Cleanup step kept for observability; skip cargo clean to preserve Rust cache | |
| - name: Cleanup build artifacts (Main Branch Only) | |
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' | |
| run: | | |
| echo "Skipping 'cargo clean' to preserve Rust build cache. Disk usage after build:" | |
| df -h |
| - name: Check disk space before build | ||
| run: df -h | ||
|
|
There was a problem hiding this comment.
The df -h command is not available on Windows by default. This step needs to be conditional or use a cross-platform alternative. Consider adding a shell-specific condition or using PowerShell equivalent Get-PSDrive for Windows.
| - name: Check disk space before build | |
| run: df -h | |
| - name: Check disk space before build (Unix) | |
| if: matrix.os != 'windows-latest' | |
| run: df -h | |
| - name: Check disk space before build (Windows) | |
| if: matrix.os == 'windows-latest' | |
| shell: pwsh | |
| run: | | |
| Get-PSDrive -PSProvider FileSystem |
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| echo "Running quick check for PR (cargo check)..." | ||
| cargo check --target ${{ matrix.target }} --jobs 2 2>&1 | tail -50 |
There was a problem hiding this comment.
The tail command is not available in PowerShell by default on Windows. This will cause the PR check to fail on the Windows matrix job. Consider using Select-Object -Last 50 for Windows or make this command conditional.
| echo "Running quick check for PR (cargo check)..." | ||
| cargo check --target ${{ matrix.target }} --jobs 2 2>&1 | tail -50 | ||
| echo "=== Quick check completed ===" | ||
| df -h |
There was a problem hiding this comment.
The df -h command is not available on Windows by default and will cause the quick check step to fail on Windows matrix jobs. Use a cross-platform approach or add conditional logic.
| - name: Build (Debug, Main Branch Only) | ||
| if: github.event_name == 'push' && github.ref == 'refs/heads/main' | ||
| run: | |
There was a problem hiding this comment.
The tee command is not available in PowerShell by default on Windows. This will cause the build step to fail on Windows matrix jobs. Consider using Tee-Object for Windows or redirecting output differently.
| - name: Build (Debug, Main Branch Only) | |
| if: github.event_name == 'push' && github.ref == 'refs/heads/main' | |
| run: | | |
| - name: Build (Debug, Main Branch Only, Windows) | |
| if: github.event_name == 'push' && github.ref == 'refs/heads/main' && matrix.os == 'windows-latest' | |
| shell: pwsh | |
| run: | | |
| Write-Host "Starting full debug build for main branch (Windows)..." | |
| cargo build --target ${{ matrix.target }} --jobs 2 2>&1 | Tee-Object -FilePath build.log | |
| Write-Host "=== Build completed ===" | |
| env: | |
| OPENSSL_DIR: ${{ matrix.os == 'macos-latest' && '/usr/local/opt/openssl@3' || (matrix.os == 'ubuntu-latest' && '/usr' || '') }} | |
| OPENSSL_LIB_DIR: ${{ matrix.os == 'macos-latest' && '/usr/local/opt/openssl@3/lib' || (matrix.os == 'ubuntu-latest' && '/usr/lib/x86_64-linux-gnu' || '') }} | |
| OPENSSL_INCLUDE_DIR: ${{ matrix.os == 'macos-latest' && '/usr/local/opt/openssl@3/include' || (matrix.os == 'ubuntu-latest' && '/usr/include' || '') }} | |
| LIBGIT2_SYS_USE_PKG_CONFIG: ${{ matrix.os == 'ubuntu-latest' && '1' || '0' }} | |
| - name: Build (Debug, Main Branch Only, non-Windows) | |
| if: github.event_name == 'push' && github.ref == 'refs/heads/main' && matrix.os != 'windows-latest' | |
| shell: bash | |
| run: | |
| CARGO_TERM_COLOR: always | ||
| RUST_BACKTRACE: 1 | ||
| # Optimize for disk space and stability | ||
| CARGO_BUILD_JOBS: 2 |
There was a problem hiding this comment.
Setting CARGO_BUILD_JOBS: 2 globally limits parallel compilation to 2 jobs, which may significantly slow down builds on machines with more CPU cores (GitHub runners typically have 4 cores). While this may help with disk space, it trades off build speed. Consider if this trade-off aligns with the optimization goals, or make it conditional only for resource-constrained scenarios.
| CARGO_BUILD_JOBS: 2 |
| - name: Cleanup build artifacts (Main Branch Only) | ||
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' | ||
| run: | | ||
| echo "Cleaning up build artifacts to save disk space..." | ||
| cargo clean | ||
| echo "=== Cleanup completed ===" | ||
| df -h |
There was a problem hiding this comment.
The df -h command is not available on Windows. Since this cleanup step only runs on main branch, and the diagnostic command will fail on Windows, add OS-specific conditionals or remove the diagnostic command from Windows runs.
| - name: Cleanup build artifacts (Main Branch Only) | |
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' | |
| run: | | |
| echo "Cleaning up build artifacts to save disk space..." | |
| cargo clean | |
| echo "=== Cleanup completed ===" | |
| df -h | |
| - name: Cleanup build artifacts (Main Branch Only, non-Windows) | |
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' && matrix.os != 'windows-latest' | |
| run: | | |
| echo "Cleaning up build artifacts to save disk space..." | |
| cargo clean | |
| echo "=== Cleanup completed ===" | |
| df -h | |
| - name: Cleanup build artifacts (Main Branch Only, Windows) | |
| if: success() && github.event_name == 'push' && github.ref == 'refs/heads/main' && matrix.os == 'windows-latest' | |
| run: | | |
| echo "Cleaning up build artifacts to save disk space..." | |
| cargo clean | |
| echo "=== Cleanup completed ===" |
| cache-targets: true | ||
| cache-all-crates: true |
There was a problem hiding this comment.
The options cache-targets and cache-all-crates do not exist in rust-cache@v2. The valid options are: key, shared-key, prefix-key, cache-directories, cache-on-failure, workspaces, save-if, and cache-provider. These invalid options will be silently ignored, which may not provide the intended optimization benefits.
| cache-targets: true | |
| cache-all-crates: true |
| name: build-failure-${{ matrix.os }}-${{ matrix.target }} | ||
| path: | | ||
| build.log | ||
| target/ |
There was a problem hiding this comment.
Uploading the entire target/ directory on build failure could be extremely large (10-20GB as mentioned in comments) and may cause the upload action to fail or consume excessive storage. Consider uploading only relevant failure artifacts like error logs, or add size limits to prevent storage issues.
| target/ | |
| if-no-files-found: ignore |
| with: | ||
| name: build-failure-${{ matrix.os }}-${{ matrix.target }} | ||
| path: | | ||
| build.log |
There was a problem hiding this comment.
The build.log file will only be created during the full build step (line 158), which only runs for main branch pushes. However, this upload step runs on failure() for all workflow runs including PR builds. For PR builds, the build.log won't exist since only cargo check runs. Consider making this path conditional based on the event type.
Summary
Summary about this PR