search

JeffersonLab / JANA2

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

HAVE_PODIO not added to JANA2 targets as COMPILE_DEFINITIONS #242

Closed wdconinc closed 9 months ago

wdconinc commented 11 months ago

When compiling JANA2 with PODIO support, an add_compile_definitions for HAVE_PODIO is applied project-wide, but this is not added to the JANAConfig.cmake configuration that is used by downstream projects, e.g. EICrecon. Those downstream projects must independently add HAVE_PODIO to be able to use JANA2 headers. Downstream projects can set HAVE_PODIO when JANA2 is compiled without PODIO support, resulting in undefined references.

wdconinc commented 11 months ago

@nathanwbrei A relevant comment at https://github.com/eic/EICrecon/blob/3c1d1c875a7698071079ed6db86f89e052ec5d55/CMakeLists.txt#L109-L111

nathanwbrei commented 11 months ago

Thanks for the reminder. This is a good thing to get into the next release.

nathanwbrei commented 11 months ago

EICrecon uses HAVE_PODIO quite extensively. We should deprecate the old name before renaming it outright.

wdconinc commented 11 months ago

The problem I see is that JANA2 doesn't really have a versioning scheme that is conducive to deprecating things, not as long as we stick to minor version updates only or putting new features in bugfix releases (or not release the bugfixes at all). We essentially have to treat every new version, including bugfix releases, as equivalent to a major release that potentially breaks API compatibility.

nathanwbrei commented 11 months ago

@wdconinc Are you suggesting we skip deprecating HAVE_PODIO and just go ahead and rename it right away, since it will be the same amount of work for you? Everything else in this upcoming release is a bugfix. We have actual API changes in the pipeline but we've been deferring those.

wdconinc commented 11 months ago

I am thinking that it can be a change that is applied as a regular release: HAVE_PODIO -> JANA2_USE_PODIO, and it gets announced in the release notes as a breaking change. I don't think having a phase where HAVE_PODIO emits a deprecation warning is required.