ElementsProject / libwally-core

Useful primitives for wallets
Other
280 stars 134 forks source link

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

Closed whitslack closed 10 months ago

whitslack commented 11 months ago

This PR has two remaining objectives:

whitslack commented 11 months ago

@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 commented 11 months ago

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.

jgriffiths commented 11 months ago

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).

jgriffiths commented 11 months ago

cherry-picked e14ada812e8aa720fe29740563a86d788e34a291 to reduce rebase pain.

jgriffiths commented 11 months ago

@whitslack can you rename WALLY_BUILD_SHARED to WALLY_ABI_PY_WHEEL_SHARED?

whitslack commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.

jgriffiths commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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. 🤦‍♂️

whitslack commented 11 months ago

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

This is done.

jgriffiths commented 10 months ago

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 commented 10 months ago

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?

jgriffiths commented 10 months ago

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

Will do.

whitslack commented 10 months ago

Superseded by #419.