Speed up coverage job ci

[mcbarton]

Given when coverage is turned on CppInterOp is built with debug options, when cppyy makes use of it its tests run much slower. This PR should fix this issue, by rebuilding CppInterOp as a Release version with no debug options once the coverage report has been uploaded. It takes extra time to build CppInterOp again, but this should be much less time then how much longer it takes to run the cppyy tests when CppInterOp is built with debug options.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.31%. Comparing base (f93e7b6) to head (ee4b544).
Report is 31 commits behind head on main.

❗ Current head ee4b544 differs from pull request most recent head 9998713. Consider uploading reports for the commit 9998713 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #283       +/-   ##
===========================================
- Coverage   87.65%   54.31%   -33.35%     
===========================================
  Files           4        8        +4     
  Lines        1742     2957     +1215     
===========================================
+ Hits         1527     1606       +79     
- Misses        215     1351     +1136     

see 8 files with indirect coverage changes

see 8 files with indirect coverage changes

[mcbarton]

This PR saves almost 10 minutes off the coverage job.

[vgvassilev]

Do we have guidance from codecov if building in Release would produce useful information? We need to relate the coverage information against the source code.

[mcbarton]

Do we have guidance from codecov if building in Release would produce useful information? We need to relate the coverage information against the source code.

@vgvassilev The release build is done after the code coverage report is produced and uploaded based on the debug version. This change cannot effect the outcome of the coverage report.

[mcbarton]

@vgvassilev can this PR be reviewed too?

[aaronj0]

Hi @mcbarton

Looks good to me! If I understand correctly this PR rebuilds CppInterOp in release mode after the codecov report.

[mcbarton]

Hi @mcbarton

Looks good to me! If I understand correctly this PR rebuilds CppInterOp in release mode after the codecov report.

@maximusron That's exactly what its doing. Saves approximately 10 minutes of time for that job due to the speed up in the time to do the cppyy tests. Can it be merged?

[aaronj0]

@mcbarton the environment variables and paths remain the same after the codecov job so this commit should perform the same minimally

[aaronj0]

@vgvassilev this works for me but the only con would be that when we ssh into the runners to debug, we will have to rebuild in debug mode. Seems like a fair enough trade-off for a faster CI

[mcbarton]

@vgvassilev this works for me but the only con would be that when we ssh into the runners to debug, we will have to rebuild in debug mode. Seems like a fair enough trade-off for a faster CI

@maximusron I have made this change. Are you happy with it?

[aaronj0]

@mcbarton I think the last commit is not required, since we would probably need to rebuild all the way to cppyy after this which is something we can do manually (these builds also add overheads)

[aaronj0]

we can drop this rebuild

[mcbarton]

You mentioned that if you have to ssh into the machine it will need to be rebuilt with debug. This saves time by getting the ci to rebuild, and also guards again the case the person sshing into the macine forgets to (we know too, but others may not).

[mcbarton]

@maximusron I can revert this change if your sure but I think its a good guard against those sshing in.

[vgvassilev]

We can make it conditional on if the runner is run in debug.

[mcbarton]

We can make it conditional on if the runner is run in debug.

@vgvassilev Currently we have no debug flag/mode. I can make it dependent on a debug run when I address #262 soon if that's ok?

[mcbarton]

@mcbarton I think the last commit is not required, since we would probably need to rebuild all the way to cppyy after this which is something we can do manually (these builds also add overheads)

I'd argue the build is not an overhead since its only rebuilt if the job fails.

[aaronj0]

I realise that the valgrind commands on cppyy and the unittests need CppInterOp to remain in debug mode.

Otherwise don't get any function names or line number information. Using Valgrind on release build finds fewer or no memory issues since llvm has memory pool management of its own. In release mode valgrind will not be able to see the alloc/dealloc mechanism.

[mcbarton]

I realise that the valgrind commands on cppyy and the unittests need CppInterOp to remain in debug mode.

Otherwise don't get any function names or line number information. Using Valgrind on release build finds fewer or no memory issues since llvm has memory pool management of its own. In release mode valgrind will not be able to see the alloc/dealloc mechanism.

