ElementsProject / libwally-core

Useful primitives for wallets
Other
284 stars 136 forks source link

support `--with-system-secp256k1` #414

Closed whitslack closed 1 year ago

whitslack commented 1 year ago

Libwally-core, in its stock configuration, compiles and statically links against a private copy of libsecp256k1_zkp. However, Gentoo unbundles libsecp256k1_zkp and instead links libwally-core against a system-wide shared libsecp256k1_zkp with headers in /usr/include/secp256k1_zkp and a libsecp256k1_zkp.pc that specifies the relevant CFLAGS and LIBS.

Implement a --with-system-secp256k1 configure option to allow compiling and linking against a system-installed libsecp256k1. Pass the user-specified package name (or libsecp256k1 or libsecp256k1_zkp by default, depending on --enable-standard-secp) to PKG_CHECK_MODULES to find the CFLAGS and LIBS of a system-installed libsecp256k1. Call AC_CHECK_FUNCS to assert that the required modules are present in the system-installed libsecp256k1.

If the user does not specify --with-system-secp256k1 (or specifies --without-system-secp256k1), then the pre-existing behavior is preserved: namely, the build will use the bundled copy of libsecp256k1_zkp.

Adjust several #include directives to reference secp256k1 headers in the configured header search path rather than explicitly specifying paths to the private copies. In the case of using the bundled libsecp256k1_zkp, src/secp256k1/include is added to the header search path; otherwise, the necessary CFLAGS, if any, are taken from pkg-config.

jgriffiths commented 1 year ago

@whitslack you'll need to rebase, I'm keeping the docs updates and version bump floating as the top commits.

jgriffiths commented 1 year ago

@whitslack I've merged the core change for --with-system-secp256k1 and removed the commit comment about the missing secp dependency being irrelevant as its clearly a libtool bug and users may force static linking in which case the missing dependency is relevant. I've documented instead that users should manually link with secp when statically linking.

I think this just leaves the python/setup.py changes outstanding. If you can close these two outstanding PRs, rebase and put the Python changes into a single PR then I'll look at how to merge them. Note I won't be changing the amalgamation compile by default, but it should be possible to allow you to build without it via env var or similar.

edit: PKG_CHECK_MODULES breaks the configure script if pkg-config is not installed, even if the user isn't passing --with-system-secp256k1. This will need reworking.

whitslack commented 1 year ago

Note I won't be changing the amalgamation compile by default, but it should be possible to allow you to build without it via env var or similar.

@jgriffiths: Can you clarify? It doesn't make sense to include both the amalgamated sources and -lwallycore.

PKG_CHECK_MODULES breaks the configure script if pkg-config is not installed

Who the heck is building a software package from source without having pkg-config installed? Lolwhut?! That's almost as essential as make. But hmm, I'll see if I can hack around it. 🤔

whitslack commented 1 year ago

I've documented instead that users should manually link with secp when statically linking.

When building as a static library, the user will need to link libsecp256k1.a in addition to libwallycore.a.

This is a little misleading. Preferably users shouldn't be specifying any .a files when linking against a libtool-built library; they should specify libwallycore.la, which will automatically handle adding both the libwallycore.a and the required libsecp256k1.a to the link. It's only for non-libtool build systems that one would need to manually specify dependency libraries.

whitslack commented 1 year ago

PKG_CHECK_MODULES breaks the configure script if pkg-config is not installed

25e1658dfb82fa58aa4474c5f90566a046d66a16 should address this.

jgriffiths commented 1 year ago

they should specify libwallycore.la, which will automatically handle adding both the libwallycore.a and the required libsecp256k1.a to the link.

As above, this is broken with libtool if they build dynamic and static, but want to link statically. But also, not everyone uses libtool to link their app, and wally should not force that upon them.

whitslack commented 1 year ago

As above, this is broken with libtool if they build dynamic and static, but want to link statically.

True, but isn't that a moot point? If both shared and static libraries are found, -lwallycore will prefer the shared one.

Anyway, I have no real point. If someone uses libtool, then they ought to know that they're supposed to link against only libwallycore.la and let libtool handle the dependency, so it doesn't need to be spelled out to them.

jgriffiths commented 1 year ago

Hi @whitslack sorry for the delay. I've cherry-picked and merged most of the 1.0.0 changes, with https://github.com/ElementsProject/libwally-core/pull/422 scheduled to be merged next.

Can you rebase this on https://github.com/ElementsProject/libwally-core/pull/422 and include your proposed fixup commit https://github.com/ElementsProject/libwally-core/commit/25e1658dfb82fa58aa4474c5f90566a046d66a16 ? the CI will then prove whether the build is working without pkgconfig installed. Once this commit is merged all that remains is the python linkage PR which I'll look at once this change is in, thanks.

whitslack commented 1 year ago

Can you rebase this on #422 and include your proposed fixup commit 25e1658 ?

Done.

jgriffiths commented 1 year ago

Merged under https://github.com/ElementsProject/libwally-core/pull/423, closing.