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

GHA: Update Ubuntu version to 20.04 #160

Merged
merged 56 commits into from
Apr 17, 2023
Merged

GHA: Update Ubuntu version to 20.04 #160

merged 56 commits into from
Apr 17, 2023

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Apr 12, 2023

Github actions removed the runner for Ubuntu 18.04, which is why ABIN CI tests were not running. Here I update all jobs to Ubuntu 20.04.

This unfortunately means we can no longer test with gfortran-7.
@JanosJiri for your PRs, please always manually test on Argon or Neon with GCC 7.3.

EDIT: I figured out how to install GCC-7 so we're good here.

@danielhollas danielhollas requested a review from JanosJiri April 12, 2023 14:58
@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #160 (39dc950) into master (31b1bb6) will decrease coverage by 1.49%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   93.90%   92.41%   -1.49%     
==========================================
  Files          43       43              
  Lines        6005     6053      +48     
  Branches        0      735     +735     
==========================================
- Hits         5639     5594      -45     
- Misses        366      447      +81     
- Partials        0       12      +12     
Flag Coverage Δ
unittests 24.82% <75.00%> (-0.38%) ⬇️

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

Impacted Files Coverage Δ
src/force_mm.F90 100.00% <100.00%> (ø)
src/nosehoover.F90 91.66% <100.00%> (-4.87%) ⬇️

... and 12 files with indirect coverage changes

@danielhollas
Copy link
Contributor Author

Code coverage is not working for certain builds (FFTW, Plumed, OpenMPI), working on it...

@danielhollas
Copy link
Contributor Author

danielhollas commented Apr 15, 2023

There are some residual coverage changes that I haven't been able to resolve, which mainly come from the FFTW build (so affects normal mode PIMD/PIGLET). It looks like gcc/gcov versions >7 have some bugs which means some statements (e.g. allocate/deallocate) look like not covered even though they clearly are. For other builds it's not a problem since we still build with GCC7 at least once. But FFTW build must run on GCC version that is default for the given Ubuntu version, because we install libfftw via apt. If we wanted to switch the FFTW build to GCC-7, we would need to compile libfftw ourselves, otherwise there's a conflict. This seems like too much work so we will take the coverage hit for now, and hope it will get resolved in future GCC versions. I have also tested various compiler options but none of them fixed the situation.

Note that some coverage changes seem to be real, specifically the lines that became "partially" covered, see the code on codecov for details. These come from the if statements that are not fully exercised in terms of "branch coverage", meaning that the if statement is either always true, or always false, end hence not all possible code branches are exercised.

For example, the following snippet from plumed.F90

obrazek

The if statement is on line 73 is only partially covered, because it always evaluates to false (as can be easily seen since the next line in the if branch is not covered.

@danielhollas
Copy link
Contributor Author

I have enabled extra bound checks and runtime memory leak detection on some of the builds. (I couldn't enable it for all builds because it messes up with the code coverage). This uncovered one minor out-of-bounds array access in NHC initialition, which was however not critical since it was happening only in the error path. I also uncovered a memory leak in the pFUnit library that we use for unit tests, which I reported here: Goddard-Fortran-Ecosystem/pFUnit#419

Copy link
Contributor

@JanosJiri JanosJiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the gfortran.yml and codecov.yml staff but the rest of it seemed OK for me.

@JanosJiri JanosJiri merged commit 70a1327 into master Apr 17, 2023
@danielhollas
Copy link
Contributor Author

@JanosJiri thanks for taking a look. I'll walk you through the CI settings once you're back in Bristol.

In terms of merging, for PRs like these with a large number of commits (56!) it is better to use the "Squash and merge" strategy, which means that all the commits will be squashed to one, resulting in much cleaner commit history. I will fix this up on master (but will have to force push to master, which is not ideal, you might need to fix up your local repo if you've pulled master already).

@danielhollas danielhollas deleted the gha-ubuntu-update branch April 17, 2023 20:51
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

Successfully merging this pull request may close these issues.

2 participants