Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For Wrapping processors inside Gaudi #36

Merged
merged 3 commits into from
Sep 4, 2019
Merged

Conversation

andresailer
Copy link
Contributor

@andresailer andresailer commented Aug 22, 2019

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

  • Marlin::Processor: make setParameters and setName public functions
  • Marlin::EventSelector: moved to marlin namespace

ENDRELEASENOTES

Copy link
Contributor

@rete rete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to move the class in the marlin namespace ?

@andresailer
Copy link
Contributor Author

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

@andresailer
Copy link
Contributor Author

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
Copy link
Contributor

rete commented Sep 4, 2019

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

@gaede
Copy link
Contributor

gaede commented Sep 4, 2019

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

@rete
Copy link
Contributor

rete commented Sep 4, 2019

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
Copy link
Contributor

petricm commented Sep 4, 2019

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
Copy link
Contributor Author

Indented

We need to export the CMAKE_CXX_STANDARD in the ILCSoft.cmake

@rete
Copy link
Contributor

rete commented Sep 4, 2019

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
Copy link
Contributor

gaede commented Sep 4, 2019

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
Copy link
Contributor Author

See iLCSoft/iLCInstall#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
Copy link
Contributor

gaede commented Sep 4, 2019

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.

Copy link
Contributor

@gaede gaede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI fails due to wrong cxx standard - to be fixed in other PR...

@gaede gaede merged commit 3aa0141 into iLCSoft:master Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants