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.

Sign up for GitHub

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

Jump to bottom

CMake: Install Blosc2Config.cmake #537

Merged
merged 2 commits into from
Jul 28, 2023
Merged

CMake: Install Blosc2Config.cmake #537

merged 2 commits into from
Jul 28, 2023

Conversation

ax3l
Copy link
Contributor

@ax3l ax3l commented Jul 25, 2023 •
edited
Loading

This adds a CMake installer to conserve targets and properties on install, so CMake users do not need to write FindBlosc2.cmake files anymore.

export CMAKE_PREFIX_PATH=$HOME/sw/c-blosc2-installed-here
# ...
find_package(Blosc2 2.4 CONFIG)

This also helps to preserve transitive dependencies on CMake targets, especially useful for fully static builds, e.g., for Python wheels.

The installed config package is designed to work both on Unix (Linux/macOS) and Windows.

@ax3l ax3l mentioned this pull request Jul 25, 2023
Closed
@ax3l ax3l force-pushed the topic-cmake-install-targets branch 3 times, most recently from a8fb220 to 80cc257 Compare July 25, 2023 23:12
@ax3l ax3l changed the title Topic cmake install targets [WIP] CMake: Install blosc2Config.cmake Jul 25, 2023
@ax3l ax3l force-pushed the topic-cmake-install-targets branch 3 times, most recently from 211c0da to 0ea519c Compare July 26, 2023 00:07
@ax3l ax3l mentioned this pull request Jul 26, 2023
Closed
@ax3l ax3l changed the title [WIP] CMake: Install blosc2Config.cmake [WIP] CMake: Install Blosc2Config.cmake Jul 26, 2023
@ax3l ax3l force-pushed the topic-cmake-install-targets branch from 0ea519c to 04c4b6b Compare July 26, 2023 00:22
@ax3l ax3l changed the title [WIP] CMake: Install Blosc2Config.cmake CMake: Install Blosc2Config.cmake Jul 26, 2023
@ax3l ax3l mentioned this pull request Jul 26, 2023
Merged
2 tasks
@ax3l ax3l force-pushed the topic-cmake-install-targets branch 6 times, most recently from af5f733 to bbea358 Compare July 26, 2023 06:42
@ax3l
CMake: Install Blosc2Config.cmake
a67dfca 
This adds a CMake installer to conserve targets and properties
on install, so CMake users do not need to write `FindBlosc2.cmake`
files anymore.

This also helps to preserve transitive dependencies on CMake
targets, especially useful for fully static builds, e.g., for
Python wheels.
@ax3l ax3l force-pushed the topic-cmake-install-targets branch from bbea358 to a67dfca Compare July 26, 2023 06:43
@ax3l
Copy link
Contributor Author

ax3l commented Jul 26, 2023

For @FrancescAlted - please let me know if you prefer more inline comments or need more details.

I am also happy to do a few general updates/modernizations to CMake as a follow-up PR. The PR here tries to focus on only changing the things necessary for now, enabling all-static builds and propagating CMake targets and their dependencies to downstream developers cleanly.

FrancescAlted
FrancescAlted requested changes Jul 26, 2023
View reviewed changes
Copy link
Member

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

That looks great to me. However, as some changes are quite large, I would appreciate some more comments in the places that things have changed most. Thanks in advance!

@FrancescAlted
Copy link
Member

For @FrancescAlted - please let me know if you prefer more inline comments or need more details.

Yes, I have suggested that already :-)

I am also happy to do a few general updates/modernizations to CMake as a follow-up PR. The PR here tries to focus on only changing the things necessary for now, enabling all-static builds and propagating CMake targets and their dependencies to downstream developers cleanly.

Sounds good to me. Let's first merge this one and proceed gradually. Thanks!

@ax3l
Copy link
Contributor Author

ax3l commented Jul 26, 2023 •
edited
Loading

as some changes are quite large, I would appreciate some more comments in the places that things have changed most. Thanks in advance!

Thank you @FrancescAlted! I added more inline comments and added some cleaning in the last commit.
Please feel free to add inline notes for lines where you like to see further comments :)

@ax3l ax3l requested a review from FrancescAlted July 26, 2023 22:18
@ax3l
Copy link
Contributor Author

ax3l commented Jul 28, 2023

Attn @FrancescAlted :)

FrancescAlted
FrancescAlted approved these changes Jul 28, 2023
View reviewed changes
Copy link
Member

@FrancescAlted FrancescAlted left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@FrancescAlted FrancescAlted merged commit d510951 into Blosc:main Jul 28, 2023
16 checks passed
@ax3l ax3l deleted the topic-cmake-install-targets branch July 28, 2023 18:41
@ax3l
Copy link
Contributor Author

ax3l commented Jul 28, 2023

My pleasure! :)
Thank you for merging!

@FrancescAlted FrancescAlted mentioned this pull request Aug 1, 2023
Closed
3 tasks
This was referenced Aug 18, 2023
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrancescAlted FrancescAlted FrancescAlted approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ax3l @FrancescAlted