Skip to content

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Aug 4, 2024

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
    • 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 Update CI dependencies #507

Depends-on: #458

Closes #510 Closes #462

@LecrisUT LecrisUT marked this pull request as ready for review August 4, 2024 16:55
Copy link

codecov bot commented Aug 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.30%. Comparing base (705beb7) to head (cf20b8c).
Report is 31 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #520   +/-   ##
========================================
  Coverage    73.30%   73.30%           
========================================
  Files           26       26           
  Lines         8068     8068           
  Branches      1691     1692    +1     
========================================
  Hits          5914     5914           
  Misses        1627     1627           
  Partials       527      527           
Flag Coverage Δ
c_api 68.68% <ø> (ø)
fortran_api 50.70% <ø> (ø)
python_api 64.14% <ø> (ø)
unit_tests 11.67% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Aug 4, 2024

Great, it seems we need to resolve #466 for this 😮‍💨

@LecrisUT
Copy link
Collaborator Author

Current status:

  • The macos issue has me completely stumped. I would need someone on a mac device to try and build the wheel and share it with me to see if there is some inconsistencies. Either building with python -m build or with cibuildwheel if possible
  • The windows issue is a design issue in that windows does not have rpath equivalent. Instead all .dll files must be in the same runtime directory, but CMake convention puts it at bin which makes it incompatible. On the other hand this clashes with the import ... path. Need some more help from a windows user to figure out how are find_package working in those environments particularly when each package has a different ROOT.
  • I had to alter the documentation because it was failing at relative imports evaluation. This should be better fixed in doc: Use autodoc #466. @lan496 can you review c8d58da and the current status of the generated docs?

@lan496
Copy link
Member

lan496 commented Aug 13, 2024

can you review c8d58da and the current status of the generated docs?

The commit looks good to me.

@LecrisUT
Copy link
Collaborator Author

can you review c8d58da and the current status of the generated docs?

The commit looks good to me.

I was rather referring to the generated documentation, e.g.:

@lan496
Copy link
Member

lan496 commented Aug 13, 2024

Oh, I didn't find a successfully built RtD for this PR.

  • From some point, the extra "None" appears in the docs.
    image

  • import spglib disappears in python-interface.md
    image

@LecrisUT
Copy link
Collaborator Author

import spglib disappears in python-interface.md

Oh weird that must have sneaked in when we introduced doctest.

From some point, the extra "None" appears in the docs.

Wasn't that some weird quirk of autodoc2? It seems to be present in develop as well. That seems to go away with autodoc, so how about moving that forward?

@LecrisUT
Copy link
Collaborator Author

Ok, managed to fix the windows and macos issues. But this PR is blocked by an upstream issue: scikit-build/scikit-build-core#860 basically if we run pip install . twice, it might not bundle spglib C library the second time

@LecrisUT
Copy link
Collaborator Author

Final blocker: scikit-build/scikit-build-core#880

@LecrisUT
Copy link
Collaborator Author

Ok, I've managed to figure out a workaround for the pip isolation failure. This PR is ready for review, but I don't know if we want to add it to 2.6.0, it has quite a big change in the build process.

Could you manually check with

$ pip install "spglib@git+https://github.com/LecrisUT/spglib@fix/510"

Particularly test that it imports correctly every time you re-run pip install, and that it works in both normal and editable mode.

You should also have a full set of wheel artifacts here

@LecrisUT LecrisUT requested review from lan496 and atztogo January 30, 2025 13:09
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
- `add_subdirectory` must not be conditional to a cache variable before it is defined (`Spglib_SOURCE_DIR` is a cache variable)
- The only appropriate conditional to use here is `TARGET`
- Moved main target definitions before "External packages". Must be after `BUILD_SHARED_LIBS` is set.

Signed-off-by: Cristian Le <git@lecris.dev>
- Python installation within CMake only is no longer supported. Avoid missing metadata files.
- Install everything under the `spglib` package using default `GNUInstallDirs` paths

Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
@lan496
Copy link
Member

lan496 commented Jan 31, 2025

Awesome! I've checked both normal and editable installs, and they work.

@lan496 lan496 merged commit c42fc50 into spglib:develop Jan 31, 2025
57 checks passed
@LecrisUT LecrisUT deleted the fix/510 branch January 31, 2025 12:15
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.

Revise using auditwheel in ci-buildwheel Controlling python's C library import spglib 2.1.0 fails on alpine/python3.11
2 participants