Bugfix: CMake configuration

[nathanwbrei]

• Correctly specifies podio, podioRootIO as link library dependencies of JANA2 target
• Correctly propagates preprocessor (e.g. HAVE_PODIO) defines as compile options of JANA2 target
• Prefixes all preprocessor defines with JANA2 to avoid conflicts downstream

[wdconinc]

It's not necessarily the most obvious that it's find_package(JANA) with targets in the namespace JANA but that other things are prefixed with JANA2...

[nathanwbrei]

It's not necessarily the most obvious that it's find_package(JANA) with targets in the namespace JANA but that other things are prefixed with JANA2...

That's because (in my opinion) it really should be find_package(JANA2), I just don't want to deal with all the breakages right now. At any rate, prefixing the target_compile_definitions makes it clear that they are meant to be internal to JANA2 itself, and not something that upstream users should be looking at. (Unless they are, for instance, performing surgery on JMultifactory)

I'm also very tempted to remove the target_compile_definitions from exported target entirely, and bake them into the generated jana-config.h header file instead. This would do a much more solid job enforcing that the preprocessor defines used to build the library are the same as the ones used downstream in the header files, and they would become more internal to JANA. The main reason I haven't already is because the existing jana-config.h is a bit of a mess, and cleaning up that mess involves negotiating with halld_recon dependencies that I can't easily test.

[nathanwbrei]

@faustus123 @DraTeots @wdconinc @veprbl How would everyone feel if I moved target_compile_definitions such as JANA2_HAVE_PODIO to jana-config.h?

[DraTeots]

Not the objection but JANA2_HAVE_ROOT and <JANA/JEventProcessor.h>. Could it be possible to make a code style decision e.g. to use JANA in code (code should not carry the knowledge of the major version number) but e.g. JANA2 in... repository name? JANA2/JANA mixed around confuses and makes one remember where one should use each

[nathanwbrei]

The "2" in "JANA2" needs to be part of the name (as opposed to the major version number) all the time, so that we can make breaking changes without turning it into "JANA3". It was an unambiguous mistake to organize our code as "JANA/JEventProcessor.h" to begin with, when we have identically named JANA1 headers. (For one thing, it prevents us from having system installs of JANA1 and JANA2 side-by-side). If I can't increment the major version number, I can't fix our broken naming conventions, hence I can't increment the major version number.

[faustus123]

@faustus123 @DraTeots @wdconinc @veprbl How would everyone feel if I moved target_compile_definitions such as JANA2_HAVE_PODIO to jana-config.h?

I agree. This sounds like the better solution to have it in the header. In principle, it allows non-cmake build systems to use the build. (Or at least have an easier time of it.)

[wdconinc]

How would everyone feel if I moved target_compile_definitions such as JANA2_HAVE_PODIO to jana-config.h?

I don't really like the auto-generated header approach. It messes with code analysis tools, often causes more recompiles than needed, etc. Unless JANA2 gets to the scale of ROOT where you need two pages of defines in RConfigure.h to keep track of this, I'd just keep using the compile definitions and attach them to targets as properties. If you do go for the auto-generated header, it would be useful to allow for handling these options in full C++20 model with constexpr and consteval support instead of forcing this through preprocessor defines.

[wdconinc]

Some consistency in jana_config.h would be desired anyway, if you do decide to overhaul that.

#define HAVE_ROOT     1
#define HAVE_XERCES   
#define XERCES3       
#define HAVE_CCDB     0
#define HAVE_NUMA     0

We are supposed to use #ifdef HAVE_XERCES but #if HAVE_CCDB == 1?

[nathanwbrei]

Some consistency in jana_config.h would be desired anyway, if you do decide to overhaul that.

#define HAVE_ROOT     1
#define HAVE_XERCES   
#define XERCES3       
#define HAVE_CCDB     0
#define HAVE_NUMA     0

We are supposed to use #ifdef HAVE_XERCES but #if HAVE_CCDB == 1?

This is exactly the mess I was referring to :)
Even if we decide we really really want the header file, I'd rather get this release out before Thanksgiving