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.

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

clean up linkage of libsecp256k1 into libwallycore and Python extension #417

Closed
wants to merge 6 commits into from
Closed

clean up linkage of libsecp256k1 into libwallycore and Python extension #417

wants to merge 6 commits into from

Conversation

whitslack
Copy link
Contributor

@whitslack whitslack commented Sep 1, 2023 •
edited
Loading

This PR has two remaining objectives:

  • Eliminate the FIXME in configure.ac relating to embedding the symbols from libsecp256k1.a when building libwallycore.so.
  • Adding an option to setup.py to link the Python native extension dynamically with a libwally-core DSO.

@whitslack whitslack changed the title use AX_SUBDIRS_CONFIGURE; don't alter ac_configure_args use AX_SUBDIRS_CONFIGURE; don't alter ac_configure_args Sep 1, 2023
@whitslack
Copy link
Contributor Author

whitslack commented Sep 1, 2023 •
edited
Loading

@jgriffiths: I noticed one regression when switching from AC_CONFIG_SUBDIRS to AX_SUBDIRS_CONFIGURE: autoreconf no longer recurses into src/secp256k1, meaning the Autotools infrastructure needs to be explicitly (re)generated in src/secp256k1. I am not sure if there will be an impact to your automated testing, continuous integration, and/or release engineering processes due to this regression.

@whitslack whitslack force-pushed the ax_subdirs_configure branch from 7654b9d to 34fd67d Compare September 2, 2023 02:29
@whitslack
Copy link
Contributor Author

whitslack commented Sep 2, 2023 •
edited
Loading

I am not sure if there will be an impact to your automated testing, continuous integration, and/or release engineering processes due to this regression.

Indeed, there was a break in that setup.py was relying on the top-level tools/autogen.sh to recursively run autoreconf in src/secp256k1. That no longer happens, so I added an explicit call to src/secp256k1/autogen.sh in setup.py tools/autogen.sh.

@whitslack whitslack marked this pull request as draft September 2, 2023 03:40
@whitslack whitslack marked this pull request as ready for review September 2, 2023 05:00
whitslack
whitslack commented Sep 2, 2023
View reviewed changes
configure.ac Show resolved Hide resolved
@whitslack whitslack force-pushed the ax_subdirs_configure branch from 5dc21e0 to 61584a1 Compare September 2, 2023 06:24
@jgriffiths
Copy link
Contributor

That no longer happens, so I added an explicit call to src/secp256k1/autogen.sh in setup.py.

I think this explicit call should be added to tools/autogen.sh instead of in setup.py. This will then not affect the other users of the library who build wally according to the README instructions (in particular the Green wallets and c-lightning).

@whitslack whitslack force-pushed the ax_subdirs_configure branch from 61584a1 to 69f35ee Compare September 2, 2023 23:59
@jgriffiths
Copy link
Contributor

cherry-picked e14ada8 to reduce rebase pain.

@whitslack whitslack force-pushed the ax_subdirs_configure branch from 69f35ee to e377be1 Compare September 3, 2023 00:13
@jgriffiths
Copy link
Contributor

@whitslack can you rename WALLY_BUILD_SHARED to WALLY_ABI_PY_WHEEL_SHARED?

@whitslack
Copy link
Contributor Author

can you rename WALLY_BUILD_SHARED to WALLY_ABI_PY_WHEEL_SHARED?

@jgriffiths: Hmm, yes, but is that a clear name for what the variable controls? To me, that name suggests that the variable controls whether the wheel is to be shared, which doesn't entirely make sense. What we're actually talking about here is whether the Python native extension library (which is always a shared library included in the wheel) will link dynamically to libwally-core (or else will embed the libwally-core and libsecp256k1 code wholly within the Python native extension library).

@jgriffiths
Copy link
Contributor

Naming things, always the hardest part of coding lol.

How about WALLY_ABI_PY_WHEEL_USE_DSO? indicating that the wheel is using dynamic shared objects?

@whitslack
Copy link
Contributor Author

WALLY_ABI_PY_WHEEL_USE_DSO

Okay, sure. I have no strong objections to that name. It's a little verbose, but it's okay.

@whitslack whitslack force-pushed the ax_subdirs_configure branch from e377be1 to 1ef4131 Compare September 3, 2023 01:03
@jgriffiths
Copy link
Contributor

In the first commit you need to include the m4 file defining AX_SUBDIRS_CONFIGURE from the autoconf archive, I think you've forgotten to check it in?

@whitslack
Copy link
Contributor Author

In the first commit you need to include the m4 file defining AX_SUBDIRS_CONFIGURE from the autoconf archive, I think you've forgotten to check it in?

It's part of Autoconf Archive, which I think you're already using. autoreconf takes care of finding the macro definition beneath /usr{,/local}/share/aclocal.

@jgriffiths
Copy link
Contributor

It's part of Autoconf Archive

I'm aware; we check in the macros we need from the archive under tools/build-aux/m4/, we don't expect the user to have it installed.

@whitslack
Copy link
Contributor Author

we check in the macros we need from the archive

