[root6] Add include statements to StarDb macros

[klendathu2k]

ROOT6/cling needs to have include files for the table classes used in these macros.

This PR consists of one single change... to many files: Adding the lines

#ifdef __CLING__
#  include "tables/TABLECLASSHEADER.h"
#endif

at the top of each macro. We only make this change for ROOT6/cling, as ROOT5/cint is happy with the status-quo. (And ROOT5 does not like the added includes.)

This change has been tested for a single nightly test job (job 10).

[plexoos]

This change has been tested for a single nightly test job (job 10).

Since you mention it, is it something that is not covered by the CI tests?

[veprbl]

This change has been tested for a single nightly test job (job 10).

Since you mention it, is it something that is not covered by the CI tests?

The root6 ci on github ignores the failures. It was setup to track the progress for porting. I can't see how this PR alone can improve the current crashes.

[plexoos]

The root6 ci on github ignores the failures. It was setup to track the progress for porting. I can't see how this PR alone can improve the current crashes.

I am very well aware of that. That's why I want Jason to clarify what he means by that statement. (I didn't even know that nightly tests had IDs one could use to refer to them)

[klendathu2k]

This PR is necessary, but not sufficient, to make the nightly tests / CI tests run. It has been tested under root 5 to confirm that our code still works. For root 6, there are a couple of other changes that need to be made to the code to get to the point where this PR is necessary. These will be future PR's.

[plexoos]

We only make this change for ROOT6/cling, as ROOT5/cint is happy with the status-quo. (And ROOT5 does not like the added includes.)

I think ROOT5 cannot find the includes because the search paths are not set properly for the interpreter. Hopefully, #441 can solve this, and if it works for ROOT5 we can leave out the #ifdef __CLING__ statements.

[klendathu2k]

We only make this change for ROOT6/cling, as ROOT5/cint is happy with the status-quo. (And ROOT5 does not like the added includes.)

I think ROOT5 cannot find the includes because the search paths are not set properly for the interpreter. Hopefully, #441 can solve this, and if it works for ROOT5 we can leave out the #ifdef __CLING__ statements.

No objections if it works.

[plexoos]

Looks like the header is not generated due to a missing idl file (?)

https://github.com/star-bnl/star-sw/actions/runs/3464168743/jobs/5787237505

LoadTable: .L /star-sw/StarDb/ctf/ctg/ctb.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/ctb_slat.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/ctb_slat_eta.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/ctb_slat_phi.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/tof.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/tof_slat.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/tof_slat_eta.C
LoadTable: .L /star-sw/StarDb/ctf/ctg/tof_slat_phi.C
LoadTable: .L /star-sw/StarDb/ctf/cts/cts_ctb.C
LoadTable: .L /star-sw/StarDb/ctf/cts/cts_tof.C
St_sls_Maker:INFO  - TAttr::Found : .histos = 1

