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

DOC: add documentation about using shared libraries #700

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rgommers
Copy link
Contributor

@rgommers rgommers commented Oct 27, 2024

Opening this PR now to get feedback on topics and structure, not ready for detailed review yet.

We get a lot of questions about shared libraries, and this is a tricky thing to get right. So try to document how to use internal libraries as well as link to external shared libraries as well as possible. Subproject-related questions also come up more and more, and there are some extra gotchas here, so treat those as a third "source" of shared (or static) libraries.

@rgommers rgommers added the documentation Improvements or additions to documentation label Oct 27, 2024
@rgommers rgommers force-pushed the doc-sharedlibs branch 2 times, most recently from 44f372e to 89dbcc7 Compare October 28, 2024 10:14
.cirrus.yml Outdated Show resolved Hide resolved
@rgommers
Copy link
Contributor Author

Okay, changed to os.add_dll_directory with some compat notes added. The new test case passes on all Windows configs now except for Cygwin. I don't know what's going on there - I don't think we do anything special in SciPy for Cygwin with the same kind of shared-library-inside-python-package setup. The build completes but at runtime the shared library goes missing:

[1/5] Compiling C object mypkg/cygexamplelib.dll.p/examplelib.c.o
[2/5] Compiling C object mypkg/_example.cpython-39-x86_64-cygwin.dll.p/_examplemod.c.o
[3/5] Linking target mypkg/cygexamplelib.dll
[4/5] Generating symbol file mypkg/cygexamplelib.dll.p/cygexamplelib.dll.symbols
[5/5] Linking target mypkg/_example.cpython-39-x86_64-cygwin.dll
[1/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/cygexamplelib.dll
[2/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/libexamplelib.dll.a
[3/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/_example.cpython-39-x86_64-cygwin.dll
[4/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/_example.cpython-39-x86_64-cygwin.dll.a
[5/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/mypkg/__init__.py
----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/tmp/pytest-of-runneradmin/pytest-0/mesonpy-test-venv4/lib/python3.9/site-packages/mypkg/__init__.py", line 32, in <module>
    from ._example import example_sum
ImportError: No such file or directory

@DWesl could you perhaps have a look at this? I'm not even sure it's supposed to be working, or it requires support within Cygwin somehow.

@dnicolodi
Copy link
Member

dnicolodi commented Oct 28, 2024

[1/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/cygexamplelib.dl

Why does the shared library have this funny name?

tests/test_wheel.py Outdated Show resolved Hide resolved
@rgommers
Copy link
Contributor Author

Why does the shared library have this funny name?

Seems like the fun of building on Windows - with MinGW we get libexamplelib.dll, with MSVC we get examplelib.dll and with Cygwin we get cygexamplelib.dll.

@rgommers rgommers force-pushed the doc-sharedlibs branch 2 times, most recently from ddaea0f to f95d160 Compare October 28, 2024 19:22
@dnicolodi
Copy link
Member

Would it be worth to run pyupgrade --py38-plus as part of dropping support for Python 3.7? I've just tried and it upgrades almost all typing annotations (and introduces some bugs about encoding...) thus the noise may be not worth the simplifications.

@rgommers
Copy link
Contributor Author

Would it be worth to run pyupgrade --py38-plus as part of dropping support for Python 3.7? I've just tried and it upgrades almost all typing annotations (and introduces some bugs about encoding...) thus the noise may be not worth the simplifications.

I'll have a look, but if that's useful enough then I'd prefer to do it in a separate PR I think.

@DWesl
Copy link

DWesl commented Oct 28, 2024

Okay, changed to os.add_dll_directory with some compat notes added. The new test case passes on all Windows configs now except for Cygwin. I don't know what's going on there - I don't think we do anything special in SciPy for Cygwin with the same kind of shared-library-inside-python-package setup. The build completes but at runtime the shared library goes missing:

[1/5] Compiling C object mypkg/cygexamplelib.dll.p/examplelib.c.o
[2/5] Compiling C object mypkg/_example.cpython-39-x86_64-cygwin.dll.p/_examplemod.c.o
[3/5] Linking target mypkg/cygexamplelib.dll
[4/5] Generating symbol file mypkg/cygexamplelib.dll.p/cygexamplelib.dll.symbols
[5/5] Linking target mypkg/_example.cpython-39-x86_64-cygwin.dll
[1/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/cygexamplelib.dll
[2/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/libexamplelib.dll.a
[3/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/_example.cpython-39-x86_64-cygwin.dll
[4/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/.mesonpy-8c69nja6/mypkg/_example.cpython-39-x86_64-cygwin.dll.a
[5/5] /cygdrive/d/a/meson-python/meson-python/tests/packages/sharedlib-in-package/mypkg/__init__.py
----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/tmp/pytest-of-runneradmin/pytest-0/mesonpy-test-venv4/lib/python3.9/site-packages/mypkg/__init__.py", line 32, in <module>
    from ._example import example_sum
ImportError: No such file or directory

@DWesl could you perhaps have a look at this? I'm not even sure it's supposed to be working, or it requires support within Cygwin somehow.

Most of the packages I've worked with have only depended on system DLLs/shared libraries, but the reference BLAS and Lapack libraries are in a non-standard location so I have some experience with this from NumPy. The easy things to check are permissions (both shared libraries need execute permissions to work properly) and whether the PATH is set properly to find dependent shared libraries actually linked into the extension module (ldd is the tool you're probably familiar with, though I tend to use cygcheck).

From what I remember of the last time I compiled SciPy on Cygwin a few years back, SciPy doesn't link to itself; dependencies are Python-level or header-only, so this problem wouldn't come up. I haven't run into os.add_dll_directory (in the CPython repo it looks Windows-only and would not be available on Cygwin), I've only ever directly modified PATH outside Python. I wonder whether something like:

if sys.platform == "cygwin":
    def add_dll_directory(path: str):
        os.environ["PATH"] = f"{os.environ['PATH']:s}:{path:s}"
    os.add_dll_directory = add_dll_directory

might help.

@rgommers
Copy link
Contributor Author

Thanks for the input @DWesl!

From what I remember of the last time I compiled SciPy on Cygwin a few years back, SciPy doesn't link to itself; dependencies are Python-level or header-only, so this problem wouldn't come up.

That changed recently, the SciPy 1.14.0 release has a shared library:
https://github.com/scipy/scipy/blob/ea916c6f7f487bd53e98de082649d542cc6106ed/scipy/special/meson.build#L37

I've only ever directly modified PATH outside Python. I wonder whether something like: [...] might help

I'll try but I doubt it, since modifying os.environ shouldn't affect the existing process IIRC.

but the reference BLAS and Lapack libraries are in a non-standard location so I have some experience with this from NumPy.

Is there a patch/repo somewhere for how the numpy package in Cygwin is built?

@DWesl
Copy link

DWesl commented Oct 29, 2024

Thanks for the input @DWesl!

but the reference BLAS and Lapack libraries are in a non-standard location so I have some experience with this from NumPy.

Is there a patch/repo somewhere for how the numpy package in Cygwin is built?

For NumPy:
https://github.com/numpy/numpy/blob/main/.github/workflows/cygwin.yml

For SciPy, most recent I have:
DWesl/scipy#7

That changed recently, the SciPy 1.14.0 release has a shared library

It might still work, depending on what Windows thinks is the current directory for a dlopen call and which extensions load it. I can't test at the moment, since Cygwin doesn't have Python>=3.10 (numpy/numpy#26247)

@rgommers
Copy link
Contributor Author

The Cygwin tests pass now, thanks for the pointers @DWesl. Amending os.environ['PATH'] worked ('LD_LIBRARY_PATH' did not), and is necessary when the shared library is in another directory and also when it is right next to a Python extension module that needs it. For regular Windows, curdir is searched by default so os.add_dll_directory is only necessary when the shared library is elsewhere.

@DWesl
Copy link

DWesl commented Nov 4, 2024

The Cygwin tests pass now, thanks for the pointers @DWesl. Amending os.environ['PATH'] worked ('LD_LIBRARY_PATH' did not),

I think LD_LIBRARY_PATH gets used by dlopen on Cygwin, but Python already has its own logic for that (sys.path), so that's not relevant here.

and is necessary when the shared library is in another directory and also when it is right next to a Python extension module that needs it. For regular Windows, curdir is searched by default so os.add_dll_directory is only necessary when the shared library is elsewhere.

It sounds like Windows does some helpful chdir things/PATH manipulation around its version of dlopen then (LoadDynamicLibraryEx maybe?), that Cygwin (and the lower-level Windows function it passes things off to) does not.
Interesting to know about in case it comes up again.

Copy link

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

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

This seems to be shaping up very nicely, and is super useful (drive by spelling nits).

docs/how-to-guides/shared-libraries.rst Outdated Show resolved Hide resolved
docs/how-to-guides/shared-libraries.rst Outdated Show resolved Hide resolved
@rgommers
Copy link
Contributor Author

rgommers commented Dec 2, 2024

Would it be worth to run pyupgrade --py38-plus as part of dropping support for Python 3.7? I've just tried and it upgrades almost all typing annotations (and introduces some bugs about encoding...) thus the noise may be not worth the simplifications.

I tried now. A very large diff indeed with things that mostly seemed pretty uninteresting, so I prefer to skip this exercise.

@eli-schwartz
Copy link
Member

FWIW, nearly all changes that pyupgrade does, it does for --py37-plus as well. There's no granularity whatsoever with regard to what sort of changes you're interested in making.

Consider using ruff instead, as you can enable/disable your choice of 42 distinct rules in the UP range: https://docs.astral.sh/ruff/rules/#pyupgrade-up

@rgommers rgommers force-pushed the doc-sharedlibs branch 2 times, most recently from b909b28 to ba985e6 Compare December 2, 2024 19:17
@rgommers rgommers marked this pull request as ready for review December 2, 2024 19:24
@rgommers rgommers added the tests label Dec 2, 2024
@rgommers
Copy link
Contributor Author

rgommers commented Dec 2, 2024

This should be good to go now.

Copy link
Member

@dnicolodi dnicolodi 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 working on this @rgommers! I have a first set of comments based on a first quick read through the additions. I'll come back with more.

tests/packages/sharedlib-in-package/mypkg/__init__.py Outdated Show resolved Hide resolved
tests/packages/sharedlib-in-package/mypkg/examplelib.c Outdated Show resolved Hide resolved

This function is Windows-specific due to lack of RPATH support on Windows.
It cannot find shared libraries installed within wheels unless we either
amend the DLL search path or pre-load the DLL.
Copy link
Member

Choose a reason for hiding this comment

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

I find this docstring a bit repetitive and contrived in places. I think it can be made much more concise. I can try to come up with a better formulation, if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes please, that could make it better.

tests/packages/sharedlib-in-package/mypkg/__init__.py Outdated Show resolved Hide resolved
docs/how-to-guides/shared-libraries.rst Outdated Show resolved Hide resolved
docs/how-to-guides/shared-libraries.rst Outdated Show resolved Hide resolved
docs/how-to-guides/shared-libraries.rst Outdated Show resolved Hide resolved
basnijholt added a commit to basnijholt/pfapack that referenced this pull request Dec 3, 2024
@basnijholt
Copy link

@rgommers, just wanted to thank you for writing these docs!

It has been key in getting the Windows builds in basnijholt/pfapack#21 to work.

If only I found this PR 5 hours ago 😅

Note that for Meson versions older than 1.2.0, CI failed with:
```
mesonpy.BuildError: Could not map installation path to an equivalent wheel directory: '{libdir_static}/libexamplelib.a'
```
because the `--skip-subprojects` install option isn't honored.
Hence the test skip on older versions.

In addition, the `c_shared_libs` usage requires Meson 1.3.0
@rgommers
Copy link
Contributor Author

rgommers commented Dec 4, 2024

Thanks for the review @dnicolodi, and for the feedback @basnijholt!

Copy link
Member

@dnicolodi dnicolodi left a comment

Choose a reason for hiding this comment

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

Thanks @rgommers, especially for bearing with me for the tedious work of keeping indentation consistent. I've spotted a couple more things that may warrant some attention. Otherwise I think we can just merge this and iterate on it in follow up patches, if needed.

There is another tedious thing I have noticed: the copyright years in some files are wrong. I don't care and I remember suggesting quite strongly that having the copyright year in the SPDX headers is just a waste of time, but I lost that battle 😃 and if we have the years we should have them accurate.

standalone library, then the method shown above won't work - the library has to
be installed to the default ``libdir`` location. In that case, ``meson-python``
will detect that the library is going to be installed to ``libdir`` - which is
not a recommended install location for wheels, and not supported by
Copy link
Member

Choose a reason for hiding this comment

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

I think "recommended install location" is a bit weak: there is no way for a Python wheel to install in libdir. I'm pointing it out because I foresee someone reading this and interpreting it as meson-python enforcing a recommendation as a strict rule and asking for this to be supported (we already had cases of this interpretation and of this request in the issue tracker). Maybe a stronger wording is justified here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a way: use the data directory. Unpack an mkl or pybind11-global wheel to verify - they install into prefix/lib and prefix/include. It is indeed a recommendation that we enforce as of today.

Copy link
Member

Choose a reason for hiding this comment

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

I think we may be referring to two different things. libdir as defined earlier in this document is a system wide installation location and the location where meson would install a shared library. What these wheels do is to install in the lib directory of the current Python environment, which is not the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. Yeah not the same, although it does end up being the same if you're not using a virtualenv on top of your system Python.

Let me think about how to rephrase this.

Copy link
Member

Choose a reason for hiding this comment

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

It is definitely not at all the same thing. The issue isn't that wheels cannot install outside of site-packages. They absolutely can do that by brute force, as long as the location is still in sys.prefix. The issue is that even if you assume the garden path of sys.prefix being /use and wanting to install to a location within /usr...

... on some systems this is /usr/lib, and on other systems it is /usr/lib64, and on yet more systems it is /usr/lib/x86_64-linux-gnu

There are more configurations too, this is just the most common ones for Linux specifically.

If you have system-specific wheels then this all is entirely fine. It's not physically possible to design a wheel that is simultaneously installable on Arch Linux and on Debian and on Gentoo, without adding new features to the wheel standard (described in a PEP that was rejected years ago).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rephrased this section and make it a bit shorter as well - please take another look.

Also, I defined what libdir means right above its first usage, in the table.


If this happens, workarounds include adding the directory that the shared
library is located in to the search path by amending the ``LD_LIBRARY_PATH``
environment variable, or by using a compile flag ``-Wl,-rpath,/path/to/sharedlib/dir``.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this note. Adding an RPATH is equivalent to setting LD_LIBRARY_PATH and the compiler flag is just a way to add to RPATH. Without studying them in detail, what is going on in the linked issues is that RPATH is absolute/relative but the user would like it to be relative/absolute, respectively.

The only way for the library to go missing at load time is when the RPATH is relative and the library install location is changed or the library is not at the location it was when the artifact linking to it was built.

Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is going on is that C/C++/Fortran libraries like zlib, openblas, etc. expect to be installed into a path that's on the loader search path. As a result, the link line of their .pc file doesn't include any RPATH. It will include something like -lmylib or -L/path/to/mylib -lmylib. This then works fine when the library is indeed in /usr/lib/ or another standard location.

However, if you build such a library yourself and put it elsewhere, an RPATH is needed, but not present at all. Meson isn't aware of what directories are or aren't in the loader search path, and hence it's not able to add (or avoid stripping) of RPATHs to such directories at install time. This is particularly confusing because Autotools does know how to do this (xref mesonbuild/meson#13046 (comment)).

It's actually not uncommon for this to bite you in practice if your Python package depends on a shared library, because you'll have it in some custom directory as soon as you want to do any development on it (or put it into its own wheel, like we did with scipy-openblas32).

Copy link
Member

Choose a reason for hiding this comment

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

I understand the problem you are describing in your latest comment, but I am not able to make the connection from what this PR adds to the documentation and the problem you describe. The documentation added here, almost literally says "An RPATH entry alone is not always enough... if this is the case, add an RPATH" because setting LD_LIBRARY_PATH or the compiler flag just do that.

I now understand what you have in mind, but I think the explanation in the documentation is a bit too terse: I think I have an above average understanding of these issues and still I could not make the connection. I'll try to come up with a concrete suggestion on how to explain this.

Copy link
Member

Choose a reason for hiding this comment

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

It's always enough to add your own RPATH by adding -Wl,-rpath=... to your personal LDFLAGS with the same value that you'd add to LD_LIBRARY_PATH, I guess.

The problem is not with rpath, it's with assuming that rpath will as of today be configured out of the box for you.

Copy link
Member

@eli-schwartz eli-schwartz Dec 5, 2024

Choose a reason for hiding this comment

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

hence it's not able to add (or avoid stripping) of RPATHs to such directories at install time

Once upon a long time ago meson stripped rpaths unless they were listed in the install_rpath in meson.

These days it fixed that, and now only strips rpaths that are listed exclusively in build_rpath (or point to locations inside the build tree).

So your note is not true for versions of meson that meson-python supports. Your manually added rpaths will NOT be stripped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is not with rpath, it's with assuming that rpath will as of today be configured out of the box for you.

Ah, that's a helpful way of phrasing it I think, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrote this section - shorter now, and hopefully more correct and easier to understand.

@rgommers
Copy link
Contributor Author

rgommers commented Dec 5, 2024

There is another tedious thing I have noticed: the copyright years in some files are wrong. I don't care and I remember suggesting quite strongly that having the copyright year in the SPDX headers is just a waste of time, but I lost that battle 😃 and if we have the years we should have them accurate.

IIRC what I did is for files that I copied from another test package to keep the year unchanged, and for files that I wrote from scratch I added 2024. Happy to do it some other way (2024 for everything?), but I did try to follow a consistent rule at least. No preference from my side - please let me know what you prefer here.

@dnicolodi
Copy link
Member

IIRC what I did is for files that I copied from another test package to keep the year unchanged, and for files that I wrote from scratch I added 2024. Happy to do it some other way (2024 for everything?), but I did try to follow a consistent rule at least. No preference from my side - please let me know what you prefer here.

It makes sense. I don't have a strong opinion. I would just remove the copyright years, but IANAL. Things are good as they are. Thanks @rgommers

@tobiasdiez
Copy link

Thanks a lot for these very helpful and easy-to-understand instructions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how shared libraries distributed with the wheel are handled
8 participants