Skip to content

don't embed Python code into libwallycore.so #501

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
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

don't embed Python code into libwallycore.so #501

wants to merge 2 commits into from

Conversation

whitslack
Copy link
Contributor

Embedding SWIG Python code into libwallycore.so ties the library to a specific version of Python, thereby precluding concurrent installation of the wallycore Python module for multiple implementations of Python on the same system if the Python modules are linked with a system-wide libwallycore.so. On Gentoo at least, linking libwallycore.so with libpython3.14.so and then attempting to use the wallycore Python module from Python 3.11, 3.12, or 3.13 causes an immediate segfault.

Rather than injecting Python-specific glue into libwallycore.so, we can and should keep it contained within the Python native extension library that we build for each Python implementation. The Python wheel actually already compiles swig_python_wrap.c and links it into the native extension library, so linking it into libwallycore.so as well is redundant and harmful.

  • Remove libswig_python.la from libwallycore_la_LIBADD, and expunge its existence from Makefile.am entirely since it's now unused.

  • The Python wheel build for each Python implementation compiles swig_python_wrap.c against the headers for that particular Python implementation, ensuring that the resulting native extension library matches the ABI of the Python implementation for which it is installed. This may be a different Python implementation than the one found by Autoconf, the only relevance of which now is in finding the interpreter with which to run the tests.

  • Since we no longer link libwallycore.so with libpython*.so, there should no longer be any manylinux compatibility issues, so drop the --enable-python-manylinux Autoconf option and the PYTHON_MANYLINUX Automake conditional. This also means that the test programs no longer need to link with $(PYTHON_LIBS) since libwallycore.la now never has any dependence on Python (implicit or explicit).

  • Note that the Python native extension libraries do not explicitly link with libpython*.so either. This is correct, as the dynamic linker resolves their undefined Py* symbols when it loads them into the Python interpreter.

After applying these changes, the Gentoo ebuild for libwally-core 1.5.0 now successfully runs the SWIG Python tests on all currently supported versions of Python (3.11 through 3.14), all using the same libwallycore.so.6.

For reference, this is a follow-up to a comment I made back in November 2023:

The one gripe I still have, which I am okay to address in a later release, is that the libwally-core library differs depending on whether the Python module is configured to be built. Really, libwallycore.so should have no awareness of Python in any configuration, and all necessary Python glue should reside in the Python native extension library. This is especially important when we install the Python module concurrently for multiple versions of Python. For instance, on my system I have the wallycore Python module installed for both Python 3.11 and 3.12, but /usr/lib64/libwallycore.so.1.0.0 specifies a dynamic link with libpython3.12.so.1.0. I don't have a good feeling about that link when the module is loaded under Python 3.11. Really, libwallycore.so should not link with any libpython*.so at all, as such a link indicates an abstraction leak / layer violation.

That bad feeling has indeed blossomed into an actual segfault as of Python 3.14.

At that same time, @jgriffiths noted:

I am open to addressing [your final point] in a future release[…]. Feel free to raise an issue with this info for tracking.

@jgriffiths
Copy link
Contributor

Hi @whitslack the CI does not like these changes, that will need to be fixed. The CI build results must be uploadable to PyPI as complete wheels, i.e. if a new venv is created then pip install wallycore==<version> must result in a usable installed wheel with no external dependencies other than libc.

You may need to gate this change behind a configure flag.

@jgriffiths jgriffiths mentioned this pull request Aug 19, 2025
Merged
@jgriffiths
Copy link
Contributor

So, this breaks make check in a standard build, for both python and Java.

In #505 I've fixed python, but you will need to fix java to get this merged. I'm releasing 1.5.1 without it as I need python 13 wheels in pypi.

@whitslack
Copy link
Contributor Author

I do see the issue you raised about make check. As it turns out, Gentoo's distutils-r1.eclass automatically adds its Python module installation staging directory to PYTHONPATH, so I think that's why I didn't catch this issue in testing. Unfortunately, I am not sure if I can nicely add setup.py build's staging location to PYTHONPATH in src/Makefile.am, as I can't seem to find an elegant way of querying it from shell code. I can see that setup.py build stages it in build/lib.linux-x86_64-cpython-313 in my case, but obviously that path is variable depending on the host architecture and Python interpreter in use. I can't see any way to query setup.py from a shell script (or Makefile) to discover the location that it actually used. I guess this is why you went the way of virtualenv in your latest PR. Regrettably, once you merge that PR, I'm going to have to start carrying a patch alongside the Gentoo ebuild to revert it, as we don't use virtualenvs (or Pip) when building Python packages in Gentoo. (I don't even know how they work; I've literally never used one despite having written Gentoo ebuilds for dozens of Python packages.)

I'll see if I can rebase this PR atop your virtualenv and get it working. At the end of the day, though, I might just give up and do things the Gentoo way on Gentoo and not worry about everyone else having a clean libwallycore.so if I get too frustrated with virtualenv. 🤞 It seems like your preferred distribution method is a fully self-contained wheel complete with statically linked kitchen sink anyway, so maybe nobody else even installs a libwallycore.so built from a libwally-core source tree that was configured with --enable-swig-python, so maybe I'm barking up a tree that nobody else cares about.

