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

support --with-system-secp256k1 #414

Closed

Conversation

whitslack
Copy link
Contributor

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.

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Show resolved Hide resolved
jgriffiths pushed a commit that referenced this pull request Sep 1, 2023
Quoting jgriffiths:
> --enable-openssl-tests is no longer supported in secp and should be removed

Suggested-by: Jon Griffiths <[email protected]> (@jgriffiths)
See: #414 (comment)
@jgriffiths
Copy link
Contributor

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

@whitslack whitslack force-pushed the with-system-secp256k1 branch from b745937 to 8c7625a Compare September 1, 2023 20:46
@whitslack whitslack force-pushed the with-system-secp256k1 branch from 8c7625a to 1e6e8d3 Compare September 1, 2023 22:20
@whitslack whitslack marked this pull request as draft September 2, 2023 03:41
@whitslack whitslack force-pushed the with-system-secp256k1 branch from 1e6e8d3 to c0a97b0 Compare September 2, 2023 04:29
@whitslack whitslack marked this pull request as ready for review September 2, 2023 05:00
@whitslack whitslack force-pushed the with-system-secp256k1 branch 2 times, most recently from b2f3331 to 7e6ab8b Compare September 2, 2023 23:59
setup.py Outdated Show resolved Hide resolved
jgriffiths pushed a commit that referenced this pull request Sep 3, 2023
Quoting jgriffiths:
> --enable-openssl-tests is no longer supported in secp and should be removed

Suggested-by: Jon Griffiths <[email protected]> (@jgriffiths)
See: #414 (comment)
@whitslack whitslack force-pushed the with-system-secp256k1 branch 3 times, most recently from ee3196e to 041475a Compare September 3, 2023 01:55
@whitslack whitslack force-pushed the with-system-secp256k1 branch from 041475a to 36488de Compare September 4, 2023 04:10
@jgriffiths jgriffiths force-pushed the build_updates branch 2 times, most recently from 81ccbab to 0df2d50 Compare September 4, 2023 10:02
@jgriffiths
Copy link
Contributor

jgriffiths commented Sep 4, 2023

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 whitslack force-pushed the with-system-secp256k1 branch from 36488de to 25e1658 Compare September 4, 2023 18:40
@whitslack
Copy link
Contributor Author

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

25e1658 should address this.

@jgriffiths
Copy link
Contributor

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
Copy link
Contributor Author

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 pushed a commit that referenced this pull request Oct 2, 2023
Quoting jgriffiths:
> --enable-openssl-tests is no longer supported in secp and should be removed

Suggested-by: Jon Griffiths <[email protected]> (@jgriffiths)
See: #414 (comment)
@jgriffiths
Copy link
Contributor

Hi @whitslack sorry for the delay. I've cherry-picked and merged most of the 1.0.0 changes, with #422 scheduled to be merged next.

Can you rebase this on #422 and include your proposed fixup commit 25e1658 ? 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 whitslack force-pushed the with-system-secp256k1 branch from 25e1658 to b9434ac Compare October 3, 2023 07:45
@whitslack whitslack changed the base branch from build_updates to abi_merge October 3, 2023 07:45
@whitslack
Copy link
Contributor Author

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

Done.

jgriffiths and others added 2 commits October 5, 2023 00:23
Altering ac_configure_args to configure libsecp256k1 with options
differing from those of libwally-core causes issues if the wally
Makefile is out of date and needs to call autoreconf etc to
regenerate itself. The secp arguments are passed back to the top
level configure which changes and possibly breaks the original
configuration.

Instead, import and use AX_SUBDIRS_CONFIGURE from the autoconf-archive
which supports sub-confiration with bespoke arguments.

Because autoreconf does not recurse into subdirectories named by
AX_SUBDIRS_CONFIGURE, we lose the automagic Autotools (re)generation in
src/secp256k1, so add an explicit call to src/secp256k1/autogen.sh in
tools/autogen.sh.
(Modified from the original submission).

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), the build will use the bundled copy of
libsecp256k1_zkp as before. Remove the existing libtool hack: When
building as a static library, the user will need to link libsecp256k1.a
in addition to libwallycore.a.
@whitslack whitslack force-pushed the with-system-secp256k1 branch from b9434ac to cc65798 Compare October 5, 2023 04:24
@jgriffiths jgriffiths mentioned this pull request Oct 5, 2023
@jgriffiths
Copy link
Contributor

Merged under #423, closing.

@jgriffiths jgriffiths closed this Oct 30, 2023
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