Skip to content
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

Switch to using Codecov GitHub Action. #1158

Merged
merged 5 commits into from Feb 23, 2022

Conversation

nmoinvaz
Copy link
Member

@nmoinvaz nmoinvaz commented Feb 6, 2022

This uses codecov's GitHub Actions action.
https://github.com/codecov/codecov-action

Which also uses their new uploader.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 6, 2022

@Dead2 @iii-i Is it possible to install these packages on z15 runner?

sudo apt-get install libxml2-dev libxslt-dev python-dev

https://stackoverflow.com/questions/5178416/libxml-install-error-using-pip

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 6, 2022

Fixed pigz workflow.

@iii-i
Copy link
Member

iii-i commented Feb 6, 2022

I've installed libxml2-devel libxslt-devel python3-devel (the builder is running RHEL).

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 6, 2022

It looks like this command is still failing:
python3 -u -m pip install --user gcovr

@iii-i
Copy link
Member

iii-i commented Feb 6, 2022

Argh, of course - the build is running inside a Docker container. I've updated the image, please try again. If it works, feel free to cherry-pick iii-i@f26c347 into your PR.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 6, 2022

Cherry-picked. Thanks!

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 7, 2022

@Dead2 what is the plan for buildkite?

@Dead2
Copy link
Member

Dead2 commented Feb 7, 2022

