Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the benchmark infrastructure to publish results to GitHub Pages instead of embedding static images in the README. The changes improve maintainability by automating benchmark result hosting while keeping the README cleaner.
- Removes static benchmark images from README, directing users to GitHub Actions for results
- Adds automated GitHub Pages deployment for benchmark results with artifact organization
- Updates workflow permissions and concurrency settings to support the new deployment workflow
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| README.md | Removes embedded benchmark images, keeping only the link to GitHub Actions results |
| .github/workflows/bench.yml | Adds publish-pages job to deploy benchmark results to GitHub Pages; updates permissions to write and changes concurrency settings |
| .clang-format | Removes the Language field (clang-format can infer language from file extensions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| group: benchmark-pages | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
The concurrency settings have been changed to use a static group name "benchmark-pages" with cancel-in-progress set to false. This means all benchmark workflow runs (regardless of the branch or trigger) will be grouped together and will not cancel each other. If multiple workflow runs are triggered (e.g., from different branches or manual triggers), they will queue up and run sequentially rather than canceling previous runs. Consider if this is the intended behavior, as it could lead to a queue of benchmark runs if triggered frequently.
| group: benchmark-pages | |
| cancel-in-progress: false | |
| group: benchmark-pages-${{ github.ref }} | |
| cancel-in-progress: true |
|
|
||
| permissions: | ||
| contents: read | ||
| contents: write |
There was a problem hiding this comment.
The workflow permissions have been changed from "contents: read" to "contents: write". While this is necessary for the new publish-pages job to push to the gh-pages branch, it grants write access to the entire workflow, including the linux and windows jobs which only need read access. Consider using job-level permissions instead to follow the principle of least privilege, granting write access only to the publish-pages job.
| if [ -d "$d" ]; then | ||
| name="$(basename "$d")" | ||
| mkdir -p "${OUT_DIR}/${name}" | ||
| cp -R "$d"/* "${OUT_DIR}/${name}"/ || true |
There was a problem hiding this comment.
The bash command uses "cp -R" with "|| true" which silently ignores all copy failures. If there are permission issues, missing files, or other problems during the copy operation, they will be suppressed without any indication. Consider using proper error handling or at least logging a warning when the copy fails, so that issues can be diagnosed.
| cp -R "$d"/* "${OUT_DIR}/${name}"/ || true | |
| # Only attempt to copy if the artifact directory is non-empty. | |
| if compgen -G "$d/*" > /dev/null; then | |
| if ! cp -R "$d"/* "${OUT_DIR}/${name}/"; then | |
| echo "warning: failed to copy contents of '$d' to '${OUT_DIR}/${name}/'" >&2 | |
| exit 1 | |
| fi | |
| fi |
1 participant
No description provided.