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

CI improvements #103

Open
10 of 26 tasks
N-Coder opened this issue Oct 24, 2023 · 7 comments
Open
10 of 26 tasks

CI improvements #103

N-Coder opened this issue Oct 24, 2023 · 7 comments

Comments

@N-Coder
Copy link
Member

N-Coder commented Oct 24, 2023

  • check why we currently have no cache hits on Linux
  • fix cache pruning jobs always pruning master instead of the branch that triggered the process
  • fine tune llvm-cov to report good data for sonar coverage
  • collect cache sizes/usages/hit rate, runner architectures, build and test timing in a summary
  • store ccache to external storage / only for master
  • automatically build and publish docker containers
  • check whether precompiled headers might speed up builds (see CI Improvements #238 and 2e42c1d)
  • Optionally migrate simple checks to pre-commit. This tool provides a wide range of simple checks similar to the ones we already perform in the "style" stage and many more (I collected some here). Plus, it makes it easy to run these quick checks in a git pre-commit hook, potentially saving you a loop via the CI if you e.g. forgot the final newline in a file.

Orga / Config

MacOS

  • disable -Wdeprecated-declarations for ogdf/src/coin and src/ogdf/lib (MacOS warning that sprintf is deprecated, could be suppressed via CMake like this, but we probably should update the dependencies / fix this somehow)
  • check why OGDF_ARCH/mtune=native breaks on M1
  • add Apple Silicon runners to CI

Done

  • Enable SonarQube again, e.g. using the respective ready-made Action. Instead, we could also use the SonarCloud SaaS option, as SonarCloud is entirely free for all open source projects and we would thus no longer need to maintain a dedicated SonarQube machine. SonarCloud also has a ready-made Action for scanning C++ projects. Remove test_coverage.sh and and sonar-related files from repository afterwards.
  • Reduce build times for clang:15 (main culprit is probably clang-tidy, see here for possible speed-up methods).
  • make it more explicit when clang-tidy is run (currently on all clang debug builds, see compile_for_tests.sh)
  • use clang-tidy-diff to only check changes in a PR, make this passing a requirement
  • cache clang-tidy results for unchanged files
  • report test coverage (changes) via SonarSource, Coveralls or CodeCov (see existing test_coverage.sh)
    For SonarSource, this would require running their whole static analysis in our CI, but it would then also be able to collect clang-tidy and cppchecker reports.
  • report issues with building the docs
  • enable SonarCloud checks for fork pull requests
  • fix ccache leading to SIGILL(egal instruction)s on Mac OS (there are no exact infos on which hardware could be used)
  • see also things resolved in CI Improvements #238
@N-Coder
Copy link
Member Author

N-Coder commented Oct 31, 2023

Migrated ToDos from GitLab [1] and [2]

@N-Coder

This comment was marked as outdated.

@N-Coder
Copy link
Member Author

N-Coder commented Dec 3, 2023

SonarCloud currently reports a very low branch coverage, this is probably because every potential failure in an OGDF_ASSERT macro is counted as a possible (and usually uncovered) branch. We should instruct gcovr to exclude these branches from the report that will be imported into SonarCloud, see also here. SonarCloud seems to have no option to do so after the report is imported.

@N-Coder
Copy link
Member Author

N-Coder commented Dec 3, 2023

Currently, we use the following coverage generation pipeline:

clang++ --coverage -> gcovr --gcov-executable="llvm-cov gcov" --sonarqube -> sonar.coverageReport

Alternative pipelines would be:

g++ --coverage -> gcovr -> sonar.cfamily.gcov.report

or:

clang++ -fprofile-instr-generate -fcoverage-mapping -> llvm-profdata merge -> llvm-cov show -> sonar.cfamily.llvm-cov.report

There are multiple variables here:

  • which compiler (clang or gcc)
  • which coverage format (gcov --coverage or llvm-cov -fprofile-instr-generate -fcoverage-mapping [1], where the latter is only supported by clang)
  • which report format (sonarqube-specifc json, gcov json or llvmc-cov text output [4])

It seems that gcc bases its branches on the jump commands in the generated machine code [5], while clang uses its AST for the branch analysis and is thus much more precise [1]. gcov-style reports as generated when passing --coverage can be collected by gcovr, while llvm-style reports are collected by using llvm-profdata merge [1]. While gcovr can filter the coverage data and produce different outputs [3], llvm-cov show can do neither [2]. I ran a few tests with the three different variants and included the results in cov-test.zip. Here, the plain report generated using the third pipeline seems to match the expectations and OGDF_ASSERT also does not generate unwanted branches as long as we do not turn macro expansion on, while the gcovr based reports contain a lot of unwanted branches (which are even hard to entirely remove with all available gcovr filters). So all in all, we should probably migrate to the third, entirely clang-based / gcov-free pipeline that yields better results while not being able to use but also not needing filters. Note that I haven't tested the different import formats into SonarCloud yet.

[1] https://clang.llvm.org/docs/SourceBasedCodeCoverage.html
[2] https://llvm.org/docs/CommandGuide/llvm-cov.html
[3] https://gcovr.com/en/stable/manpage.html
[4] https://docs.sonarsource.com/sonarcloud/enriching/test-coverage/c-c-objective-c-test-coverage/
[5] https://gcovr.com/en/stable/faq.html#why-does-c-code-have-so-many-uncovered-branches

@N-Coder
Copy link
Member Author

N-Coder commented Dec 6, 2023

Also, the thrown exceptions in OGDF_ASSERT with OGDF_USE_ASSERT_EXCEPTIONS=ON not only confuse compilers when assertions are checked e.g. in destructors or noexcept methods (which is why we now have OGDF_DISABLE_WARNING_THROW_TERMINATE), but they also confuse SonarCloud to produce similar false warnings and seem to reduce coverage. So we should at least turn them off for the sonar analysis and also ensure that they don't affect coverage (e.g. by not expanding these macros in the third pipeline).

@N-Coder

This comment was marked as outdated.

@milsen

This comment was marked as outdated.

@N-Coder N-Coder mentioned this issue Apr 23, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants