Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: use ubuntu-latest for coverage checks #578

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Jan 24, 2025

Similar to moonbitlang/core#1549, this should help mitigate our CI bottleneck caused by the shortage of macOS runners.

Copy link

peter-jerry-ye-code-review bot commented Jan 24, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

⚠️ [Potential Missing LLVM Dependencies on Ubuntu Runner]
  • Category: Correctness
  • Code Snippet: cargo tarpaulin -j 3 --ignore-tests --engine llvm --out Xml
  • Recommendation: Ensure LLVM dependencies (like llvm-tools-preview Rust component) are installed in the workflow before running tarpaulin.
  • Reasoning: The --engine llvm flag requires LLVM tools to be available. Ubuntu runners may not have these pre-installed, unlike macOS runners. Add a step like rustup component add llvm-tools-preview to prevent failures.
💡 [Suboptimal Parallel Job Count]
  • Category: Performance
  • Code Snippet: -j 3 in tarpaulin command
  • Recommendation: Use -j 2 instead to match common GitHub runner core count
  • Reasoning: GitHub-hosted Ubuntu runners typically have 2 vCPUs. Using 3 parallel jobs may cause resource contention and suboptimal scheduling compared to matching available cores.
ℹ️ [Explicit Tool Version Specification]
  • Category: Maintainability
  • Code Snippet: CARGO_TARPAULIN_VERSION environment variable usage
  • Recommendation: Consider pinning to a specific version hash for security
  • Reasoning: While the version variable is good practice, adding a checksum verification step when downloading binaries would improve supply chain security and prevent potential MITM attacks.

@rami3l rami3l requested review from Young-Flash and removed request for Young-Flash January 24, 2025 05:21
@rami3l rami3l marked this pull request as draft January 24, 2025 05:32
@rami3l
Copy link
Contributor Author

rami3l commented Jan 24, 2025

@lynzrand Can you reproduce this locally on Linux x64?

@lynzrand
Copy link
Collaborator

@lynzrand Can you reproduce this locally on Linux x64?

Can confirm the 8% coverage regress locally on x64-linux. Rerunning with LLVM...

@rami3l
Copy link
Contributor Author

rami3l commented Feb 11, 2025

It seems that cargo-tarpaulin will exit with an OOM error on our free CI runners. This is a known issue: xd009642/tarpaulin#1639.

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