gh-94759: Collect C-level coverage using llvm-cov #94760
+275
−24
Prev
Previous commit
Cleanup after recent changes
- Loading branch information
commit ceb32ee3af138a30703930d019761ae063394038
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 |
|---|---|---|
|
|
@@ -50,7 +50,7 @@ jobs: | |
| - name: Publish coverage-report | ||
| uses: JamesIves/github-pages-deploy-action@v4 | ||
| with: | ||
| folder: coverage-report | ||
| repository-name: '' # TODO Destination | ||
| token: ${{ secrets.COVERAGE_DEPLOY_TOKEN }} # TODO: Use an organization-level token | ||
|
Member
There was a problem hiding this comment. What permissions does the token need?
Contributor
Author
There was a problem hiding this comment. That's a good question, and it isn't terribly well-documented: https://github.com/JamesIves/github-pages-deploy-action#required-setup I think the permission to push to the coverage results repo is all that's needed, but we may not know until we try.
Member
There was a problem hiding this comment. Does it need two separate tokens then, one (with read permissions) for the cpython repo, and another (with write permissions) for the coverage results repo?
Contributor
Author
There was a problem hiding this comment. We don't need a token for read permissions to the cpython repo. Just one to write to a repo under the python org. |
||
| single-commit: true | ||
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
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 |
|---|---|---|
| @@ -1,2 +1 @@ | ||
| The ``coverage-report`` Makefile now supports both the ``gcc/lcov`` and ``clang/llvm-profdata`` stacks to generate coverage reports, and will select the correct one based on the compiler in use. |
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.
Does every run clobber the previous run's report? What about CI runs on PRs and CI runs from release branches? Can this be configured to commit the coverage results into a branch in the repo matching the reponame+branchname? Or do branches not render in gh-pages? in which case it'd need to be subdirectories in the repo.
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.
Currently, this clobbers the previous run. That is easily changed with a flag, but then we could run into Github repository size limits (each result is around 100MB of HTML). It's probably possible to keep the last N commits, but the tool I'm using to publish doesn't support that directly.
This is currently configured to just run on the
mainbranch once a day. We could do multiple branches that publish to subdirectories if you think there's a good use case for that. (Github pages only publishes a single branch, but subdirectories would work).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.
If we would get some funding to run a persistent VM with public hostname from ... let's say Azure, then it would be trivial to create a buildbot worker and serve the LCOV results from HTTP server. They are static HTTP and JS files on the file system after all. Just saying :)
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 don't mind
mainbranch only and once a day. Anyone working on coverage is presumably doing their own local coverage runs while creating PRs.Regarding a buildbot configured to host the results, while I could simply set one up it'd probably make more sense for mdboom to do that and be an admin given who's driving this work. :)
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.
Yeah, I'd be happy to admin that if it comes to it. I think it's fine to go with the Github Action here as an MVP, and if the amount of history or frequency of runs isn't good enough, we can revisit migrating to our own VM down the road.