GeezerVison

What is the benefit to run the suit daily? If the report is used as a guide for additions into the tests, a weekly run seems to be enough.

What is the benefit to run the suite daily? If the report is used as a guide for additions into the tests, a weekly run seems to be enough.

If it's daily then it's up-to-date more than the weekly. More up-to-date is better than less up-to-date 😉. There's also the benefit that people will have a better idea of what to work on in terms of coverage than not as someone may put in a lot of work for something that's already been fixed earlier in the week.

Not to say that a weekly coverage run isn't good as well, but daily is just better if we can make it happen.

Is there any particular reasons why you are using clang and custom instructions for LCOV instead of make coverage-report? The latter uses gcc and lcov tool.

Is there any particular reasons why you are using clang and custom instructions for LCOV instead of make coverage-report? The latter uses gcc and lcov tool.

Which one produces more useful output? (I really don't care which we use even though everything I do at work is clang/llvm based - I just want line and branch coverage data)

@tiran the idea of using clang comes from faster-cpython/ideas#426 where I believe branch coverage was considered the big win.

Maybe coverage-report should get updated to use LLVM? Or make it configurable for either clang or gcc?

GCC and LCOV can do branch coverge, too. I'll have a branch ready by tomorrow.

I would welcome to have LLVM coverage support in Makefile, too.

Is there any particular reasons why you are using clang and custom instructions for LCOV instead of make coverage-report? The latter uses gcc and lcov tool.

It was an attempt to get branch coverage, not realizing lcov could do it. I still think llvm-cov displays branches within subexpressions in a more readable way, however. I don't know if using gcov for collection + llvm-cov for output would give us the best of both worlds. (Using gcc for the collection would be preferable, probably, since that's what we test with on Linux).

I don't know if using gcov for collection + llvm-cov for output would give us the best of both worlds.

It doesn't look like this is possible. There is an llvm-cov gcov command that takes the gcc coverage data as input and outputs human-readable text file reports. But I can't find a way to use gcc coverage data and output the nice llvm-cov html reports.

@brettcannon wrote:

Maybe coverage-report should get updated to use LLVM? Or make it configurable for either clang or gcc?

@tiran wrote:

I would welcome to have LLVM coverage support in Makefile, too.

I think that's a good idea, but a fair chunk of fiddly work. The Makefile doesn't have any direct support for building with clang on Linux. It works if you set CC, obviously, but you have to do that prior to re-running configure. So I don't think it's possible to just have make coverage-report-llvm work (since the Makefile can't call configure). We could, probably, make a task that errors out if clang isn't being used.

It works if you set CC, obviously, but you have to do that prior to re-running configure

Can you have the coverage-report target do the right thing based on what CC was set to?

I just refactored this so there is a new Makefile target coverage-report-llvm that uses llvm-cov to generate a coverage report -- and then the Github Action just uses that.

Can you have the coverage-report target do the right thing based on what CC was set to?

I hadn't thought of this -- it's probably doable. Given that you have to completely reconfigure to switch between each "mode" that probably makes sense.

I've updated this to automatically choose lcov or llvm-cov depending on the compiler in use.

@erlend-aasland wrote:

Also: please don't force push :) The GitHub UI don't play well with force-pushes. Using git merge --no-ff main makes for a better review experience.

I tried this and unfortunately it created a bunch of co-authored commits for everything that had happened in the meantime and then requested review from ~20 codeowners. I went back to rebasing/force pushing for a cleaner review of the actually relevant commits. Going to read the relevant parts of the devguide for next time...

Are there any additional comments on this?

Create a new repository in the python organization for the results, e.g. cpython-c-coverage

There's a process for this now.
I don't think this repo needs to start under a personal GitHub account -- moving the config and creating a new token would be more trouble than it's worth. That leaves asking the SC as the next step, see here for example: python/steering-council#236

Adding LLVM support to coverage-report would work better as a separate PR, but I won't block on that.

This PR is stale because it has been open for 30 days with no activity.