This does not seem to work right.
Codecov.io shows "No report uploaded yet" for this PR, other PRs (like #1159) work fine.

Buildkite.. not decided yet, but was considering using it for automated benchmarking.

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

I wonder why this PRs coverage does not appear in codecov.
It seems like the token is sent correctly, and it seems like it finds all the coverage data, and it probably gets uploaded. But codecov .io sees no reports..

Processing file: /home/runner/work/zlib-ng/zlib-ng/CMakeFiles/zlib.dir/insert_string.c.gcda
Running gcov: 'gcov /home/runner/work/zlib-ng/zlib-ng/CMakeFiles/zlib.dir/insert_string.c.gcda --branch-counts --branch-probabilities --preserve-paths --object-directory /home/runner/work/zlib-ng/zlib-ng/CMakeFiles/zlib.dir' in '/tmp/tmpx5lhk_69'
Finding source file corresponding to a gcov data file
  currdir      /home/runner/work/zlib-ng/zlib-ng
  gcov_fname   /tmp/tmpx5lhk_69/#home#runner#work#zlib-ng#zlib-ng#insert_string_tpl.h.gcov
               ['        -', '    0', 'Source', '/home/runner/work/zlib-ng/zlib-ng/insert_string_tpl.h\n']
  source_fname /home/runner/work/zlib-ng/zlib-ng/CMakeFiles/zlib.dir/insert_string.c.gcda
  root         /home/runner/work/zlib-ng/zlib-ng
  fname        /home/runner/work/zlib-ng/zlib-ng/insert_string_tpl.h
Parsing coverage data for file /home/runner/work/zlib-ng/zlib-ng/insert_string_tpl.h

I see nothing wrong here at all. Only possibility is the \n at the end of the file name, but it would be a bit strange if that was the error, no?

Relevant part of the codecov yaml below; it replaces that part of the path with nothing, so that it sees files according to the path in the repo. So that should still be correct.

fixes:
- '/home/runner/work/zlib-ng/zlib-ng/::'

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 10, 2022

I noticed in the CI list that the 2 Codecov instances no longer show.

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

@nmoinvaz That is a problem, but unrelated to this PR. Codecov claims it was because of the buildkite build was failing and suggested config changes. I have applied the changes, but not sure whether it will work for old PRs.
In any case, those are missing from all PRs, not just this one. But this one is the only one without any uploaded coverage reports.

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

Still nothing. Just to exclude some of the recent codecov problems, could you close this PR and make a new one?

@nmoinvaz
Copy link
Member Author

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

Then perhaps it is because it is a foreign repo and thus treated differently (hence our token workaround)

@nmoinvaz
Copy link
Member Author

Yes, it is probably due to the token not being exposed to my PR. You could create a branch in zlib-ng and cherry-pick my changes and test it there?

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

Sure, but we would still need to fix the workaround, so that we get coverage data for PRs like we do today.
As-is, this would be a regression in that regard.

@nmoinvaz
Copy link
Member Author

It keeps saying there was an error in processing.. what does your codecov.yaml look like?

coverage:
  status:
    project:
      default:
        target: auto
        threshold: 1.0
fixes:
- '^build/home/runner/work/zlib-ng/zlib-ng/::'
- '^build/home/runner/work/zlib-ng/build/::'
ignore:
- ^usr/include/.*
- ^usr/lib/.*
- ^build/usr/include/.*
- ^build/usr/lib/.*
- ^lib/.*
- ^lib/.*/.*

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

codecov:
  max_report_age: 72h
  notify:
    wait_for_ci: false
  require_ci_to_pass: false
comment:
  require_base: false
  require_head: false
coverage:
  status:
    project:
      default:
        threshold: 0.07
fixes:
- '/home/runner/work/zlib-ng/zlib-ng/::'
- '/home/runner/work/zlib-ng/build/::'
- '/build/home/runner/work/zlib-ng/zlib-ng/::'
- '/build/home/runner/work/zlib-ng/build/::'
ignore:
- usr/include/.*
- /usr/include/.*
- /build/usr/include/.*
- usr/lib/.*
- /usr/lib/.*
- /build/usr/lib/.*
- usr/lib64/.*
- /usr/lib64/.*
- /build/usr/lib64/.*

We really should add it as a file though.

@nmoinvaz
Copy link
Member Author

Do you want me to add it as part of this PR?

@Dead2
Copy link
Member

Dead2 commented Feb 10, 2022

Sure, if you want to.
The fixes and ignore parts should be cleaned up first though, some of that stuff is old Travis stuff, but I have not wanted to mess with it as long as it worked correctly.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 10, 2022

I can't figure out why the logs are showing Token found by environment variables even when I don't set environment variable.

@nmoinvaz
Copy link
Member Author

Actually it looks like it is the codecov/action that is setting that environment variable for the NodeJS uploader.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 10, 2022

@thomasrockhu any idea why this PR would not be showing up on the code coverage website? The reports generate properly and upload to codecov.io but fail to show on the website.

They do however show for my repository, but not in the PR commit.

My PR for zlib-ng repo:
https://codecov.io/gh/zlib-ng/zlib-ng/commit/1b2851f3af49fa67fc34f2b4a24d7352cfb3bda4/
My branch for my repo:
https://codecov.io/gh/nmoinvaz/zlib-ng/commit/baa58349fe038d70cc55a473ea36785c12d92b43/

We are trying to move away from python uploader and to use the codecov GH action.

@thomasrockhu-codecov
Copy link

@nmoinvaz just confirming that I saw this and responded on the community thread here

@codecov
Copy link

codecov bot commented Feb 12, 2022

Codecov Report

Merging #1158 (0e5bee5) into develop (321028c) will increase coverage by 5.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1158      +/-   ##
===========================================
+ Coverage    81.99%   87.08%   +5.08%     
===========================================
  Files           97       96       -1     
  Lines         9080     9034      -46     
  Branches      1440     2234     +794     
===========================================
+ Hits          7445     7867     +422     
+ Misses        1068     1050      -18     
+ Partials       567      117     -450     
Flag Coverage Δ
macos_clang ∅ <ø> (∅)
macos_gcc 68.69% <ø> (-0.26%) ⬇️
ubuntu_clang 82.91% <ø> (+6.83%) ⬆️
ubuntu_clang_debug 84.23% <ø> (+8.13%) ⬆️
ubuntu_clang_inflate_allow_invalid_dist 82.77% <ø> (+6.83%) ⬆️
ubuntu_clang_inflate_strict 84.27% <ø> (+9.89%) ⬆️
ubuntu_clang_mmap 84.39% <ø> (+8.35%) ⬆️
ubuntu_clang_pigz 35.50% <ø> (+1.51%) ⬆️
ubuntu_clang_pigz_no_optim 39.97% <ø> (-0.07%) ⬇️
ubuntu_clang_pigz_no_threads 35.07% <ø> (+1.43%) ⬆️
ubuntu_clang_reduced_mem 84.51% <ø> (+8.32%) ⬆️
ubuntu_gcc 72.00% <ø> (-2.52%) ⬇️
ubuntu_gcc_aarch64 72.29% <ø> (-3.63%) ⬇️
ubuntu_gcc_aarch64_compat_no_opt 68.79% <ø> (-4.80%) ⬇️
ubuntu_gcc_aarch64_no_acle 69.29% <ø> (-6.14%) ⬇️
ubuntu_gcc_aarch64_no_neon 69.47% <ø> (-6.08%) ⬇️
ubuntu_gcc_armhf 72.24% <ø> (-3.67%) ⬇️
ubuntu_gcc_armhf_compat_no_opt 68.70% <ø> (-4.99%) ⬇️
ubuntu_gcc_armhf_no_acle 72.43% <ø> (-3.74%) ⬇️
ubuntu_gcc_armhf_no_neon 72.65% <ø> (-3.65%) ⬇️
ubuntu_gcc_armsf 72.24% <ø> (-3.67%) ⬇️
ubuntu_gcc_armsf_compat_no_opt 68.70% <ø> (-4.99%) ⬇️
ubuntu_gcc_benchmark 73.22% <ø> (-4.05%) ⬇️
ubuntu_gcc_compat_no_opt 72.20% <ø> (-2.32%) ⬇️
ubuntu_gcc_compat_sprefix 71.90% <ø> (-3.61%) ⬇️
ubuntu_gcc_mingw_i686 0.00% <ø> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <ø> (ø)
ubuntu_gcc_no_avx2 69.13% <ø> (-6.70%) ⬇️
ubuntu_gcc_no_ctz 72.80% <ø> (-3.77%) ⬇️
ubuntu_gcc_no_ctzll 72.55% <ø> (-3.75%) ⬇️
ubuntu_gcc_no_pclmulqdq 68.42% <ø> (-6.01%) ⬇️
ubuntu_gcc_no_sse2 69.18% <ø> (-5.07%) ⬇️
ubuntu_gcc_no_sse4 69.14% <ø> (-5.92%) ⬇️
ubuntu_gcc_o3 71.84% <ø> (-1.56%) ⬇️
ubuntu_gcc_osb ∅ <ø> (∅)
ubuntu_gcc_pigz 35.96% <ø> (+1.57%) ⬆️
ubuntu_gcc_pigz_aarch64 38.42% <ø> (+0.73%) ⬆️
ubuntu_gcc_ppc 71.85% <ø> (-2.89%) ⬇️
ubuntu_gcc_ppc64 72.67% <ø> (-3.59%) ⬇️
ubuntu_gcc_ppc64le 71.83% <ø> (-3.07%) ⬇️
ubuntu_gcc_ppc_no_power8 72.90% <ø> (-4.45%) ⬇️
ubuntu_gcc_s390x 73.35% <ø> (-4.37%) ⬇️
ubuntu_gcc_s390x_dfltcc 72.44% <ø> (+0.27%) ⬆️
ubuntu_gcc_s390x_dfltcc_compat 69.61% <ø> (-1.77%) ⬇️
ubuntu_gcc_s390x_no_crc32 73.06% <ø> (-4.41%) ⬇️
ubuntu_gcc_sparc64 73.20% <ø> (-4.27%) ⬇️
ubuntu_gcc_sprefix 71.63% <ø> (-3.65%) ⬇️
win64_gcc 70.29% <ø> (∅)
win64_gcc_compat_no_opt 71.52% <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/benchmarks/benchmark_compare256.cc 42.85% <0.00%> (-51.59%) ⬇️
arch/s390/dfltcc_detail.h 54.90% <0.00%> (-45.10%) ⬇️
test/benchmarks/benchmark_adler32.cc 50.00% <0.00%> (-43.34%) ⬇️
arch/x86/chunkset_sse2.c 61.11% <0.00%> (-38.89%) ⬇️
arch/x86/slide_hash_sse2.c 64.70% <0.00%> (-29.05%) ⬇️
test/benchmarks/benchmark_crc32.cc 66.66% <0.00%> (-26.67%) ⬇️
test/benchmarks/benchmark_slidehash.cc 72.00% <0.00%> (-23.46%) ⬇️
chunkset.c 94.11% <0.00%> (-5.89%) ⬇️
compare256.c 84.33% <0.00%> (-3.01%) ⬇️
zutil.c 79.16% <0.00%> (-2.98%) ⬇️
... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 321028c...0e5bee5. Read the comment docs.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Feb 12, 2022

Thank you @thomasrockhu-codecov that seems to have fixed the issue. Not sure why though because I did check the timestamp being put in the XML and it looked right. Anyways, by turning that check off at least now there is one less point of failure. I ended up using gcovr instead of pytest because we needed to specify gcov-executable for some of our test runs.

@nmoinvaz
Copy link
Member Author

@Dead2 I am seeing -5% coverage. I think these results are accurate. It could be because the benchmarks are not being run on every platform. I think after incorporating gtest (which has all the c++ tooling) we can turn on the benchmarks for more platforms.

@nmoinvaz nmoinvaz force-pushed the ci/codecov-actions branch 2 times, most recently from 2755d3f to 84bcbbb Compare February 13, 2022 01:46
@nmoinvaz
Copy link
Member Author

Rebased.

@Dead2
Copy link
Member

Dead2 commented Feb 23, 2022

I really don't see any reason why this PR should change the coverage at all (other than the usual noise), this commit does not really make any changes to build, code or testing compared to the develop commit it is based on, so coverage should stay the same.

-5% is a pretty big change. Interestingly, most of the lost coverage is related to assert() or failure modes.
See test/infcover.c for example: https://app.codecov.io/gh/zlib-ng/zlib-ng/compare/1158/changes
I have no idea why that would be any different with this PR.

But I'll reluctantly approve this for merge.

@nmoinvaz
Copy link
Member Author

@Dead2, I think it might have something to do with gcovr generating reports. I could try --exclude-unreachable-branches and see if that helps..

@nmoinvaz
Copy link
Member Author

@Dead2 looks like that gave us +5% in coverage.

@Dead2
Copy link
Member

Dead2 commented Feb 23, 2022

@nmoinvaz I see, looking at the docs it seems there is a mismatch with how the statistics count different parts of the code. And --exclude-unreachable-branches seems to exclude a little more than the previous codecov-uploader does.
But from what I can see, there is nothing being excluded that we actually do care about.
Exclude branch coverage from lines without useful source code (often, compiler-generated “dead” code)., so things like initialization code, GCC attributes, closing curly braces, etc.

So, I think this new percentage is actually more useful to us than the old one was. 👍

.github/workflows/cmake.yml Outdated Show resolved Hide resolved
.github/workflows/pigz.yml Outdated Show resolved Hide resolved
@Dead2 Dead2 merged commit 5945d87 into zlib-ng:develop Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants