build: Make `libunivalue` a non-Libtool convenience library #24972

Although bitcoin/bitcoin#22646 was a great change, it broke Windows builds with DEBUG=1:

$ make -C depends HOST=x86_64-w64-mingw32 DEBUG=1 NO_QT=1 NO_ZMQ=1 NO_WALLET=1 NO_UPNP=1 NO_NATPMP=1
$ ./autogen.sh
$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site --without-libs
$ make clean
$ make
...
  CXXLD    bitcoind.exe
/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x7d1): undefined reference to `__imp_pthread_mutex_lock'
/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x7d8): undefined reference to `__imp_pthread_mutex_unlock'
...

Please note that the --without-libs configuration option, therefore, this bug differs from bitcoin/bitcoin#19772.

Unfortunately, this is another evidence of the tough relationship between Libtool and cross-compiler for MinGW-w64. Other workarounds see here and here.

This PR avoids using Libtool for the libunivalue library at all. After bitcoin/bitcoin#22646 the libunivalue library been built not using its own build system. In such a context it will never be a shared library. Therefore, no need to use Libtool, and the libunivalue library could be a non-Libtool convenience library.

Btw, I strongly believe such an approach could also solve bitcoin/bitcoin#19772.

FWIW, while working on this PR I found a more concise fix:

--- a/src/Makefile.univalue.include
+++ b/src/Makefile.univalue.include
@@ -4,3 +4,6 @@ LIBUNIVALUE = libunivalue.la
 noinst_LTLIBRARIES += $(LIBUNIVALUE)
 libunivalue_la_SOURCES = $(UNIVALUE_LIB_SOURCES_INT) $(UNIVALUE_DIST_HEADERS_INT) $(UNIVALUE_LIB_HEADERS_INT) $(UNIVALUE_TEST_FILES_INT)
 libunivalue_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)
+if TARGET_WINDOWS
+libunivalue_la_LDFLAGS = -static
+endif

Any reason for a new PR rather than pushing the new changes to #23612? How does this interop with #24322? Looks like some of the changes are going to conflict.

Any reason for a new PR rather than pushing the new changes to #23612?

This does not fix builds with --with-libs.

Anyway, going to close #23612.

@fanquake

How does this interop with #24322? Looks like some of the changes are going to conflict.

To follow #24322 approach, this PR should contain the alternative diff mentioned in the OP:

--- a/src/Makefile.univalue.include
+++ b/src/Makefile.univalue.include
@@ -4,3 +4,6 @@ LIBUNIVALUE = libunivalue.la
 noinst_LTLIBRARIES += $(LIBUNIVALUE)
 libunivalue_la_SOURCES = $(UNIVALUE_LIB_SOURCES_INT) $(UNIVALUE_DIST_HEADERS_INT) $(UNIVALUE_LIB_HEADERS_INT) $(UNIVALUE_TEST_FILES_INT)
 libunivalue_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)
+if TARGET_WINDOWS
+libunivalue_la_LDFLAGS = -static
+endif

Code review ACK c91fc29e0d17ba662b07b1ad19a190c1ea6c94e3 for the current fix. The -static fix in #24972 (comment) also seems ok if there is a comment explaining why -static is needed on windows.

I'm not sure how these fixes relate to the original issue. If univalue test binaries depend on pthread functions, shouldn't they just be built with -lpthread? Having the test binaries inherit the library's dependencies by linking the library statically instead of dynamically seems like in an indirect workaround, if it is how this fix works.

re: #24972#issue-1214416331

After #22646 the libunivalue library been built not using its own build system. In such a context it will never be a shared library. Therefore, no need to use Libtool, and the libunivalue library could be a non-Libtool convenience library.

I think this isn't strictly true. Even if the library is only built as a static library, not a dynamic library, you may still need to build it with libtool to apply PIC flags if that static library is a dependency of a dynamic library. libbitcoinconsensus is the only dynamic library we have though, so probably this isn't a concern.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #24322 ([kernel 1/n] Introduce initial libbitcoinkernel by dongcarl)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake

How does this interop with #24322? Looks like some of the changes are going to conflict.

Just verified -- this PR is not compatible with #24322. Going to rework it.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Concept NACK. Prefer the libtool change from #25008.

<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.