search

star-bnl / star-sw

Core software for STAR experiment
26 stars 62 forks source link

Look for local StEvent enumerations first #705

Closed genevb closed 3 months ago

genevb commented 3 months ago

Even though STAR is winding down, there is still some development work ongoing involving FWD detectors. I believe the parsing of StEnumerations.h within StEnumerations.cxx may be making it a little difficult for them because it does not currently allow for a local (developmental) version of enumerations. This PR modified that with a local search first, and hopefully that helps them out.

Note that the parsing is done at run-time, so even with this patch, test jobs need to have not just the compiled code where the job runs, they need also have a local StRoot/StEvent/StEnumerations.h present where the job runs.

p.s. I meant to include @dkapukchyan as a reviewer, but it seems he doesn't show up as someone I can add.

genevb commented 3 months ago

Good point about compiling the full stack and setting $STAR, Jason. Well, @dkapukchyan says this didn't resolve his issue. So, we don't really need to make this change on their behalf. Would you prefer we don't merge this, @klendathu2k ? Doesn't matter much to me either way.

klendathu2k commented 3 months ago

For consistency we should merge. If someone has StEvent checked out... we should be building the detector ID map off of the local copy, in case it has been modified.

Good point about compiling the full stack and setting $STAR, Jason. Well, @dkapukchyan says this didn't resolve his issue. So, we don't really need to make this change on their behalf. Would you prefer we don't merge this, @klendathu2k ? Doesn't matter much to me either way.

plexoos commented 3 months ago

Unless I am missing something, I sense a bit of a contradiction here 😕

For consistency we should merge.

@dkapukchyan says this didn't resolve his issue

klendathu2k commented 3 months ago

You haven't missed anything. This change does not fix the reported issue. But as it is currently implemented, the code is not guaranteed to pickup the local StEnumerations.h file when building the detector ID map. My $0.02 is that this makes it more consistent with a user's reasonable expectation of how it should operate.

Unless I am missing something, I sense a bit of a contradiction here 😕

For consistency we should merge.

@dkapukchyan says this didn't resolve his issue

genevb commented 3 months ago

Re-phrasing:

I saw what appears to be a (small) hurdle for someone developing for detectors. Given that a user (David) was also having an issue with the same enumerations, I hoped that patching to make the hurdle lower might help the user get past their issue, but it didn't. However, we can decide on patching independently of whether it helped this particular user.

I have tested my patch to be sure it works as desired when a local StEnumerations.h is present. Maybe it will help someone.

BTW, I have encouraged the user in question to join the star-bnl group on githbu and submit an issue. So resolving their issue will likely be independent of this PR.