[CI] Port CMake Codecov job from Codebuild to Github Actions. #5809

NlightNFotis · 2021-02-09T16:20:46Z

This is part of our streamlining and cleanup of our CI infrastructure.

This change is supposed to bring functional parity between what
we support for coverage in Codebuild and GitHub Actions.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

codecov · 2021-02-09T16:59:34Z

Codecov Report

Merging #5809 (f02904e) into develop (3cb60fc) will increase coverage by 3.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5809      +/-   ##
===========================================
+ Coverage    69.83%   72.83%   +3.00%     
===========================================
  Files         1242     1423     +181     
  Lines       100842   154045   +53203     
===========================================
+ Hits         70422   112200   +41778     
- Misses       30420    41845   +11425

Flag	Coverage Δ
cproversmt2	`?`
regression	`?`
unit	`?`

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

Impacted Files	Coverage Δ
src/goto-checker/goto_verifier.h	`0.00% <0.00%> (-100.00%)`	⬇️
src/goto-checker/symex_bmc_incremental_one_loop.h	`0.00% <0.00%> (-100.00%)`	⬇️
src/cpp/cpp_storage_spec.cpp	`65.00% <0.00%> (-35.00%)`	⬇️
src/solvers/strings/array_pool.h	`66.66% <0.00%> (-33.34%)`	⬇️
src/solvers/strings/string_refinement.h	`66.66% <0.00%> (-33.34%)`	⬇️
...rs/strings/string_concatenation_builtin_function.h	`0.00% <0.00%> (-33.34%)`	⬇️
src/util/config.h	`50.00% <0.00%> (-25.00%)`	⬇️
src/analyses/local_cfg.h	`75.00% <0.00%> (-25.00%)`	⬇️
src/jsil/jsil_parser.cpp	`0.00% <0.00%> (-25.00%)`	⬇️
src/assembler/assembler_parser.cpp	`0.00% <0.00%> (-25.00%)`	⬇️
... and 1365 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 3cb60fc...f02904e. Read the comment docs.

tautschnig

Do we have any way of knowing whether this genuinely works as expected?

.github/workflows/pull-request-checks.yaml

tautschnig · 2021-02-09T21:08:11Z

#5799 needs to be merged to address the CSmith failure (which can also be safely ignored for this PR).

piotr-grabalski

Looks good

piotr-grabalski · 2021-02-10T09:22:37Z

.github/workflows/pull-request-checks.yaml

+      - name: Collect coverage statistics
+        run: |
+          lcov --capture --directory build --output-file lcov.info
+          lcov --remove lcov.info '/usr/*' --output-file lcov.info


Is there anything else that could be removed from trace file (e.g. miniSAT/or other SAT solver files)?

Hm, I'm not sure to be honest, and I'd rather not play around with the contents of the files for now, as this is supposed to be just a port with functionality parity.

Attempting any changes like this would cause us to need extra time to validate/test those changes. I am happy however to add them on a todo list for later inspection/implementation. Does that work for you, @piotr-grabalski ?

Yes, that is OK for me.

I've been wondering if the report could be more accurate and display the test coverage of the code that is a part of CBMC rather than some third-party stuff that we are not involved in. I don't know how much of external code is going into the statistics so, as you said, more investigation will be required.

NlightNFotis · 2021-02-10T14:32:44Z

@tautschnig Addressing you question as to how we would know if this is working, I intend to push a change to this PR deactivating the other coverage jobs (the ones on code build), so they don't submit coverage statistics. In this case, if we see coverage report, it should be coming from this change, validating it.

tautschnig · 2021-02-11T12:01:44Z

.github/workflows/pull-request-checks.yaml

@@ -391,7 +391,7 @@ jobs:
          # This is needed in addition to -yq to prevent apt-get from asking for
          # user input
          DEBIAN_FRONTEND: noninteractive
-        run: sudo apt-get install --no-install-recommends -y g++ binutils flex bison cmake maven jq libxml2-utils openjdk-11-jdk-headless lcov
+        run: sudo apt-get install -y g++ gcc binutils flex bison cmake maven jq libxml2-utils openjdk-11-jdk-headless lcov


I think you're doing yourself a disservice in removing --no-install-recommends. Perhaps use build-essential to get all of g++, gcc, binutils and any other packages really required for building.

Hi Michael, why do you think it's a disservice to ourselves to remove --no-install-recommends? My rationale for removing it is that by keeping it here we may be subtly missing dependencies that are recommended by the system, but they really are essential for our build process, and by missing them we have all sorts of errors.

I guess this makes the process slightly more opaque (in terms of what dependencies will be installed, because they can be changed under our feet) but it's making it safer in terms of making the build much less likely to fail because of missed dependencies. What do you think?

I'm not entirely sure I understand whether you argue for or against --no-install-recommends, but let me try to clarify my position:

We should (ourselves) have a very clear understanding of the dependencies so that we can correctly document these in files like COMPILING.md. We cannot know whether our customers use/don't use --no-install-recommends (or their equivalents in APT configuration files). That said, a GitHub action need not be the one place to test our own understanding of dependencies, but it might well be a good one.

Installing recommended packages might make it easier to get the package lists right, but will likely also install all sorts of packages that we do not need. Installing packages takes time, and CI jobs shouldn't be wasting time.