Do you monitor the Archive for bug fixes and feature enhancements in the macros you've copied from it, and do you promptly make new releases of libwally-core whenever the macros you're using have received such a bug fix or enhancement? This is the static linking versus dynamic linking debate all over again. 😅 Maybe one of your users has an unusual setup that causes an Autoconf Archive macro to misbehave. Upon researching it, they discover that the misbehavior has already been corrected in the Archive, but because you haven't been keeping up with fixes, your project is broken for them, whereas everything would have been fine if you hadn't made your own private copy that inevitably becomes outdated. (I'm sure this comes across as though I'm very perturbed, but I am not. It's a difference of philosophy as old as the concept of linking itself. I don't have any illusions of changing your mind.) I'll make a copy of the macro definition and check it in. 🤦‍♂️

@jgriffiths jgriffiths mentioned this pull request Sep 3, 2023
Closed
@whitslack whitslack force-pushed the ax_subdirs_configure branch from 1ef4131 to 84d2b50 Compare September 3, 2023 01:55
@whitslack
Copy link
Contributor Author

I'll make a copy of the macro definition and check it in.

This is done.

@jgriffiths
Copy link
Contributor

I don't have any illusions of changing your mind

Note the requirements for wally, exposed functionality, how it is expected to be used and the time available for me to work on it are all determined by my day job; my personal opinions on the correct way of doing things is irrelevant and I'm not arguing from my POV at all. I'm not adverse to explaining the rationale behind these decisions as I understand them but your views (however correct they may be) don't supersede my work obligations.

I'll be removing some verbage from your commit descriptions before finalizing the build_updates branch, I will call for final review and get an ack from you before merging this branch (which may take some time given it needs further work and internal testing).

whitslack and others added 3 commits September 4, 2023 14:00
@whitslack @jgriffiths
configure.ac: use AX_SUBDIRS_CONFIGURE; don't alter ac_configure_args
a5b1ba5 
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.
@jgriffiths
secp: use the configured include path for secp headers
62a3542 
Extracted from a patch by Matt Whitlock <[email protected]>.

Also reduce the include directories given for python wheel building.
@jgriffiths
docs: update for v1.0.0
bb52971
@whitslack
Copy link
Contributor Author

whitslack commented Sep 4, 2023 •
edited
Loading

I'll be removing some verbage from your commit descriptions

Since you removed the paragraph beginning "At the insistence of this project's maintainer," I would respectfully request that you git commit --amend --reset-author. I don't want to take the blame for creating technical debt, but I am completely fine with being unattributed. In other words, I don't care about getting credit for my work, but I do care about not being blamed for doing something that I opposed. Make sense?

@whitslack
clean up linkage of libsecp256k1 into libwallycore and Python extension
e95a6d9 
We have two levels of library embedding:

 * libsecp256k1.la is linked into libwallycore.la via LIBADD. The FIXME
   comment in configure.ac contained an assertion that seems to be
   irrelevant: passing libsecp256k1.la to libtool when linking
   libwallycore.la indeed does not embed the objects from libsecp256k1.a
   into libwallycore.a, but it does add a dependency on libsecp256k1.la
   in libwallycore.la, so any future links against libwallycore.la will
   automagically pull in libsecp256k1.la. This is exactly the scenario
   that libtool was conceived to facilitate. Do note one caveat: if both
   --enable-shared and --enable-static are passed to configure, then
   libwallycore.la will *not* specify a dependency on libsecp256k1.la,
   but this is irrelevant since any future links against libwallycore.la
   will link against libwallycore.so, which *does* contain all of the
   symbols from libsecp256k1.a, so there is no additional dependency.

 * libwallycore.la is linked into the Python native extension library.
   Previously this wasn't really being done at all; rather, all of the
   libwally-core code was being recompiled into some "combined" objects
   that would then be linked into the Python extension. This was a
   needless duplication of work since the compiled objects already exist
   in libwallycore.a and libsecp256k1.a. Simply link those libraries
   into the extension. Note that libwally-core does need to be compiled
   as PIC for this to work, so setup.py passes --with-pic to configure.

Additional cleanup: adjust several #include directives to reference
libsecp256k1 headers in the configured header search path rather than
explicitly specifying paths to the instances beneath src/secp256k1/.
@whitslack
setup.py: support optionally building libwally-core as a shared library
9b451a8 
Iff the environment contains WALLY_ABI_PY_WHEEL_USE_DSO=1, then:

 * setup.py will configure and build libwally-core as a shared library;
 * the Python native extension library will not contain any
   libwally-core or libsecp256k1 code; and
 * the dynamic linker will need to find and load libwallycore.so.1
   whenever the Python native extension is loaded.
@whitslack whitslack force-pushed the ax_subdirs_configure branch from 84d2b50 to 9b451a8 Compare September 4, 2023 04:10
@whitslack whitslack changed the title use AX_SUBDIRS_CONFIGURE; don't alter ac_configure_args clean up linkage of libsecp256k1 into libwallycore and Python extension Sep 4, 2023
@jgriffiths
Copy link
Contributor

I would respectfully request that you git commit --amend --reset-author.

Will do.

@jgriffiths jgriffiths force-pushed the build_updates branch 2 times, most recently from 81ccbab to 0df2d50 Compare September 4, 2023 10:02
@whitslack
Copy link
Contributor Author

Superseded by #419.

@whitslack whitslack closed this Sep 4, 2023
@whitslack whitslack deleted the ax_subdirs_configure branch September 4, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgriffiths jgriffiths jgriffiths left review comments

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