Skip to content

Conversation

henryiii
Copy link
Contributor

Using Python for the python portion is better, especially for editable installs. Still seems to be an issue in scikit-build-core with the path on the editable install for the .so file, though, so will look into it.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii
Copy link
Contributor Author

This won't work in an editable install:

sorted(os.listdir(os.path.dirname(__file__))),

Because the .so file is not next to the python file. The Python file is in the original source location, and the .so file is in site-packages. I'll need to think about how this should be done. In what case does it not trigger the ImportError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, MANIFEST.in is a setuptools thing. Scikit-build-core uses sdist.include and sdist.exclude.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, I have that in a clean-up pr

@LecrisUT
Copy link
Collaborator

Thanks for the help on this

@LecrisUT
Copy link
Collaborator

LecrisUT commented Apr 12, 2023

This won't work in an editable install:

sorted(os.listdir(os.path.dirname(__file__))),

Because the .so file is not next to the python file. The Python file is in the original source location, and the .so file is in site-packages. I'll need to think about how this should be done. In what case does it not trigger the ImportError?

Actually I think we can do this with RPATH. The first part is so that it can use the system's libsymspg.so, but if it cannot find it, check for the bundled version. But we can hijack this by either adding an RPATH to _spglib.so pointing to the installed location of libsymspg.so, or changing the needed library to be absolute path. The RPATH method is usually the case when you first build it and then it gets stripped by cmake --install. Let's see how can we tell cmake to add an RPATH on install: CMAKE_INSTALL_RPATH.

How about exposing two tool.scikit-build options (with more sensible naming):

  • installed-library-path: a list of paths relative to CMAKE_INSTALL_PREFIX pointing to where the library paths are installed (maybe this is exposed by cmake-file-api so that it can be deduced from the target and dependencies). Use this as CMAKE_INSTALL_RPATH. Default to CMAKE_INSTALL_LIBDIR
  • install-rpath: whether or not to install rpath (or to override the rpath list) on non-editable installs. Always append rpath on editable installs. Default false

Editable installs overwrite the paths relative to the package names, and it fails when directory relative paths are used

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator

Ok I have added a quick fix for the python paths to make it work with editable installs. I have also tested the RPATH and it works fine. Just needs an implementation in scikit-build-core ;)

For reference here is what I did manually:

$ pip install -e .
$ cd venv/lib/python3.11/site-packages/spglib
$ patchelf --add-rpath $(pwd) _spglib.cpython-311-x86_64-linux-gnu.so
$ readelf -d _spglib.cpython-311-x86_64-linux-gnu.so
$ cd ../../../../../
$ pytest

The patchelf command should be equivalent with CMAKE_INSTALL_RPATH.

PS: the 3-4 rebuilding issue is also fixed by this

@codecov-commenter
Copy link

codecov-commenter commented Apr 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (c3af1dc) 85.90% compared to head (186997e) 85.90%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #269   +/-   ##
========================================
  Coverage    85.90%   85.90%           
========================================
  Files           23       23           
  Lines         6071     6071           
========================================
  Hits          5215     5215           
  Misses         856      856           
Flag Coverage Δ
c_api 74.23% <ø> (ø)
fortran_api 37.36% <ø> (ø)
python_api 82.83% <ø> (ø)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LecrisUT LecrisUT changed the title chore: use Python for package copy Python: fix packaging Apr 13, 2023
@LecrisUT LecrisUT merged commit 42527b0 into spglib:develop Apr 13, 2023
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.

3 participants