Error: cannot open file "tables/St_sce_ctrl_Table.h"  /star-sw/StarDb/svt/ssd/sce_ctrl.C:2:
*** Interpreter error recovered ***
root4star: .sl79_gcc485/OBJ/StRoot/St_db_Maker/St_db_Maker.cxx:932: virtual TDataSet* St_db_Maker::LoadTable(TDataSet*): Assertion `!ee' failed.
LoadTable: .L /star-sw/StarDb/svt/ssd/sce_ctrl.C
Error: Process completed with exit code 1.

[klendathu2k]

Looks like the header is not generated due to a missing idl file (?)

... So... this comes back to a fundamental difference between root5/CINT and root6/cling.

1 TDataSet *CreateTable() {
2 // -----------------------------------------------------------------
3 // sce_ctrl Allocated rows: 1  Used rows: 1  Row size: 84 bytes
4 //  Table: sce_ctrl_st[0]--> sce_ctrl_st[0]
5 // ====================================================================
6 // ------  Test whether this table share library was loaded ------
7   if (!gROOT->GetClass("St_sce_ctrl")) return 0;
8 sce_ctrl_st row;
9 St_sce_ctrl *tableSet = new St_sce_ctrl("sce_ctrl",1);
10 // 
...
33 tableSet->AddAt(&row,0);
34 // ----------------- end of code ---------------
35  return (TDataSet *)tableSet;
36 }

CINT is a line-by-line c++ interpreter. When we reach line 7 and note that the class does not exist in root's class table, we return a null pointer. ROOT never reaches lines 8 & 9, which could invoke root's facilities to try to load in the header files.

On the other hand, cling has to compile the entire file. It looks ahead, notes that we need to load include files, and tries to load them. And fails.

Doing a quick grep through the codes... I believe that the sce_ctrl.C, scf_ctrl.C, scm_ctrl.C and sls_ctrl.C files can be deprecated...

Just pushed test code... if this succeeds, perhaps we remove these files.

[plexoos]

Doing a quick grep through the codes... I believe that the sce_ctrl.C, scf_ctrl.C, scm_ctrl.C and sls_ctrl.C files can be deprecated...

Just pushed test code... if this succeeds, perhaps we remove these files.

Yes, I confirm that the missing types supposed to be generated for those DB tables are not used anywhere in StRoot/ It should be safe to remove the files:

$ ls StarDb/svt/ssd/*_ctrl.C
StarDb/svt/ssd/sce_ctrl.C  StarDb/svt/ssd/scf_ctrl.C  StarDb/svt/ssd/scm_ctrl.C  StarDb/svt/ssd/sls_ctrl.C

[klendathu2k]

Search paths are causing a side effect that is doesn't let me run the nightly tests... but I can avoid loading the rootlogon and keep testing. I think we can merge.

[plexoos]

Search paths are causing a side effect that is doesn't let me run the nightly tests... but I can avoid loading the rootlogon and keep testing.

It is something for the Production team to figure out preferably before we merge. I don't know how they set up .rootrc or rootlogon.C/rootlogoff.C macros on the farm. In the CI jobs we obviously use these the current revisions from StRoot/macros/

[genevb]

The production environment is just like any other STAR user environment on the SDCC farm. If a different approach is being used for github CI tests, I don't see figuring that out as a Production Team responsibility.

Thanks, -Gene

On Nov 16, 2022, at 10:08 AM, Dmitri Smirnov @.***> wrote:

Search paths are causing a side effect that is doesn't let me run the nightly tests... but I can avoid loading the rootlogon and keep testing.

It is something for the Production team to figure out preferably before we merge. I don't know how they set up .rootrc or rootlogon.C/rootlogoff.C macros on the farm. In the CI jobs we obviously use these the current revisions from StRoot/macros/

[plexoos]

The production environment is just like any other STAR user environment on the SDCC farm.

Of course not. You can't possibly know how users set up their environments.

If a different approach is being used for github CI tests, I don't see figuring that out as a Production Team responsibility.

So, what is the right approach? Could you elaborate and guide us through if you think this is some else's responsibility.

[genevb]

Hi, Dmitri

On Nov 16, 2022, at 12:11 PM, Dmitri Smirnov @.***> wrote: The production environment is just like any other STAR user environment on the SDCC farm.

Of course not. You can't possibly know how users set up their environments.

Indeed, I left out the word "default" before "STAR user environment". Thanks for the correction as we do not prevent people from customizing their own.

If a different approach is being used for github CI tests, I don't see figuring that out as a Production Team responsibility.

So, what is the right approach? Could you elaborate and guide us through if you think this is some else's responsibility.

Two items I can think of:

(1) The Production Team does not manage/maintain the default STAR user environment. That has been a task under the care of Jerome historically as an infrastructure task, not as a production task. The Production Team cares if the environment changes in an impactful way and may have to make adjustments for such changes as one of the users, albeit a very important user.

(2) The default STAR user environment on SDCC is, to my knowledge, considered the official environment under which STAR software must work. If the environment on github for CI differs from that, I expect the burden is on the maintainer of the non-official environment to match the official environment. But if anyone identifies something about the official environment that could benefit from a specific improvement, see item (1).

Thanks, -Gene

[plexoos]

Gene, you are leaving out a very important point. It is the production team who installs the STAR software on the farm. As far as I know nobody else can do it. So, for this change to work properly and to move us closer to ROOT6, the production team (as one of the users of the STAR software) needs to either use the StRoot/macros/rootlogon.C provided in the code tree or update their own accordingly. ROOT also provides a mechanism to pick a specific rootlogon/rootlogoff.C via .rootrc. Again, you can use the one in StRoot/macros or come up with your own or suggest changes to all of these files. These are the handles to run the code but if you have a better suggestion how to accommodate this change please let us know.

[genevb]

The production user starreco does not use a custom environment:

root -l -b Float Point Exception is OFF *** Start at Date : Wed Nov 16 15:30:55 2022 QAInfo:You are using STAR_LEVEL : pro, ROOT_LEVEL : 5.34.38 and node : rcas6003.rcf.bnl.gov root.exe [0] .which .rootrc /afs/rhic.bnl.gov/star/packages/SL22b/StRoot/macros/.rootrc root.exe [1] .which rootlogon.C /afs/rhic.bnl.gov/star/packages/SL22b/StRoot/macros/rootlogon.C

Does that help? -Gene

On Nov 16, 2022, at 3:12 PM, Dmitri Smirnov @.***> wrote:

the production team (as one of the users of the STAR software) needs to either use the StRoot/macros/rootlogon.C provided in the code tree or update their own accordingly. ROOT also provides a mechanism to pick a specific rootlogon/rootlogoff.C via .rootrc. Again, you can use the one in StRoot/macros or come up with your own or suggest changes to all of these files.

[plexoos]

Does that help?

I don't know. If you say it is safe to merge this we'll do it. I thought something did not work for Jason...

[genevb]

On Nov 16, 2022, at 3:53 PM, Dmitri Smirnov @.***> wrote:

Does that help?

I don't know. If you say it is safe to merge this we'll do it. I thought something did not work for Jason...

I do not know whether it is safe to merge in a final sense. I do not know the details of Jason's tests, like whether his search paths are what they would be in the default STAR dev environment. It is certainly preferable that Jason or another user attempts to replicate that environment for testing, but a fall-back option may be to allow a quickly-reversible merge to go through to DEV for one night (we are not running FastOffline, so DEV offers a little more flexibility now than during experiment Runs).

-Gene

[veprbl]

If Jerome had left STAR, why we still can't just give someone else the permissions and assign the responsibility?

[klendathu2k]

Sorry, I lost track of some of this discussion.

When the include paths are added via the root logon... root 6 complains when trying to autoload headers. So there is a conflict somewhere that i don't understand. And I infer from other parts of this discussion that it is environment dependent. So ... I would propose that the rootlogon change be a separate pull request, which we address after setting up an intial test library.

[plexoos]

I would propose that the rootlogon change be a separate pull request

441 ? It is already merged

I don't understand what problem you are experiencing. If you need help please provide a how to reproduce example.

[klendathu2k]

I would propose that the rootlogon change be a separate pull request

441 ? It is already merged

I don't understand what problem you are experiencing. If you need help please provide a how to reproduce example.

I guess the issue was unrelated to the include paths. Or my environment doesn't load in the rootlogon. I am assuming that this PR #439 is still to be merged? Or is the belief that #441 will resolve the problem with StarDb?

[plexoos]

441 is needed to get rid of #ifdef __CLING__ statements which is the current state of this PR branch. The CI tests pass and it is absolutely fine with me to merge this PR into main ASAP.

However, Jason your own comments above indicated that there might be a problem with using this on the farm. I warned the production team that they may have a similar problem. That is it. If the production team does not want to test or double check this before it goes into main, it is their choice. As I noted above, I don't even understand what the problem is or if there is a problem :)

[klendathu2k]

Well... the CLING guard was in place just to isolate this change to root 6...

Let's move forward on everything else but this PR. I can then test to see if the issue I was seeing when I tried the new root logon was real or pilot error. And then move forward from there.

[plexoos]

the __CLING__ guard was in place just to isolate this change to root 6...

Yes, and as you can see from the CI tests it works perfectly for ROOT5 too. We don't want to create branches in our code unnecessarily