-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-94759: Collect C-level coverage using llvm-cov #94760
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
e2eed08
776e423
1ab7870
80f0039
dfa3fcb
50ba4f4
84f042a
ceb32ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,26 +40,17 @@ jobs: | |
| uses: hendrikmuhs/ccache-action@v1 | ||
| - name: Configure clang (with coverage) | ||
| run: | | ||
| echo "CC=/usr/lib/ccache/clang-12 -fprofile-instr-generate -fcoverage-mapping" >> $GITHUB_ENV | ||
| echo "CXX=/usr/lib/ccache/clang++-12 -fprofile-instr-generate -fcoverage-mapping" >> $GITHUB_ENV | ||
| echo "CC=clang" >> $GITHUB_ENV | ||
| echo "CXX=clang++" >> $GITHUB_ENV | ||
| - name: Configure CPython | ||
| run: ./configure --with-pydebug --with-openssl=$OPENSSL_DIR | ||
| - name: Build CPython | ||
| run: make -j4 | ||
| - name: Collect coverage data | ||
| # Specify the LLVM_PROFILE_FILE using %m so multiple shared objects can write | ||
| # in parallel. Set the full path to the directory so results aren't written | ||
| # into temporary directories created by tests. | ||
| # Using "-j 1" is important, or the Github Action runs out of memory | ||
| run: LLVM_PROFILE_FILE=${PWD}/python%m.profraw xvfb-run ./python -m test -j 1 | ||
| - name: Generate coverage report | ||
| run: | | ||
| llvm-profdata-12 merge -sparse python*.profraw -o python.profdata | ||
| llvm-cov-12 show -format=html -output-dir=cpython-coverage -instr-profile=python.profdata -show-branches=count -show-regions python . | ||
| # Using "-j1" is important, or the Github Action runs out of memory | ||
| run: EXTRATESTOPTS=-j1 xvfb-run make coverage-report-llvm | ||
| - name: Publish coverage-report | ||
| uses: JamesIves/github-pages-deploy-action@v4 | ||
| with: | ||
| folder: cpython-coverage | ||
| folder: llvm-cov-report | ||
| repository-name: '' # TODO Destination | ||
| token: ${{ secrets.COVERAGE_DEPLOY_TOKEN }} # TODO: Use an organization-level token | ||
|
Member
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. What permissions does the token need?
Contributor
Author
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| *.gc?? | ||
| *.profclang? | ||
| *.profraw | ||
| *.profdata | ||
| *.dyn | ||
| .gdb_history | ||
| .purify | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| A new Makefile target ``coverage-report-llvm`` will use ``clang`` and | ||
| ``llvm-cov`` to generate a coverage report. This provides more details about | ||
| branch coverage and subexpressions than the existing ``gcc`` and ``lcov`` | ||
| based ``coverage-report``. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.