build: Do not include server symbols in wallet #19331
Conversation
fa024ae to
faa4d7c
Compare
|
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. |
hebasto
left a comment
There was a problem hiding this comment.
Approach ACK, tested with GCC 7.5.0 -- works as expected.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures.
Trying to get the complete understanding of linker failures. Could you elaborate or point out further reading the reasons of linker failures.
From my current understanding, if a symbol is exported, it could be linked to, no?
|
faa4d7c to
fa30d36
Compare
|
Removed unused includes as requested by @hebasto |
hebasto
left a comment
There was a problem hiding this comment.
ACK fa30d36c5a94cd2f71185162b577e05ab9adf717, tested on Linux Mint 19.3 (x86_64) with both GCC 7.5.0 and Clang 6.0.0, using -O0 compiler option.
|
Concept ACK Thanks for fixing! This |
|
This looks reasonable. It's important to be consistent about which libraries are allowed to use which symbols. Concept ACK. |
fa30d36 to
fa1264a
Compare
This reverts commit b83cc0f.
ui_interface is in libbitcoin_server and can not be included in the wallet because the wallet does not link with server symbols.
fa1264a to
faabd97
Compare
-BEGIN VERIFY SCRIPT- # Move files git mv src/ui_interface.h src/node/ui_interface.h git mv src/ui_interface.cpp src/node/ui_interface.cpp sed -i -e 's/BITCOIN_UI_INTERFACE_H/BITCOIN_NODE_UI_INTERFACE_H/g' src/node/ui_interface.h # Adjust includes and makefile sed -i -e 's|ui_interface|node/ui_interface|g' $(git grep -l ui_interface) # Sort includes git diff -U0 | clang-format-diff -p1 -i -v -END VERIFY SCRIPT-
faabd97 to
faca730
Compare
|
ACK faca730 |
|
@hebasto Mind to re-ACK? |
|
ACK faca730 |
Summary: ``` FatalError has been copied from AbortNode, so the two should use the same style to avoid confusion. Follow-up to #18927 ``` Backport of core [[bitcoin/bitcoin#19295 | PR19295]], [[bitcoin/bitcoin#19309 | PR19309]] and [[bitcoin/bitcoin#19331 | PR19331]]. [[bitcoin/bitcoin#19295 | PR19295]] is the original intent [[bitcoin/bitcoin#19309 | PR19309]] is a hacky bug fix [[bitcoin/bitcoin#19331 | PR19331]] reverts [[bitcoin/bitcoin#19309 | PR19309]] and fixes [[bitcoin/bitcoin#19295 | PR19295]]. In the end the relevant commits are: bitcoin/bitcoin@fa02b47 bitcoin/bitcoin@fac96e6 bitcoin/bitcoin@fa72ca6 bitcoin/bitcoin@cccc278 Depends on D8649. Test Plan: With debug: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8650
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless
-O0) because our compilers were smart enough to strip unused symbols.Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.