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
build: Make libunivalue
a non-Libtool convenience library
#24972
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK c91fc29 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 (comment)
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 thelibunivalue
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
🐙 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. |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
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. |
Although #22646 was a great change, it broke Windows builds with
DEBUG=1
:Please note that the
--without-libs
configuration option, therefore, this bug differs from #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 #22646 thelibunivalue
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 thelibunivalue
library could be a non-Libtool convenience library.Btw, I strongly believe such an approach could also solve #19772.
FWIW, while working on this PR I found a more concise fix: