this package uses openmp, clang needs -fopenmp to link this

[pinkenburg]

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work for users)
• Requiring change in macros repository (Please provide links to the macros pull request in the last section)
• I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

clang needs an explicit -fopenmp if linking libraries which use OpenMP. jenlins does not pass clang compilation errors on, so this problem managed to get past us.

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

Summary

Motivation & Context

This PR addresses a linking issue when building the TPC dEdx calibration package with the clang compiler. The package uses OpenMP, which requires explicit -fopenmp linking support. While gcc handles this implicitly, clang requires the flag to be explicitly specified during compilation and linking. The issue was previously masked in Jenkins builds.

Key Changes

• configure.ac:

  • Added -fopenmp flag to CXXFLAGS to enable OpenMP support
  • Enhanced compiler warning flags from basic (-Wall -Werror) to comprehensive suite (-Wall -Wshadow -Wextra -Werror)
  • Updated comments to document OpenMP requirement for clang

• Makefile.am:

  • Converted OFFLINE_MAIN include paths to use -isystem (suppresses warnings from system headers)
  • Maintains all library linking flags and dependencies

Potential Risk Areas

• Compiler condition limitation: The -fopenmp flag is applied only when ac_cv_prog_gxx = yes (detecting g++ specifically). If clang is being used as the C++ compiler, this condition may not trigger properly, potentially leaving the clang build without the required -fopenmp flag. This should be verified on actual clang builds.
• Include path behavior: Use of -isystem for OFFLINE_MAIN headers suppresses compiler warnings from these dependencies, which could mask legitimate issues in included headers
• OpenMP thread-safety: Packages previously compiled without proper OpenMP flags may now exhibit different runtime behavior when using multiple threads
• Warning strictness: The addition of -Wshadow and -Wextra may cause compilation failures if the codebase has existing shadowing or other warnings not previously enforced

Possible Future Improvements

• Implement compiler detection that explicitly handles both gcc and clang (consider using Autoconf's AX_COMPILER_VENDOR or similar macro)
• Consider migrating to CMake with FindOpenMP module for more robust compiler support
• Add conditional flags for clang-specific optimizations if performance differs from gcc
• Document OpenMP threading model and any required runtime parameters for users of this library

---

Note: AI analysis may contain errors; the implementation of the compiler condition should be carefully verified to ensure clang builds actually receive the -fopenmp flag as intended.

[coderabbitai]

📝 Walkthrough

Walkthrough

Build configuration updates for TPC dEdx calibrations module: include path changed from standard (-I) to system include (-isystem) flag; compiler flags expanded to add OpenMP support and supplementary warnings (-Wshadow, -Wextra).

Changes

Cohort / File(s)	Change Summary
Build Configuration calibrations/tpc/dEdx/Makefile.am	Include path flag updated from -I$(OFFLINE_MAIN)/include to -isystem$(OFFLINE_MAIN)/include in AM_CPPFLAGS
Build Configuration calibrations/tpc/dEdx/configure.ac	CXXFLAGS expanded: -fopenmp added for OpenMP support; additional warnings -Wshadow and -Wextra introduced; existing -Wall and -Werror preserved

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[pinkenburg]

jenkins is acting up - merging this PR to get the clang build back to work

[hupereir]

@pinkenburg I looked at the code, and I don't think it actually uses any open MP functionality
My suspicion is that the dependence on openMP is artificial, due to linking against libtrack_reco (which does use openMP).
Now, investigating further, linking against track reco should be avoided as much as possible, and in fact is not necessary here. (the code relies on some functionality in libtrackbase_historic, but nothing in trackreco directly.
Should I try to clean this up in a separate pull request ?

[pinkenburg]

@hupereir sure - please go ahead. It is annoying that whatever links against libtrack_reco needs to be compiled with this flag under clang (libtrack_reco really should build this dependency into the library). The best solution is to remove a non existing dependency on libtrack_reco (I didn't check for this)