add boost to ROOT INCLUDE PATH

[shahor02]

Hi @dberzano @ktf

Also the aliBuild-installed boost needs to be added to the ROOT_INCLUDE_PATH
to have the macros compilable. Otherwise we get e.g.:

.L /home/shahoian/alice/sw/ubuntu1710_x86-64/O2/dev-1/share/macro/run_match_TPCITS.C+
...
In file included from /home/shahoian/alice/sw/ubuntu1710_x86-64/O2/dev-1/share/macro/run_match_TPCITS.C:14:
In file included from /home/shahoian/alice/sw/ubuntu1710_x86-64/O2/dev-1/include/GlobalTracking/MatchTPCITS.h:25:
In file included from /home/shahoian/alice/sw/ubuntu1710_x86-64/O2/dev-1/include/DataFormatsTPC/TrackTPC.h:18:
/home/shahoian/alice/sw/ubuntu1710_x86-64/O2/dev-1/include/DataFormatsTPC/Cluster.h:18:10: fatal error: 'boost/serialization/base_object.hpp' file not found

[dberzano]

Hmm. Isn't it just possible to mask out the boost headers to Cling with some #ifdef? I seem to recall a trick like that for headers not required to build the dictionaries (and I certainly hope we don't require boost in any of our ROOT dictionary, otherwise we have a problem).

[ktf]

Yeah, this is a slippery slope, that's why I was suggesting to refactor the code rather than have everything in the ROOT dictionaries. This will also make the memory usage explode until the C++ modules are available in ROOT.

[shahor02]

@dberzano As far as I understand, the #ifdef guards can mask the unwanted headers in the macro only in the interpreted mode, in the ACLIC compilation both both CLING and ROOTCLING are defined like in the standard compilation.

@ktf Fine as a principle, but realistically I don't see what I can do. In 3 days I had 2 unrelated occasions of O2 headers depending on the low-level framework classes. If we don't allow such dependencies, it should be imposed as a policy and probably added to CI test (e.g. no #include <boost" in the common headers).

Also note that in the example above the boost headers are successfully loaded provided the system boost is used.

@davidrohr In this particular case, the #include <boost/serialization/base_object.hpp> is added to Cluster.h just for the sake of the friend class boost::serialization::access; declaration. Do you see alternative solution, e.g. providing the access via setters/getters?

[dberzano]

What I am saying is that ROOT_INCLUDE_PATH should be needed at runtime only for dictionaries, and we should not need boost in any dictionary, in principle. What we need to do is masking out the code when invoking rootcling.

#if !defined(__CLING__) && !defined(__ROOTCLING__)
#include <relevant_boost_headers.h>
#endif

[ktf]

this only works if the header is used only by the implementation, not the API..

[dberzano]

@ktf Yes exactly. I hope this is the case (I haven't looked at the code actually), in most cases it is.

[davidrohr]

for boost i kind of disagree. this is used at many places,so it would be a good idea to make it availabe to root generally similarly to when the system installation of boost is used. Kind Regards David Rohr (Sent from my mobile, excuse the typos.)

…

On 27 September 2018 12:05:20 CEST, Dario Berzano ***@***.***> wrote: What I am saying is that `ROOT_INCLUDE_PATH` should be needed at runtime only for dictionaries, and we should not need boost in any dictionary, in principle. What we need to do is masking out the code when invoking `rootcling`. ```cxx #if !defined(__CLING__) && !defined(__ROOTCLING__) #include <relevant_boost_headers.h> #endif ``` -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1305 (comment)

[dberzano]

@davidrohr I think we should:

1. try to see if in this case it's really needed, or not; if not, let's guard the relevant headers as suggested and discard this PR; we make it available if strictly required, not just because we can 🙂
2. try to make our dictionary generation simple; we should really not depend on boost stuff on the API, if possible

In this particular case, isn't it possible to mask both the #include and the friend definition? Would it work like that? (I am pretty sure I am overlooking something, it's just a thought.)

[shahor02]

@dberzano : "...What we need to do is masking out the code when invoking rootcling."
The problem comes via class headers included to the macro being compiled. Both the
__CLING__ and __ROOTCLING__ are defined in the compilation with make and ACLIC, I don't see any way to mask the unwanted header in the compiled macro.
In the JIT mode the __ROOTCLING__ is not defined, there the ``#ifdef``` guards can work.

@davidrohr : in the particular case of the DataFormats/TPCCluster.h, the #include <boost/serialization/base_object.hpp> is actually not needed: you have

namespace boost
{
namespace serialization
{
class access;
}
} // namespace boost

declaration in the same header, which is enough to declare the friend class (and Cluster.cxx does not use boost either). I've compiled both O2 and dependent macro w/o this include. Do you want me commit the Cluster.h w/o include, until we decide common recipe for such situations?

[davidrohr]

@shahor02 : I am not the author of this class, and I didn't add boost.
No idea what is the purpose of it.

@dberzano : well, it is a bit weird, boost is a common library used in many places, and also available to root if we use the system boost. So I do not see a reason why we shouldn't make the boost headers available to root in case we use the alidist boost. This is a bit like if we would not allow any std::... includes in headers parsed by ROOT. In general, I agree we should keep the dictionary generation simple, but we also shouldn't make our life unnecessarily hard.

[dberzano]

OK so it seems it's not needed here.

@shahor02 let me further clarify. The header must not be masked in the compiled macro, this is not what I said. I suggested to mask it while generating the class dictionary. This happens at compile time by invoking rootcling and it's unrelated to compiling macros.

@davidrohr I strongly stand on both my points: if we can make it work without adding more (and more and more) stuff to ROOT_INCLUDE_PATH, let's not merge this. We, together, have seen what might be the consequences of adding alien headers to rootcling-parsed stuff (HOMER, remember?). And, secondly, if not needed (and it's not, it seems), I don't see why we should fix something that's not broken. I know you speak as a developer (sorry to make your life "unnecessarily harder"), but I speak as a librarian and maintainer, and I'd like the code maintenance to be future proof. I think the HOMER example is still a good one. rootcling is not an industry-standard product, it's something quite specific to our field, and we don't know how well/bad it will behave in the future with headers we don't control. I see your point but please try to see mine as well 🙂

