Skip to content

chore: Bump cmake 3.25 #458

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
Merged
merged 2 commits into from
Aug 12, 2024
Merged

chore: Bump cmake 3.25 #458

merged 2 commits into from
Aug 12, 2024

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Mar 27, 2024 •
edited
Loading

Ubuntu 24.04 is out with CMake 3.28
RHEL9 bumped the CMake 3.26

@LecrisUT LecrisUT added this to the 2.5 milestone Mar 27, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.12%. Comparing base (adf8bc1) to head (ec314da).
Report is 16 commits behind head on develop.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #458   +/-   ##
========================================
  Coverage    84.12%   84.12%           
========================================
  Files           26       26           
  Lines         8122     8122           
  Branches      1702     1702           
========================================
  Hits          6833     6833           
  Misses        1289     1289
Flag Coverage Δ
c_api 75.39% <ø> (ø)
fortran_api 56.29% <ø> (ø)
python_api 81.39% <ø> (ø)
unit_tests 13.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT modified the milestones: 2.5, 2.6 Jul 2, 2024
@LecrisUT LecrisUT force-pushed the chore/cmake-3.25 branch from 5fdbe4c to 15721c0 Compare July 2, 2024 16:56
@LecrisUT LecrisUT self-assigned this Jul 2, 2024
@LecrisUT LecrisUT force-pushed the chore/cmake-3.25 branch from 15721c0 to 73be1fb Compare July 2, 2024 17:19
@LecrisUT LecrisUT mentioned this pull request Jul 8, 2024
Merged
@LecrisUT LecrisUT force-pushed the chore/cmake-3.25 branch from 73be1fb to 64a0578 Compare July 8, 2024 12:19
@LecrisUT LecrisUT mentioned this pull request Jul 9, 2024
Closed
@LecrisUT LecrisUT force-pushed the chore/cmake-3.25 branch 2 times, most recently from 7cd9e3b to 74bb352 Compare August 4, 2024 10:23
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Aug 4, 2024 •
edited
Loading

Well, this one is ready to be merged, since the current LTS distros have CMake >= 3.25. Waited a grace period after 2.5 was released to catch any bug fixes and so far there is only #519 and #517. After those 2 are merged, this can be merged, and if we catch more bugs that need to be backported, we can just cherry-pick those later.

@LecrisUT LecrisUT requested a review from lan496 August 4, 2024 10:31
@LecrisUT LecrisUT assigned lan496 and unassigned LecrisUT Aug 4, 2024
@LecrisUT LecrisUT requested a review from atztogo August 4, 2024 10:31
@LecrisUT LecrisUT mentioned this pull request Aug 4, 2024
Merged
@lan496
Copy link
Member

lan496 commented Aug 5, 2024

Ping me after these two PRs are merged just to be sure.

LecrisUT added 2 commits August 9, 2024 14:26
@LecrisUT
Bump CMake minimum required to 3.25
e9074dd 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Remove FetchContent Spglib in sub-projects
ec314da 
In retrospect it is not desired.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
lan496
lan496 approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

LGTM

@atztogo Do you have any concerns about bumping up CMake's version?

@atztogo
Copy link
Collaborator

atztogo commented Aug 10, 2024

@atztogo Do you have any concerns about bumping up CMake's version?

Honestly, I have no idea and no objection. Please go ahead.

@LecrisUT
Copy link
Collaborator Author

pip install wise it is completely transparent, because skbuild-core (at least as of 0.10) will read the minimum CMake from CMakeLists.txt and if it's not satisfied, it will effectively add cmake to the build-system.requires and thus use the version from PyPI. If users only want the C library, and they are on outdated distros, they should similarly pip install cmake. The only audience that should be affected are packagers, hence the delay up to now.

@lan496 lan496 merged commit 824382a into spglib:develop Aug 12, 2024
56 checks passed
@LecrisUT LecrisUT deleted the chore/cmake-3.25 branch August 13, 2024 11:54
lan496 added a commit that referenced this pull request Jan 31, 2025
@lan496
fix: Python installation (#520)
c42fc50 
Reworked a lot of the CMake build
- Python bindings installation outside of `scikit-build-core` is no
longer supported. Avoids dangling files that cannot be removed due to
lacking python package metadata files (`spglib.dist-info`)
- Reworked the detection of when the bundled project needs to be
included:
- Check for Target `Spglib::symspg` (more robust than checking for any
cache variables)
  - Use `find_package` to check for system installed `Spglib`
- Use
[`CMAKE_DISABLE_FIND_PACKAGE_Spglib=ON`](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html)
to make sure the bundled version is used
- Use
[`CMAKE_REQUIRE_FIND_PACKAGE_Spglib=ON`](https://cmake.org/cmake/help/latest/variable/CMAKE_REQUIRE_FIND_PACKAGE_PackageName.html)
to make sure a system version is used
- If `find_package` failed (and `CMAKE_REQUIRE_FIND_PACKAGE_Spglib` was
not set), use the bundled sources
- Removed the dynamic loading of the `spglib` C library, `libsymspg`
must be resolved at build time
- Added `INSTALL_RPATH` for the python `_spglib` library pointing to
`$ORIGIN/${CMAKE_INSTALL_LIBDIR}`. *If* you want to use a system
installed library, there *must not* be a `libsysmspg` library there
- Reverted the `cibuildweel` workaround from #507

Depends-on: #458 

Closes #510 Closes #462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atztogo atztogo atztogo approved these changes

@lan496 lan496 lan496 approved these changes

Assignees

@lan496 lan496

Labels
None yet
Projects
None yet
Milestone
2.6
Development

Successfully merging this pull request may close these issues.
3 participants
@LecrisUT @lan496 @atztogo