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

Use FILE_SET to install the headers in /include together with the edm4hep library #235

Merged
merged 16 commits into from
Dec 5, 2023

Conversation

jmcarcell
Copy link
Contributor

@jmcarcell jmcarcell commented Nov 5, 2023

BEGINRELEASENOTES

  • Use FILE_SET to install the headers in the top folder together with the library. This also adds them in the BUILD_INTERFACE, something that a simple INSTALL doesn't do.
  • Bump the CMake version of the LCG stacks
  • Simplify finding ROOT, don't do environment variable manipulation in CMake

ENDRELEASENOTES

FILE_SET requires CMake 2.23 so the version of the LCG stacks has to be bumped. I discovered that it also has the nice property that it complains if any of the sources that are attached to the library are not installed.

edm4hep/CMakeLists.txt Outdated Show resolved Hide resolved
@jmcarcell jmcarcell changed the title Add a BUILD_INTERFACE and INSTALL_INTERFACE for edm4hep Use FILE_SET to install the headers in /include together with the edm4hep library Nov 15, 2023
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

How important is this for the new Key4hep release?

It seems the only thing that is missing are a few format complaints from pre-commit / clang-format. Which either haven't been caught by previous versions of clang-format, or the file wasn't checked. Anyhow, this should be fairly straight forward to fix.

.github/workflows/pre-commit.yml Show resolved Hide resolved
@jmcarcell
Copy link
Contributor Author

jmcarcell commented Nov 20, 2023

How important is this for the new Key4hep release?

It seems the only thing that is missing are a few format complaints from pre-commit / clang-format. Which either haven't been caught by previous versions of clang-format, or the file wasn't checked. Anyhow, this should be fairly straight forward to fix.

For the release it doesn't matter. About the complaints, we can ignore them since I get different ones with the new .clang-format so I'll make them when I push the file (probably after the release not to create conflicts in the last PRs). So I think on my side this is finished.

@jmcarcell
Copy link
Contributor Author

Right now the pre-commit CI works fine but there are changes, probably due to a change in the version of clang-format. Since the .clang-format is going to change anyway, I'm going to merge this as it is and then make the formatting changes in a different commit

@jmcarcell jmcarcell merged commit 073cd9e into main Dec 5, 2023
12 of 15 checks passed
@jmcarcell jmcarcell deleted the cmake branch December 5, 2023 15:06
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