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

Fix CMake configuration to make Physsim usable as a library #2

Merged
merged 7 commits into from
Jan 12, 2023

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Jan 10, 2023

BEGINRELEASENOTES

  • Update CMake configuration to use the modern target based approach of propagating dependencies and making Physsim a package that can be easily found via find_package(Physsim)
  • Remove duplicated headers in src and use the ROOT CMake macro to generate dictionaries and build the library.
  • Make examples use this functionality and in this way make it build again via the canonical iLCSoft cmake build steps.
    • Add more detailed instructions to the README
  • Add github actions based CI and remove the travis-ci configuration.

ENDRELEASENOTES

Define targets properly and export and install them such, that they are
easily usable with find_package
Copy link

@tjunping tjunping left a comment

Choose a reason for hiding this comment

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

looks fine to me

@JennyListDESY
Copy link

When building against ilcsoft v02-02-03,
an additional
INCLUDE_DIRECTORIES( ./include/physsim )
is needed in CMakeLists.txt

- Remove duplicated headers
- Fix include paths
- Merge LinkDef.h files into one and remove individual ones
- Explicitly state library source files
- Use CMake macro provided by root
@tmadlener
Copy link
Contributor Author

After a bit of fiddling and some more cleanup, this now works with key4hep and iLCSoft releases (at least with recent ones), and the CI workflows now also build the example processors. In the course of the cleanup I have removed the duplicated headers in the src/* directories and made sure that all the necessary include paths are set correctly. It should now be rather straight forward to move the example processors into a separate repository, but I would leave that for another pull request.

@tmadlener
Copy link
Contributor Author

Since all the testing has been done successfully, I think we can merge this and fix any upcoming issues in a separate pull request to not make this too cluttered.

@tmadlener tmadlener merged commit 2968806 into iLCSoft:master Jan 12, 2023
@tmadlener tmadlener deleted the cmake-update branch January 12, 2023 13:08
@tmadlener tmadlener restored the cmake-update branch January 12, 2023 13:08
cmake_policy(SET CMP0008 NEW)
#--- Define basic build settings -----------------------------------------------
# Provides install directory variables as defined for GNU software
include(GNUInstallDirs)
Copy link

@andresailer andresailer Jan 13, 2023

Choose a reason for hiding this comment

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

This makes the libraries now go to lib64, which breaks the check in ilcsoft-install
(And the assignment of the library to MARLIN_DLL, which happens because of how physsim is done in ilcsoft install https://github.com/iLCSoft/iLCInstall/blob/b32418db68960110ade0270a7ce37fbe32241983/releases/HEAD/release-ilcsoft.cfg#L167-L168 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true. I have made a PR to iLCInstall to accomodate for the lib[64] changes. MARLIN_DLL should not really matter for Physsim, because it does not build processors. Those need to be explicitly built from the examples directory.

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