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

build: use GNUInstallDirs and improve packaging #31

Merged
merged 4 commits into from
Mar 19, 2023
Merged

Conversation

c-dilks
Copy link
Member

@c-dilks c-dilks commented Mar 16, 2023

Briefly, what does this PR introduce?

  • Use GNUInstallDirs to set the installation tree, instead of doing it "by hand"
  • Install cmake files in lib/cmake/IRT instead of in lib/IRT
  • Add public-facing variables IRT_LIBRARY_DIR and IRT_INCLUDE_DIR
  • This requires updates in EICRecon: build: use IRTConfig.cmake vars EICrecon#546

Installation tree before this PR:

CMAKE_INSTALL_PREFIX
├── include
│   └── IRT
│       ├── BitMask.h
│       ├── ChargedParticle.h
│       ├── ChargedParticleStep.h
│       ├── CherenkovDetectorCollection.h
│       ├── CherenkovDetector.h
│       ├── CherenkovEvent.h
│       ├── CherenkovMirror.h
│       ├── CherenkovPhotonDetector.h
│       ├── CherenkovPID.h
│       ├── CherenkovRadiator.h
│       ├── G4Object.h
│       ├── IRT.h
│       ├── IRTSolution.h
│       ├── OpticalBoundary.h
│       ├── OpticalPhoton.h
│       ├── ParametricSurface.h
│       ├── RadiatorHistory.h
│       ├── ReflectionPoint.h
│       ├── RefractionPoint.h
│       ├── SinglePDF.h
│       └── TransientParticle.h
└── lib
    ├── IRT
    │   ├── IRTConfig.cmake
    │   ├── IRTConfigVersion.cmake
    │   ├── IRTTargets.cmake
    │   └── IRTTargets-noconfig.cmake
    ├── libDELPHES_rdict.pcm
    ├── libDELPHES.rootmap
    ├── libDELPHES.so
    ├── libIRT_rdict.pcm
    ├── libIRT.rootmap
    └── libIRT.so

After this PR:

CMAKE_INSTALL_PREFIX
├── include
│   └── IRT
│       ├── BitMask.h
│       ├── ChargedParticle.h
│       ├── ChargedParticleStep.h
│       ├── CherenkovDetectorCollection.h
│       ├── CherenkovDetector.h
│       ├── CherenkovEvent.h
│       ├── CherenkovMirror.h
│       ├── CherenkovPhotonDetector.h
│       ├── CherenkovPID.h
│       ├── CherenkovRadiator.h
│       ├── G4Object.h
│       ├── IRT.h
│       ├── IRTSolution.h
│       ├── OpticalBoundary.h
│       ├── OpticalPhoton.h
│       ├── ParametricSurface.h
│       ├── RadiatorHistory.h
│       ├── ReflectionPoint.h
│       ├── RefractionPoint.h
│       ├── SinglePDF.h
│       └── TransientParticle.h
└── lib
    ├── cmake
    │   └── IRT
    │       ├── IRTConfig.cmake
    │       ├── IRTConfigVersion.cmake
    │       ├── IRTTargets.cmake
    │       └── IRTTargets-noconfig.cmake
    ├── libDELPHES_rdict.pcm
    ├── libDELPHES.rootmap
    ├── libDELPHES.so
    ├── libIRT_rdict.pcm
    ├── libIRT.rootmap
    └── libIRT.so

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: refactor the build config

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Yes, see above PR on EICrecon. This also might break Juggler's usage (on a dev branch, not main).
This also might impact @alexander-kiselev's usage

Does this PR change default behavior?

No, but hopefully it solves our various build/linking/cling/etc. problems in EICrecon...

@c-dilks
Copy link
Member Author

c-dilks commented Mar 16, 2023

@veprbl and @wdconinc, I'm still testing this, but could you take a look to see if what I'm doing makes any sense? I am still fairly inexperienced with cmake configuration and would greatly appreciate any advice.

@c-dilks c-dilks linked an issue Mar 16, 2023 that may be closed by this pull request
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/IRTConfig.cmake.in Outdated Show resolved Hide resolved
delphes/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
@c-dilks
Copy link
Member Author

c-dilks commented Mar 17, 2023

Tested IRT PID in EICrecon, still works fine.

@c-dilks c-dilks marked this pull request as ready for review March 17, 2023 22:59
@c-dilks c-dilks requested a review from veprbl March 17, 2023 22:59
CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM

@c-dilks c-dilks merged commit 12f2c19 into main Mar 19, 2023
@c-dilks c-dilks deleted the debug-cmake branch March 19, 2023 00:19
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.

Add IRT_INCLUDE_DIR etc. to cmake/IRTConfig.cmake.in
2 participants