fix(ci): unbound `$PYTHIA8` and deprecate `valgrind` checks

eic / epic-analysis

General (SI)DIS analysis framework for the EIC

GNU Lesser General Public License v3.0

3 stars 9 forks source link

fix(ci): unbound `$PYTHIA8` and deprecate `valgrind` checks #290

Closed c-dilks closed 8 months ago

c-dilks commented 8 months ago

Briefly, what does this PR introduce?

Address CI failures in https://github.com/eic/epic-analysis/pull/289

If Pythia8 libs are in the system default location, there's no need for the (nowadays unbound) $PYTHIA8 environment variable.

What kind of change does this PR introduce?

[x] Bug fix (issue #__)
[ ] New feature (issue #__)
[ ] Documentation update
[ ] Other: __

Please check if this PR fulfills the following:

[ ] Tests for the changes have been added
[ ] Documentation has been added / updated
[x] Changes have been communicated to collaborators: @Gregtom3 @cpecar

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

c-dilks commented 8 months ago

Not clear why the valgrind check fails: https://github.com/eic/epic-analysis/actions/runs/7303577945/job/19904567989?pr=290#step:6:148

Let's just disable it in the CI. This job doesn't fail if leaks are detected, and it would be better to use an address sanitizer anyway.

c-dilks commented 8 months ago

macro/ci/valgrind_check.sh remains available in case valgrind (with the suggested suppressions) is still wanted.

c-dilks commented 8 months ago

Comparsion jobs just crash... @Gregtom3 any idea why?

https://github.com/eic/epic-analysis/pull/290/checks#step:6:167

c-dilks commented 8 months ago

seems like the crash happens here: https://github.com/eic/epic-analysis/blob/37112694ee7daa0ec44a2a71a48323348a78f987/macro/ci/comparator.C#L92-L94

c-dilks commented 8 months ago

It's not happy with

std::vector<HistosDAG*> Dext

I'm not sure what the problem is here... the inheritance is:

HistosDAG : Adage<Histos> : DAG : TObject

So ultimately this is a vector of TObject pointers. Creating std::vector<TObject*> causes no crash, so the problem may be in one of the derived classes.

Some more ideas toward a solution:

compile comparator.C
~use std::vector<HistosDAG> (vector of objects, not pointers)~
use std::vector<std::unique_ptr<HistosDAG>>

c-dilks commented 8 months ago

This PR got a bit side tracked trying to resolve a separate issue. I've opened that issue at https://github.com/eic/epic-analysis/issues/291 since I was unsuccessful in fixing it here.

This PR is ready, since it at least fixes the $PYTHIA8 and valgrind CI issues. (@Gregtom3 @cpecar the comparison jobs will fail, but you should have the power to force-merge this; if not, just approve it (if you think it's okay!) and I can force the merge).