build: Build libbitcoinconsensus from its own convenience library
#24994
Conversation
dde78a6 to
7b907f0
Compare
libbitcoinconsensus Windows builds with DEBUG=1libbitcoinconsensus from its own convenience library
|
Concept NACK.
Shuffling a bunch of code, creating a new library, and introducing a new circular dependency to fix a Windows only, debug build of libbitcoinconsensus (which, given how long it hasn't worked for, can't be a problem for many people) does not seem like a good tradeoff. I would wait and see what falls out of the discussion in #24322, as I think it's very likely that is going to produce a more general solution to this problem, and solving #19772. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
7b907f0 to
c52f7c4
Compare
|
Updated.
Much less now.
Fixed.
Actually, it is a problem for a few developers who cannot test
I see this as a nice abstraction layer that has an accurate list of sources, for example. |
c52f7c4 to
66bc371
Compare
|
MSVC build fixed. Ready for review now :) |
|
I'm still a concept NACK, and would rather we use Corys fix from #25008. |
66bc371 to
f17c358
Compare
Is this well-defined behaviour, or would we be relying on an optimisation that the user might disable (more likely with debug builds, I might note)? |
This change is a prerequisite for the following fix for `libbitcoinconsensus-0.dll` builds with `DEBUG=1`.
This change fixes `libbitcoinconsensus-0.dll` builds with `DEBUG=1`, and removes unneeded exported symbols from `libbitcoinconsensus.so`.
|
Updated f17c358 -> c28bb15 (pr24994.06 -> pr24994.07):
I failed to find supportive statements in the Autotools docs, only in other sources. |
|
The PR description has been updated. |
|
Concept ACK |
| @@ -937,12 +939,50 @@ endif # BUILD_BITCOIN_KERNEL_LIB | |||
| if BUILD_BITCOIN_LIBS | |||
| lib_LTLIBRARIES += $(LIBBITCOINCONSENSUS) | |||
|
|
|||
| noinst_LTLIBRARIES += libscriptvalidation.la | |||
| libscriptvalidation_la_SOURCES = | |||
There was a problem hiding this comment.
Might want to add this to doc/design/libraries.md
|
Concept ACK Guix hashes: x86: arm64: |
|
Could you be interesting in reviewing of this PR? |
|
Concept NACK. In general, I don't like this approach of refactoring code, and adding libraries, to workaround issues with build tooling, especially when its a problem that only exists for a single platform (let alone the DEBUG build, and ). I think this should just be closed in light of #27146 and the migration towards CMake. |
|
The PR description seems to say that this fixes a bug and that there is alternative fix in #25008. Is theuni@9612f5f the alternative fix? Neither of the two fixes seems to have an explanation of how they fix the problem. I also don't see a description of what the bug is other than "We're unable to build a I can see that this PR is doing a refactoring, and that fanquake doesn't like the refactoring. I don't see an explanation of why the refactoring is necessary, or why fanquake doesn't like it. I'm not sure if fanquake is mainly objecting to this particular refactoring, or doesn't like refactorings only addressing build problems. I also don't know what the circular dependency referenced in #24994 (comment) is, so maybe that is a key factor. This is all very hard to decipher. If I'm the only one who's confused, and everyone else understands how these fixes work, I guess that's fine. But if there is not mutual understanding of both approaches, it's not surprising there would be conflict trying to decide which one is better. |
@ryanofsky I don't like it because it's clearly the wrong solution to what is ultimately a build/tooling problem. The code is fine, and works perfectly fine when compiled on all other platforms. It doesn't work, when building a Windows DLL, and only when using So this is clearly a build system/tooling problem, and the the idea that we should refactor perfectly working code, introduce new libs etc, to fix a windows only issue, that only occurs when using certain preprocessing flags, seems completely incorrect. |
|
@fanquake that all makes sense at a high level, but without knowing the details of what exactly is causing the bug, I don't actually know that "the code is fine." If the code works on all platforms and configurations except one, it does strongly suggest that there is no problem with the code, so I see where you are coming from. But there is some reason this fix works, so it could be fixing a problem that is masked on other platforms, and it could be making improvements that would make the build less fragile and have other benefits. I think you right to push back though because it's not clear what exactly is wrong with the current code that causes it not to work on this one platform and configuration. |
I disagree with this NACK. @fanquake's push back is mostly focused on the Windows-related issue mentioned in the PR description. However, none alternatives are suggested to solve the main issue, i.e., #26698. Cross-posting my comment from #26698 (comment):
If the code of a shared library compiles and works, but a linker exports symbols, which are not a part of the library's API, this code is not fine. |
How does the above indicate a problem with our code? Wouldn't this point to the linker being the issue? Does the same happen with |
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36022 -- |
I'd like to help with this, but I just don't understand most of the discussion so far. I see that this fixes some kind of a bug on windows and is supposed to have other benefits mentioned in the PR description, but I don't see a clear description of the bug anywhere. Maybe someone can open an issue with a description of the bug and why it happens? The issue could link to whatever PRs are being proposed to fix it. |
My apologies about that.
I've added an explicit mentioning of the #26698 issue.
It is done in #26698. |
This PR has the following 4 benefits:
First benefit
This PR prunes all of unnecessary symbols being exported from
libbitcoinconsensus.so:For more details regarding this bug please refer to #26698.
Second benefit
It fixes
libbitcoinconsensusWindows builds withDEBUG=1.On master (833add0):
With this PR:
From
libstdc++docs:On the master branch, a linker adds to binaries every specified object file. And it fails, for example, for
bitcoin/src/util/strencodings.h
Lines 58 to 59 in f58c1f1
when building
libbitcoinconsensus-0.dllwhich in turnThird benefit
The resulted binaries got smaller because a linker only extracts from a convenience library those object files that are actually referenced in the code being linked (that could be easy to verify by comparing
nmoutputs):Fourth benefit
This solution does not introduce another Libtool hack as a competitive one does.
Guix builds on
x86_64:Fixes #19772.
Fixes #26698.