iLCSoft / Marlin

Modular Analysis and Reconstruction for the LINear Collider
GNU General Public License v3.0
11 stars 16 forks source link

For Wrapping processors inside Gaudi #36

Closed andresailer closed 5 years ago

andresailer commented 5 years ago

These changes are needed to configure and run Marlin processors from a Gaudi Algorithm or avoid a clash in the naming of the EventSelector class

BEGINRELEASENOTES

ENDRELEASENOTES

andresailer commented 5 years ago

I'll try it with "namespacing" the processor.

andresailer commented 5 years ago

Placing the processor in the marlin namespace also works. Because of the "using namespace marlin" in the cc file, I didn't even have to change it. I also didn't indent the code in the header file, unless it is wished for?

rete commented 5 years ago

> I also didn't indent the code in the header file, unless it is wished for? No, it's fine.

gaede commented 5 years ago

I actually would prefer proper indentation.... Why do the llvm builds fail ?

rete commented 5 years ago

It looks like we have now an issue in TravisCI.

https://travis-ci.org/iLCSoft/Marlin/jobs/580699280#L845

It looks like the C++11 flag is not present

petricm commented 5 years ago

This is not a CI problem per-se, the passing of c++14 to all packages in the compilation of iLCSoft has been fixed, but apparently not if you are building a package against a central release.

andresailer commented 5 years ago

Indented

We need to export the CMAKE_CXX_STANDARD in the ILCSoft.cmake

rete commented 5 years ago

We need to be careful by exporting this variable in a common script. Some of the packages will maybe not compile anymore with a higher cxx standard like c++17 which removes a lot of deprecated functions. As a first investigation, the fastjet package won't compile with c++17 and my first guess is that packages compiling against fastjet (MarlinFastJet) won't compile with using c++17.

gaede commented 5 years ago

We need to export the CMAKE_CXX_STANDARD in the ILCSoft.cmake Ok. Is there also a way to explicitly require CMAKE_CXX_STANDARD>="14" in Marlin (or any other package) ? @remi I have already managed to build everything w/ c++17 except the fastjet stuff - but there the recent version seems to also work w/ c++17.

andresailer commented 5 years ago

See https://github.com/iLCSoft/iLCInstall/pull/70 for exporting CMAKE_CXX_STANDARD

Is there also a way to explicitly require CMAKE_CXX_STANDARD>="14" in Marlin (or any other package) ?

Do you mean (a) when linking against a package, or (b) when compiling the package itself, or both?

b) https://github.com/AIDASoft/DD4hep/blob/5e01aa95d4377a697cef29cecb0d88631dbeca1a/CMakeLists.txt#L57-L59

IF(${CMAKE_CXX_STANDARD} LESS 14)
  MESSAGE(FATAL_ERROR "DD4hep requires at least CXX Standard 14 to compile")
ENDIF()

a)

https://github.com/AIDASoft/DD4hep/blob/5e01aa95d4377a697cef29cecb0d88631dbeca1a/DDCore/CMakeLists.txt#L83-L88

if(${CMAKE_VERSION} VERSION_GREATER 3.7.99)
  target_compile_features(DDCore
    PUBLIC
    cxx_std_${CMAKE_CXX_STANDARD} # Needs cmake 3.8
    )
endif()

Export the targets as libraries everywhere and set the c++ standard it was built with. Cmake will ensure that at least this standard is used to compile things later on, when one links against the library.

gaede commented 5 years ago

I want to set the minimal cxx standard that's needed for building the package, as just discussed w/ @andresailer:

# Set C++ standard
set(CMAKE_CXX_STANDARD 14 CACHE STRING "minimum C++ standard needed for compiling")
IF(${CMAKE_CXX_STANDARD} LESS 14)
  MESSAGE(FATAL_ERROR "DD4hep requires at least CXX Standard 14 to compile")
ENDIF()

will create a corresponding PR.