whitslack added a commit to whitslack/libwally-core that referenced this pull request Aug 20, 2025
@whitslack
src/Makefile.am: install Python module in src/swig_python/ for testing
7e87b7a 
Running the SWIG Python tests requires that Python is able to find the
wallycore native extension. To save users from needing to build it
manually and add its location to `PYTHONPATH` themselves, we add a
target dependency in `src/Makefile.am` to trigger the Python build
before we'll need the native extension for running the SWIG Python
tests. It's unfortunate that we can't merely call `setup.py build` and
then ask setuptools where it stashed its staging copy of the module, as
there seems to be no accessible query interface for that information. So
instead we have to tell `setup.py` to install the module to a location
that we do know, and then we can pass that location in `PYTHONPATH` when
we run the SWIG Python tests.

See: ElementsProject#501 (comment)
@whitslack
Copy link
Contributor Author

Okay, so this is working now as of 7e87b7a.

$ git fetch origin refs/pull/501/head
$ git checkout FETCH_HEAD
$ git clean -fdx
$ autoreconf -i
$ ./configure --enable-export-all --enable-tests --enable-elements --enable-asm --enable-swig-java --enable-swig-python
$ make
$ make check

I haven't run any into issues with the Java tests, either before this latest commit or after. What problem are you observing?

@jgriffiths
Copy link
Contributor

I'm going to have to start carrying a patch alongside the Gentoo ebuild to revert it

If you don't run make check when building your wheels you don't need to revert this surely?

@whitslack
ax_python_devel.m4: update to version 37 from Autoconf Archive
c0e4e9d
@whitslack
don't embed Python code into libwallycore.so
5e137d0 
Embedding SWIG Python code into libwallycore.so ties the library to a
specific version of Python, thereby precluding concurrent installation
of the wallycore Python module for multiple implementations of Python
on the same system if the Python modules are linked with a system-wide
libwallycore.so. On Gentoo at least, linking libwallycore.so with
libpython3.14.so and then attempting to use the wallycore Python module
from Python 3.11, 3.12, or 3.13 causes an immediate segfault.

Rather than injecting Python-specific glue into libwallycore.so, we can
and should keep it contained within the Python native extension library
that we build for each Python implementation. The Python wheel actually
already compiles swig_python_wrap.c and links it into the native
extension library, so linking it into libwallycore.so as well is
redundant and harmful.

* Remove libswig_python.la from libwallycore_la_LIBADD, and expunge its
  existence from Makefile.am entirely since it's now unused.

* The Python wheel build for each Python implementation compiles
  swig_python_wrap.c against the headers for that particular Python
  implementation, ensuring that the resulting native extension library
  matches the ABI of the Python implementation for which it is
  installed. This may be a different Python implementation than the one
  found by Autoconf, the only relevance of which now is in finding the
  interpreter with which to run the tests.

* Since we no longer link libwallycore.so with libpython*.so, there
  should no longer be any manylinux compatibility issues, so drop the
  --enable-python-manylinux Autoconf option and the PYTHON_MANYLINUX
  Automake conditional. This also means that the test programs no longer
  need to link with $(PYTHON_LIBS) since libwallycore.la now never has
  any dependence on Python (implicit or explicit).

* Note that the Python native extension libraries do not explicitly link
  with libpython*.so either. This is correct, as the dynamic linker
  resolves their undefined Py* symbols when it loads them into the
  Python interpreter.

* manylinux intentionally omits libpython*.so libraries from its build
  containers to discourage/prevent wheels' native extension libraries
  from linking against the Python DSO. We don't do that (anymore), but
  the AX_PYTHON_DEVEL Autoconf macro expects to be able to find it and
  fails its final sanity check if it can't. Thankfully, more recent
  versions of the macro support an 'optional' flag that allows configure
  to proceed even if the check fails. So now we set that flag and just
  check that we have a value in $PYTHON_CPPFLAGS, which is all the
  SWIG_PYTHON macro cares about in the first place.

After applying these changes, the Gentoo ebuild for libwally-core 1.5.0
now successfully runs the SWIG Python tests on all currently supported
versions of Python (3.11 through 3.14), all using the same
libwallycore.so.6.
@whitslack
Copy link
Contributor Author

whitslack commented Aug 21, 2025 •
edited
Loading

I'm going to have to start carrying a patch alongside the Gentoo ebuild to revert it

If you don't run make check when building your wheels you don't need to revert this surely?

We do run make check (at the user's option). Actually, we run make check-TESTS check-libwallycore if the user has enabled FEATURES="test" and then additionally make check-swig-java if they've enabled USE="java" and then additionally make check-swig-python if they've enabled USE="python".

The reversion turned out not to be a big deal. All I needed was this:

sed -e 's:\$(top_builddir)/venv/bin/python \([^-]\):$(PYTHON_TEST) \1:' \
	-e '/\$(top_builddir)\/venv/d' -i src/Makefile.am

That changes all the (non-PIP) $(top_builddir)/venv/bin/python calls back to $(PYTHON_TEST) calls and then deletes the two lines that set up the virtualenv and populate it.

@jgriffiths
Copy link
Contributor

@whitslack I've got a couple of things to tend to but will look at this early next week. Assuming no issues I should be able to release it as v1.5.2 fairly soon and hopefully minimise the packaging burden for you going forward.

@whitslack
Copy link
Contributor Author

whitslack commented Aug 21, 2025 •
edited
Loading

Honestly, you might want to wait until 1.6.0, as this is not a trivial change. Technically, it's an ABI-breaking change, as libwallycore.so no longer includes any Python symbols, although those should have been considered "undocumented, private ABI" all along, and anyone depending on them was asking for breakage. I leave it up to you to decide whether this warrants a change in soname by itself. (I could go either way.) Of course, if you batch it with a future breaking change to the public ABI, then the question would be moot.

@jgriffiths
Copy link
Contributor

Good to know, thanks. I'll have a look at the roadmap and make a call next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@whitslack @jgriffiths