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

update py docs for testing and local build if CMake was used for compile #384

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

ahbarnett
Copy link
Collaborator

@ahbarnett ahbarnett commented Dec 6, 2023

This fixes a bunch of obsolete docs re how to build and test the Py wrapper.

We still have an issue that I cannot persuade setup.py to look in both build/ and lib/ for the finufft_dlib in the ext_modules.
Thus for the CMake user I include copying the .so over to lib/ by hand = lame.

I hope this will get obsoleted by #374 anyway.

@ahbarnett ahbarnett requested a review from janden December 6, 2023 22:02
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I see the problem about linking to the absolute path here making it tricky to deal with both cmake and make builds. Ideally, we set libraries=['finufft'] and supply the compiler with a bunch of paths where the library could be. From the comment, it looks like the causes problems with DT_NEEDED though. I'll have to look a bit closer at this and see what can be done.

@janden
Copy link
Collaborator

janden commented Dec 6, 2023

From the comment, it looks like the causes problems with DT_NEEDED though.

Funny thing is, apparently I'm the one who wrote that comment: e64bd19

@ahbarnett
Copy link
Collaborator Author

ahbarnett commented Dec 6, 2023 via email

@janden
Copy link
Collaborator

janden commented Dec 7, 2023

So I've looked into this a little bit. We can revert the change that I made and specify finufft_dlib = "finufft" and things should still be ok. The caveat is that it'll be up to the user to specify their proper LD_LIBRARY_PATH in order for things to run. Note that this is not an issue for people who install from PyPI since libfinufft.so will be included in the package (vendored in).

There is a larger question here, which relates to the install target discussion we had last week: what should the default install procedure be: local (i.e., we keep everything in the project directory and link to these libraries when needed) or global (system level, i.e., into /usr/lib, etc. or user-level into $HOME/.local/lib settable by specifying prefix). The latter assumes the user knows how to install binary libraries and relieves us of figuring out paths etc. on the fly (we'll just assume that libfinufft has been properly installed somewhere where the PATHs point us). The former puts us in this position where we have to figure out where the library (and headers) are, which can be tricky. Maybe we can continue this discussion in an issue.

@janden
Copy link
Collaborator

janden commented Dec 8, 2023

Thought about this some more and found a simpler solution (following @blackwer's trick in d993d04): set the rpath to both library locations. Now both of the following should work:

make lib
python3 -m pip install python/finufft
python3 -c "import finufft; print(finufft.__version__)"
mkdir build
(cd build && cmake .. && make)
python3 -m pip install python/finufft
python3 -c "import finufft; print(finufft.__version__)"

Should print 2.2.0dev0 in both cases. No need for LD_LIBRARY_PATH or anything. I haven't tried yet, but I think the wheel building scripts should still work as well (since Python is able to find libfinufft, auditwheel and friends should be able to as well).

docs/install.rst Outdated Show resolved Hide resolved
@ahbarnett ahbarnett merged commit 1ced1f3 into master Dec 11, 2023
8 checks passed
@blackwer blackwer deleted the docpycmake branch December 11, 2023 19:46
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.

2 participants