-
Notifications
You must be signed in to change notification settings - Fork 82
Add Coverity scan #172
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
Add Coverity scan #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please squash commits once you finish? Thanks.
.github/workflows/coverity.yml
Outdated
- uses: actions/checkout@v4 | ||
- name: Download Linux 64 Coverity Tool | ||
run: | | ||
curl https://scan.coverity.com/download/cxx/linux64 --output ${GITHUB_WORKSPACE}/cov-linux64-tool.tar.gz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor:
If curl fails during tool download or the upload, it might go unnoticed. Consider adding --fail or checking exit codes:
curl --fail ... || { echo "Download failed"; exit 1; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm I'm not entirely convinced this is needed as well, Github Actions is quite good at notifying the user about errors, if curl fails to download a package, then the step would fail regardless, as in the same step the script tries to tar the file and it would simply not find it.
Worst case scenario, if it downloads part of the package and tars it, the next step would fail, as the cov tools would be corrupted/missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to fail early, with clear messagage about what went wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a GitHub Actions workflow to submit Coverity scans for the project.
- Introduces
.github/workflows/coverity.yml
- Configures download, build, archive, and upload of Coverity analysis data
- Runs on pushes and pull requests to the default branch
.github/workflows/coverity.yml
Outdated
branches: [master] | ||
|
||
pull_request: | ||
branches: [master] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow is configured to run only on pushes to the 'master' branch, but the repository’s default branch is 'main'; update this to 'main' to ensure the action triggers.
branches: [master] | |
pull_request: | |
branches: [master] | |
branches: [main] | |
pull_request: | |
branches: [main] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@i-kosarev could you please proof-check this ? According to what I checked (and found out) our branch name is "master", not "main" but - since this comment was generated - could you please check it for correctness as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maciekpac I believe the guys checking before merging. Do you ask for that?
.github/workflows/coverity.yml
Outdated
- synchronize | ||
- reopened | ||
|
||
permissions: read-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limit the workflow permissions to only what's needed (e.g., contents: read
) instead of read-all
to adhere to the principle of least privilege.
permissions: read-all | |
permissions: | |
contents: read |
Copilot uses AI. Check for mistakes.
permissions: read-all | ||
|
||
env: | ||
BUILD_CONCURRENCY: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider using the runner's available CPU count (for example, ${{ runner.cores }}
) instead of hardcoding BUILD_CONCURRENCY
to improve utilization across different runner sizes.
BUILD_CONCURRENCY: 4 | |
BUILD_CONCURRENCY: ${{ runner.cores }} |
Copilot uses AI. Check for mistakes.
Generally looks fine. Could you please share the link to the results? |
Add Coverity scan This adds the Coverity scan for the project https://scan.coverity.com/projects/oneapi-collective-communications-library Note, that there are limitations to the number of weekly builds: "Up to 21 builds per week, with a maximum of 3 builds per day, for projects with 100K to 500K lines of code" Due to this limit, jobs will only be triggered on push to the master branch
c17b173
to
7ebf85e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds the Coverity scan for the project https://scan.coverity.com/projects/oneapi-collective-communications-library
Note, that there are limitations to the number of weekly builds:
"Up to 21 builds per week, with a maximum of 3 builds per day, for projects with 100K to 500K lines of code"
Due to this limit, jobs will only be triggered on push to the master branch.
Successful run:
https://github.com/uxlfoundation/oneCCL/actions/runs/15492683484
Scan results can be seen here
https://scan.coverity.com/projects/oneapi-collective-communications-library