[ktf]

IMHO if it's hard to disentangle boost / fairmq / stl from dictionary creation it means we are doing something wrong with what we are saving as ROOT objects. Boost / STL are great when it comes to simplify implementations, but if we have anything but std::vector in our data classes, most likely we are looking for troubles down the road.

[dberzano]

Totally agree with @ktf.

[shahor02]

@davidrohr ok, sorry, missed the authorship, it comes from P.Christiansen. Incidentally, originally it was guarded by _CINT_ (which was suppressed by @ktf :).
I will commit a version w/o include.

@dberzano: fine, in the dictionary creation the header can be guarded, but my problem was not the dictionary: it was macro compilation from the root, were we don't have cmake-managed include paths.

[shahor02]

PR AliceO2Group/AliceO2#1364 solves this particular problem.

[shahor02]

Hi,
Now I've got a similar problem via Headers/DataHeader.h, again, it has nothing to do with dictionaries:

Info in <TUnixSystem::ACLiC>: creating shared library /home/shahoian/alice/O2/macro/./run_rec_ca_C.so
In file included from input_line_12:9:
In file included from ././run_rec_ca.C:15:
In file included from /home/shahoian/alice/sw/ubuntu1604_x86-64/O2/dev-1/include/DetectorsCommonDataFormats/DetID.h:37:
In file included from /home/shahoian/alice/sw/ubuntu1604_x86-64/O2/dev-1/include/Headers/DataHeader.h:40:
/home/shahoian/alice/sw/ubuntu1604_x86-64/O2/dev-1/include/MemoryResources/MemoryResources.h:31:10: fatal error: 'boost/container/pmr/memory_resource.hpp' file not found
#include <boost/container/pmr/memory_resource.hpp>

I can bypass this problem with some dirty tricks, but I find principally wrong than I cannot use e.g.
#include Headers/DataHeader.h in my class headers.

[davidrohr]

again, In my opinion boost is quite basic and we should make it available to ROOT by default, in particular to avoid difference between system-boost and alidist-boost setups, and because these issues pop up in macros so they are not caught by the CI.

[shahor02]

@davidrohr agree, and related to this (AliceO2Group/AliceO2#1351): I don't see why we need to test macros in the interpreted mode, in this way we check only the dictionaries. I think the macros should be always in compiled mode.