I realise that the valgrind commands on cppyy and the unittests need CppInterOp to remain in debug mode.

Otherwise don't get any function names or line number information. Using Valgrind on release build finds fewer or no memory issues since llvm has memory pool management of its own. In release mode valgrind will not be able to see the alloc/dealloc mechanism.

@maximusron Wouldn't this be true of the for llvm versions too since they perform the Valgrind commands too? If yes, do we need a debug version of all the Ubuntu jobs? Once this comment is answered i'll close this PR with a link to your comment.

[aaronj0]

@maximusron Wouldn't this be true of the for llvm versions too since they perform the Valgrind commands too? If yes, do we need a debug version of all the Ubuntu jobs? Once this comment is answered i'll close this PR with a link to your comment.

I think a debug build works but at the same time we seem to be departing from having the CI itself a debug mode testing platform, I do think having the CI run everything in debug mode (for unique build types since we have a multitude of compiler, OS and CPU platform specific errors). This way we catch more stuff and have good logs as a first response. I don't think the need for the builds to be blazing fast warrants this change

Also the builds have been fast and easy to work since your recent upgrades :)

@vgvassilev what do you think?

[mcbarton]

@maximusron Wouldn't this be true of the for llvm versions too since they perform the Valgrind commands too? If yes, do we need a debug version of all the Ubuntu jobs? Once this comment is answered i'll close this PR with a link to your comment.

I think a debug build works but at the same time we seem to be departing from having the CI itself a debug mode testing platform, I do think having the CI run everything in debug mode (for unique build types since we have a multitude of compiler, OS and CPU platform specific errors). This way we catch more stuff and have good logs as a first response. I don't think the need for the builds to be blazing fast warrants this change

Also the builds have been fast and easy to work since your recent upgrades :)

@vgvassilev what do you think?

@maximusron @vgvassilev I will address the debug run issue by tackling issue #262 . I will make a PR which adds a debug flag, which does different things if we feels its needed, so its not done all the time.

[vgvassilev]

I think RelWithDebInfo should be fine for the CI. That essentially turns on assertions and it is still fast. Maybe that's a good common ground. The downside is that if something fails on a particular bot we have to rebuild everything in debug mode but that happens rarely. I guess we can revisit if necessary...

[mcbarton]

I think RelWithDebInfo should be fine for the CI. That essentially turns on assertions and it is still fast. Maybe that's a good common ground. The downside is that if something fails on a particular bot we have to rebuild everything in debug mode but that happens rarely. I guess we can revisit if necessary...

@vgvassilev @maximusron This doesn't work, as turning on codecov turns on debug mode, regardless of what we put for the build type (Errors here https://github.com/compiler-research/CppInterOp/blob/791995a7b518226bbf9003ad0e13f35ec39e4705/CMakeLists.txt#L282C1-L282C63). Do you know if codecov works with RelWithDebInfo? If not, I might close this PR and revisit the issue at some point in the future.

[aaronj0]

This doesn't work, as turning on codecov turns on debug mode, regardless of what we put for the build type

ah that is a problem... https://github.com/bilke/cmake-modules/blob/1fcf7f4a2179a7b649be15473906882871ef5f0b/CodeCoverage.cmake#L213-L214 this does seem to be the case. We need to have a non optimised build to ensure the best results from codecov

Overall, with valgrind and codecov things point towards us retaining a debug build on the CI. I don't think this is much of a priority given that the builds are much faster and easy to work with now. Maybe we can drop this PR

[mcbarton]

This doesn't work, as turning on codecov turns on debug mode, regardless of what we put for the build type

ah that is a problem... https://github.com/bilke/cmake-modules/blob/1fcf7f4a2179a7b649be15473906882871ef5f0b/CodeCoverage.cmake#L213-L214 this does seem to be the case. We need to have a non optimised build to ensure the best results from codecov

Overall, with valgrind and codecov things point towards us retaining a debug build on the CI. I don't think this is much of a priority given that the builds are much faster and easy to work with now. Maybe we can drop this PR

I'll drop this PR, and only revisit the issue if we feel there is a need to in future. @maximusron can you close this issue #221 , as I feel its been resolved now?