-
Notifications
You must be signed in to change notification settings - Fork 35
Update coverage bot to Ubuntu 24.04 x86 bot #735
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,15 +23,6 @@ jobs: | |
| matrix: | ||
| include: | ||
| # Ubuntu Arm Jobs | ||
| - name: ubu22-arm-gcc12-clang-repl-20-coverage | ||
| os: ubuntu-22.04-arm | ||
| compiler: gcc-12 | ||
| clang-runtime: '20' | ||
| cling: Off | ||
| cppyy: Off | ||
| llvm_enable_projects: "clang" | ||
| llvm_targets_to_build: "host;NVPTX" | ||
| coverage: true | ||
| - name: ubu24-arm-gcc12-clang-repl-20 | ||
| os: ubuntu-24.04-arm | ||
| compiler: gcc-12 | ||
|
|
@@ -66,6 +57,15 @@ jobs: | |
| llvm_enable_projects: "clang" | ||
| llvm_targets_to_build: "host;NVPTX" | ||
| # Ubuntu X86 Jobs | ||
| - name: ubu24-x86-gcc12-clang-repl-20-coverage | ||
| os: ubuntu-24.04 | ||
| compiler: gcc-12 | ||
| clang-runtime: '20' | ||
| cling: Off | ||
| cppyy: Off | ||
| llvm_enable_projects: "clang" | ||
| llvm_targets_to_build: "host;NVPTX" | ||
| coverage: true | ||
| - name: ubu24-x86-gcc12-clang-repl-20 | ||
| os: ubuntu-24.04 | ||
| compiler: gcc-12 | ||
|
|
@@ -261,10 +261,13 @@ jobs: | |
| # Create lcov report | ||
| # capture coverage info | ||
| vers="${CC#*-}" | ||
| lcov --directory build/ --capture --output-file coverage.info --gcov-tool /usr/bin/gcov-${vers} | ||
| lcov --remove coverage.info '/usr/*' "${HOME}"'/.cache/*' ${{ github.workspace }}'/llvm-project/*' ${{ github.workspace }}'/unittests/*' --output-file coverage.info | ||
| lcov --directory build/ --capture --output-file coverage.info --gcov-tool /usr/bin/gcov-${vers} --ignore-errors mismatch | ||
| lcov --remove coverage.info '/usr/*' ${{ github.workspace }}'/llvm-project/*' --ignore-errors unused --output-file coverage.info | ||
| lcov --remove coverage.info '${{ github.workspace }}/unittests/*' --ignore-errors unused --output-file coverage.info | ||
| lcov --remove coverage.info '${{ github.workspace }}/build/*' --ignore-errors unused --output-file coverage.info | ||
| # output coverage data for debugging (optional) | ||
| lcov --list coverage.info | ||
| rm -rf ./build/ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit of a fudge but without it codecov will ignore the coverage report the ci makes, and makes its own (which will include the unittests folder in the coverage report) |
||
|
|
||
| - name: Upload to codecov.io | ||
| if: ${{ success() && (matrix.coverage == true) }} | ||
|
|
@@ -289,4 +292,3 @@ jobs: | |
| uses: mxschmitt/action-tmate@v3 | ||
| # When debugging increase to a suitable value! | ||
| timeout-minutes: 30 | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why do we have
--ignore-errors mismatchflag?The
lcov --removeis fine.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.
Without these changes the ci crashes when you upgrade, since you get a newer lcov version is more restrictive in what it will allow you to get away with. All the changes here were found to be necessary.
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.
Looking at the docs https://manpages.debian.org/unstable/lcov/lcov.1.en.html#mismatch:
I feel we should try and avoid it. Do we have a CI run without that flag?
Uh oh!
There was an error while loading. Please reload this page.
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.
Avoiding this flag would mean dealing with all the mismatch warnings we currently have in the ci now (see https://github.com/compiler-research/CppInterOp/actions/runs/18694188958/job/53307234747?pr=735#step:13:52). If you see all the warnings they are in the test files, and to me dealing with them doesn't feel like it gives any benefit given they aren't effecting the end result, and they can easily be ignored with a flag which lcov itself suggested.
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.
That run still passes. Just gives you a few warnings. I believe that is fine. We should try and fix those warnings instead of ignoring them. Will approve it if you can remove
--ignore-errorsflag. We can fix the warnings later.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.
That run passes in the link I gave above because
--ignore-errors mismatchis there 😅 . It will error without that flag. We cannot fix them later. If we remove that flag then they would need to be fixed in this PR.Uh oh!
There was an error while loading. Please reload this page.
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.
Without that flag is treats all those warnings as errors.
--ignore-errors mismatchis literally telling lcov to not treat the mismatches as errors, but warnings instead.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.
Hmm. Ok.