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

Python linkage enhancements #433

Closed

Conversation

whitslack
Copy link
Contributor

(continued from #419)

  • On systems supporting Libtool (i.e., anything but Windows), skip compiling the "amalgamated" sources for the Python module and simply link in the just-built libwallycore.a and libsecp256k1.a.

  • setup.py: support optionally building and linking to libwally-core as a dynamic shared object if WALLY_ABI_PY_WHEEL_USE_DSO=1 is set. Ignored on Windows, where setup.py does not build libwally-core.

@whitslack whitslack changed the base branch from master to build_updates November 16, 2023 09:42
@whitslack
Copy link
Contributor Author

Oh, I see this is failing. It still works for Gentoo since my ebuild deletes the whole if not IS_WINDOWS block since we do the whole library build before starting the Python build, but it no longer works if the Python module is to be built without first building the library. I'll massage this some more later today.

@jgriffiths
Copy link
Contributor

This is probably broken since the latest changes only generate a couple of files and then build with the amalgamation, instead of compiling everything twice.

In terms of a mutually acceptable change, I expect the existing build to remain unchanged (i.e. use the amalgamation) for both win and non-win. You should control the optional building with shared linkage/prebuilt libs entirely with your env var.

@whitslack
Copy link
Contributor Author

whitslack commented Nov 16, 2023

As of 40c5e5a, I no longer need to hack setup.py at all to achieve a holy-grail build, yet it should still work the way you're accustomed to if no special env vars are set.

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@jgriffiths
Copy link
Contributor

I'm pulling this into the build_updates branch and rebasing it back behind the v1.0 version bump. Feel free to update from that branch so when its merged this will be marked merged too, thanks.

@jgriffiths
Copy link
Contributor

@whitslack can you confirm that #432 works for you?

@whitslack
Copy link
Contributor Author

whitslack commented Nov 19, 2023

can you confirm that #432 works for you?

@jgriffiths: 1e6a4b0 ticks every box on my wish list (except for one, which I hadn't previously mentioned):

  • setup.py does not invoke Autotools if src/Makefile already exists, in which case it's assumed that the caller has built libwally-core in the usual way before starting the Python build. The Python module is an optional add-on, and we don't want the Python build to commandeer (or, for that matter, to ignore) the main library build.
  • setup.py does not invoke Git if it's going to link with libwally-core as a library and it's going to use a system-installed libsecp256k1-zkp. We don't want to require Git as a build-time dependency, and indeed Gentoo ebuilds run in a network-less sandbox during most build phases, so fetching libsecp256k1 sources during the src_compile phase isn't possible even if Git is installed on the build system.
  • setup.py does not recompile any libwally-core code when instructed to link with libwally-core as a library and such library already exists in the build tree. Especially we do not want duplicated code in the Python native extension library since we'll always have libwallycore.so installed whenever we have the Python module installed.

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.

@whitslack whitslack deleted the python-linkage branch November 19, 2023 01:50
@jgriffiths
Copy link
Contributor

@whitslack thanks for testing, and your summary of matters from the gentoo POV.

I am hoping to release 1.0.0 imminently so your final point will not make this release. I am open to addressing it in a future release though, even if in an optional manner as per the setup.py changes. Feel free to raise an issue with this info for tracking.

I appreciate your patience and responsiveness in getting these changes merged.

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