JeffersonLab / iguana

Preservation of common physics data analysis algorithms. Currently focused on HIPO data.
https://jeffersonlab.github.io/iguana/
GNU Lesser General Public License v3.0
2 stars 6 forks source link

feat: add ROOT macro example #210

Closed c-dilks closed 5 months ago

c-dilks commented 5 months ago

close #159

TODO

c-dilks commented 5 months ago

@dglazier, this adds a simple ROOT macro example, which relies on ROOT_INCLUDE_PATH and {,DY}LD_LIBRARY_PATH to be set correctly; this_iguana.sh will now prepend ROOT_INCLUDE_PATH, and we can make sure the modulefile does as well.

Will this work for you, or do you need anything else to support clas12root integration?

dglazier commented 5 months ago

For clas12root I do not use the .sh file. It just needs IGUANA set to point to the install directory. I was able to get round ROOT_INCLUDE_PATH by adding

   gInterpreter->AddIncludePath(IGUANA+"/include");

To the Load script. Previously I had gSystem->AddIncludePath which does not work for some reason. Is there anything in the .sh file I may have missed ?

c-dilks commented 5 months ago

For clas12root I do not use the .sh file. It just needs IGUANA set to point to the install directory. I was able to get round ROOT_INCLUDE_PATH by adding

   gInterpreter->AddIncludePath(IGUANA+"/include");

To the Load script. Previously I had gSystem->AddIncludePath which does not work for some reason. Is there anything in the .sh file I may have missed ?

I also noticed gSystem->AddIncludePath wasn't working, but gInterpreter was. I guess the main question here is whether we want to do it that way or use ROOT_INCLUDE_PATH.

The .sh file uses pkg-config variables to set the environment variables; since clas12root uses CMake, perhaps it could use pkg_get_variable to access the variables dep_includedirs and includedir (and others, if needed).

dglazier commented 5 months ago

For clas12root I prefer users just need to set 1 variable e.g. IGUANA, HIPO, QADB,... clas12root can then handle the linking at runtime, rather than have users having to set ROOT_INCLUDE_PATH etc.

c-dilks commented 5 months ago

What about users who want to run iguana with a ROOT macro, not using clas12root? If ROOT_INCLUDE_PATH isn't set, then they have to figure out the paths themselves, or we could do it for them, something like:

// somehow get the `stdout` of these, handle errors, etc.
gROOT->ProcessLine(".! pkg-config iguana --variable include")
gROOT->ProcessLine(".! pkg-config iguana --variable dep_includedirs")

// concatenate, tokenize by `:`, then for each path:
gInterpreter->AddIncludePath("-I" + path);

This sort of thing could be buried in a loadIguana.C, similar to LoadClas12Root.C; meson could even be used to generate such a file, to avoid having to run pkg-config at all (but then that might worsen #212).

OTOH, with ROOT_INCLUDE_PATH, the only other things a user needs in their ROOT macro is gSystem->Load("libIguanaAlgorithms") and the include directives. On ifarm this variable can be set for them in the module file; on local builds, users will either install to the system default location (probably not a great idea) or will source this_iguana.sh.

The .sh file uses pkg-config variables to set the environment variables; since clas12root uses CMake, perhaps it could use pkg_get_variable to access the variables dep_includedirs and includedir (and others, if needed).

This might be what you want if you want to avoid ROOT_INCLUDE_PATH in clas12root; you could use the results in gInterpreter->AddIncludePath calls.

dglazier commented 5 months ago

I think ROOT users should absolutely use the way you think is best. I was just referring to CLAS12ROOT usage. I don't want to include iguana in the build as it is not necessary, but just load the libs and includes at run-time, this is done simply with the environment variables. It looks like in general there can be some conflicts for FMT at least which can be fixed by ROOT_INCLUDE_PATH, or by including an FMT_IGUANA variable, and me modifying LoadIguana.C to use it. The advantage of environment variables is they are simple to use and do not require additional knowledge or dependencies. At some universities students will be on very old systems and will not be able to debug or fix build errors or have access to pkg or modern build systems. Also they come at no extra cost, you could just set these things in the .sh script, which on ifarm presumably gets passed into the clas12 environment. Then I am all done and do not need to do anymore work on this for now. You could even set IGUANA_INCS and IGUANA_LIBS if you are worried about $IGUANA/include not being safe. I appreciate the pkg-config and iguana build system are more optimal, however I have no time to make significant changes to clas12root build and it seems there is a straightforward solution. I am happy for Hall B to take over maintenance of any of the clas12root stuff and fix this (and CI).

c-dilks commented 5 months ago

As discussed in the discourse thread, we'll set an environment variable to the prefix, since we need something like this to fallback on for #212 resolution. ROOT_INCLUDE_PATH can be used for headers and (DY)LD_LIBRARY_PATH for libraries.

Also they come at no extra cost, you could just set these things in the .sh script, which on ifarm presumably gets passed into the clas12 environment.

The cost is dependence on a globally mutable state. No environment variables should be needed if things are installed in default locations, otherwise I believe the only choices are either standard environment variables such as linker paths, or modifying installed binary runpaths (breaking relocatability). However, many of us don't install CLAS software to the default locations.

The environment variables are defined separately in the modulefiles repo, which violates DRY but that's the preference to help keep things consistent from the environment maintainer's point of view.

We envision containerizing all this software; then we don't have to worry so much about the environment since everything can go in a common installation prefix.

I appreciate the pkg-config and iguana build system are more optimal, however I have no time to make significant changes to clas12root build and it seems there is a straightforward solution. I am happy for Hall B to take over maintenance of any of the clas12root stuff and fix this (and CI).

We definitely appreciate your work and feedback here; discussions like this are critical to the design process. The best outcome we can hope for is that no user notices any of this... things just work for them and everything is easy. Let's prioritize getting things working; we can always make improvements later.