gigabit-clowns / xmipp4-core

Core library of xmipp4
https://gigabit-clowns.github.io/xmipp4-core/
GNU General Public License v3.0
3 stars 0 forks source link

Print leaks on actions when they exist #71

Closed MartinSalinas98 closed 3 weeks ago

MartinSalinas98 commented 4 weeks ago

@oierlauzi could you add the code causing the memleak? (so i can try to detect it and print the logs). I was planning to revert to a commit before it got fixed, but the squash merge denied my plans.

MartinSalinas98 commented 3 weeks ago

tee command does not seem to be properly creating the file then running ctest (works with other commands). Will have to replicate this locally

MartinSalinas98 commented 3 weeks ago

This already works, what do you think @oierlauzi, would it be useful to have this logs shown dynamically in case of memory leaks & stop the pipeline to make sure no new leaks enter the main branch?

MartinSalinas98 commented 3 weeks ago

When you have checked this is correct, I will, locally, reset all changes to reflect main, and then add only the changes in the pipeline, leaving out all the leaky code and removing a lot of commits to leave the history clean

oierlauzi commented 3 weeks ago

What do you mean with "stop the pipeline". I would not merge any code that is leaking.

MartinSalinas98 commented 3 weeks ago

Stop the pipeline means to cause the pipeline to throw an error, therefore returning "red", and not passing the status checks (exit code != 0).

By default ctest with valgrind lets you know that there are memory leaks, but still returns 0, so if you don't enter the log specifically looking for it, you will miss it and merge it.

What my PR does (the part that modifies github action files) is that, if there are any memleaks shown by ctest, it will open the log of each affected test in a separate step (Show test with memory leaks), and do exit false (somewhat like return 1) so the pipeline stops with an erro an does not allow to merge.

oierlauzi commented 3 weeks ago

Great! In this way its almost impossible to merge leaky code

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

oierlauzi commented 3 weeks ago

Maybe could we simplify code instructing valgrind to return a non-zero return code on failure? https://stackoverflow.com/questions/55321934/how-to-let-cmake-ctest-memcheck-exit-with-status-code-1-on-failure

MartinSalinas98 commented 3 weeks ago

Maybe could we simplify code instructing valgrind to return a non-zero return code on failure? https://stackoverflow.com/questions/55321934/how-to-let-cmake-ctest-memcheck-exit-with-status-code-1-on-failure

There are two issues with that.

  1. It's not on valgrind, it's on ctest.
  2. Even if it worked as intended, it would not show the logs of the affected tests, which means the developer would need to reproduce it locally.
oierlauzi commented 3 weeks ago

Maybe could we simplify code instructing valgrind to return a non-zero return code on failure? https://stackoverflow.com/questions/55321934/how-to-let-cmake-ctest-memcheck-exit-with-status-code-1-on-failure

There are two issues with that.

1. It's not on valgrind, [it's on ctest](https://stackoverflow.com/questions/55321934/how-to-let-cmake-ctest-memcheck-exit-with-status-code-1-on-failure#comment97378412_55324752).

2. Even if it worked as intended, it would not show the logs of the affected tests, which means the developer would need to reproduce it locally.
  1. ctest returns 0 because valgrind did.
  2. The log of valgrind is not that helpful, in any case local reproducibility with valgrind and gdb would be required to find the root issue.
oierlauzi commented 3 weeks ago

I would merge this code and see later if it can be simplified

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud