search

JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

Bugfix: CMake configuration #264

Closed nathanwbrei closed 10 months ago

nathanwbrei commented 10 months ago
wdconinc commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

faustus123 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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