Hm, I see your point. I'm convinced by point 1., so I'll re-introduce the option as soon as I stabilise an issue I'm having with CI.

Re 2., I can also see this point, but any time savings are marginal. As things are, it downloads about 300MB of packages in about 7 seconds, and it takes another 10-15 seconds to unpack those. At the same time, we take more than 30 minutes to compile and test.

Though the more separate things there are to download, the greater the chance of random network gliches killing a build. GitHub actions seems a lot more reliable than, say, Travis was when we used that, but still worth minimising your 'surface area' for glitches :-)

This is part of our streamlining and cleanup of our CI infrastructure.

This helps us validate that the new job is working as it should, by being the only one that should be submitting results under the currently active token.

There was a problem with symtab2gb and libour_archive.a not being added as a target during coverage builds, whic was causing some of the tests under cbmc/regression/symtab2gb and cbmc/regression/goto-gcc/archives to fail.

hannes-steffenhagen-diffblue · 2021-02-12T10:40:36Z

.github/workflows/pull-request-checks.yaml

+        with:
+          token: ${{ secrets.CODECOV_TOKEN }}
+          files: ./lcov.info
+          fail_ci_if_error: true


❓ Last concern here: Is this a required status check? I don't think it should be, given how codecov has been a bit flaky in the past. IMHO codecov checks should be advisory.

I've marked it fail_ci_if_error so that less coverage (or an error) will cause the specific check to fail, marking it with a red X, so that it's behaviourally consistent with the previous codecov jobs we had.

However, it's not marked as a required check in Github, and as a result, a possible failure of this job doesn't block the merging of a PR.

hannes-steffenhagen-diffblue · 2021-02-12T10:41:32Z

CMakeLists.txt

@@ -149,6 +149,8 @@ if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR
              "$<TARGET_FILE:jbmc>"
              "$<TARGET_FILE:jdiff>"
              "$<TARGET_FILE:smt2_solver>"
+              "$<TARGET_FILE:symtab2gb>"


FWIW all this "$<TARGET_FILE:` stuff in here isn't invalid per se, but it also doesn't help. Just listing the target names here would work just as well and look less confusing.

@hannes-steffenhagen-diffblue Would it be worth doing a cleanup across the code base? Would you mind driving this as my CMake expertise is, well, not really existent?

hannes-steffenhagen-diffblue

Apparently this is not a required check and it seems to work, so LGTM

NlightNFotis requested a review from a team as a code owner February 9, 2021 16:20

NlightNFotis added CI cleanup labels Feb 9, 2021

NlightNFotis self-assigned this Feb 9, 2021

NlightNFotis force-pushed the codecov_gh_actions_port branch from dd96596 to 07b90ce Compare February 9, 2021 17:05

tautschnig approved these changes Feb 9, 2021

View reviewed changes

.github/workflows/pull-request-checks.yaml Outdated Show resolved Hide resolved

.github/workflows/pull-request-checks.yaml Outdated Show resolved Hide resolved

piotr-grabalski approved these changes Feb 10, 2021

View reviewed changes

NlightNFotis requested review from chrisr-diffblue, hannes-steffenhagen-diffblue, kroening, peterschrammel and smowton as code owners February 10, 2021 17:29

peterschrammel approved these changes Feb 10, 2021

View reviewed changes

tautschnig reviewed Feb 11, 2021

View reviewed changes

NlightNFotis force-pushed the codecov_gh_actions_port branch 5 times, most recently from 737a572 to 2c1f941 Compare February 11, 2021 16:07

NlightNFotis added 3 commits February 11, 2021 17:39

Port CMake Codecov job from Codebuild to Github Actions.

1e18a11

This is part of our streamlining and cleanup of our CI infrastructure.

Remove codebuild codecov jobs.

892af6b

This helps us validate that the new job is working as it should, by being the only one that should be submitting results under the currently active token.

Fix issue with coverage targets not being listed as dependencies

f02904e

There was a problem with symtab2gb and libour_archive.a not being added as a target during coverage builds, whic was causing some of the tests under cbmc/regression/symtab2gb and cbmc/regression/goto-gcc/archives to fail.

NlightNFotis force-pushed the codecov_gh_actions_port branch from 2c1f941 to f02904e Compare February 11, 2021 17:41

hannes-steffenhagen-diffblue reviewed Feb 12, 2021

View reviewed changes

hannes-steffenhagen-diffblue approved these changes Feb 12, 2021

View reviewed changes

NlightNFotis merged commit 1cc3af6 into diffblue:develop Feb 12, 2021

NlightNFotis deleted the codecov_gh_actions_port branch February 12, 2021 10:47

[CI] Port CMake Codecov job from Codebuild to Github Actions. #5809

[CI] Port CMake Codecov job from Codebuild to Github Actions. #5809

Uh oh!

Conversation

NlightNFotis commented Feb 9, 2021

Uh oh!

codecov bot commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tautschnig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tautschnig commented Feb 9, 2021

Uh oh!

piotr-grabalski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NlightNFotis commented Feb 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Feb 9, 2021 •

edited

Loading

hannes-steffenhagen-diffblue Feb 12, 2021 •

edited

Loading