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

hebasto commented at 12:13 pm on April 25, 2022: member

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

0$ 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
1$ ./autogen.sh
2$ ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site --without-libs
3$ make clean
4$ make
5...
6  CXXLD    bitcoind.exe
7/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'
8/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'
9...

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:

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

build: Make `libunivalue` a non-Libtool convenience library c91fc29e0d

maflcko added the label DrahtBot Guix build requested on Apr 25, 2022

fanquake commented at 12:20 pm on April 25, 2022: member

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.

hebasto commented at 12:22 pm on April 25, 2022: member

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.

hebasto commented at 1:09 pm on April 25, 2022: member

@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:

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

ryanofsky approved

ryanofsky commented at 3:45 pm on April 25, 2022: contributor

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.

DrahtBot commented at 5:52 pm on April 25, 2022: contributor

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

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.

hebasto commented at 6:03 pm on April 25, 2022: member

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

DrahtBot commented at 3:06 pm on April 28, 2022: contributor

🐙 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”.

DrahtBot added the label Needs rebase on Apr 28, 2022

fanquake commented at 7:56 am on April 29, 2022: member

Concept NACK. Prefer the libtool change from #25008.

DrahtBot commented at 9:59 am on August 1, 2022: contributor

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.

achow101 commented at 7:15 pm on October 12, 2022: member

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.

achow101 closed this on Oct 12, 2022

maflcko removed the label DrahtBot Guix build requested on Nov 18, 2022

bitcoin locked this on Nov 18, 2023

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

Conflicts

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