build: compile univalue as c++11 #12467
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Grr, there's a bug in the macro's handling of subdirs. I'll have to fix this up next week. |
|
Concept ACK.
There are certainly some rough edge cases, but in theory (no compiler bugs) this should be ok, right? Is there any official guideline about this? Doesn't building against system libraries have the same risks? You can never be sure what c++ standard they were compiled with, certainly not with compilers changing the default all the time. |
theuni
force-pushed
the
fix-subconfigure-args
branch
from
68b0fbc to
812e171
Compare
|
@laanwj I think we're probably ok here, but I have no idea what happens when differing abi's get linked in together. This could certainly be a problem if univalue was built as a shared lib though, as we could miss thrown exceptions due to differing type signatures. IIRC we actually had that issue at some point catching boost exceptions. |
|
This needs a rebase. |
theuni
force-pushed
the
fix-subconfigure-args
branch
from
812e171 to
b8fa047
Compare
Also don't set the value if it's already set. This allows the value to be updated then passed to a subconfigure, which would otherwise be reinitialized by the config.site
AX_SUBDIRS_CONFIGURE allows us to cleanly add/remove/filter what gets passed to subconfigures. This should be a no op.
Once we're certain that our CXX is compiling as c++11, pass it to univalue for use there as well. This avoids having a binary trying to conform to 2 abi's simultaneously.
theuni
force-pushed
the
fix-subconfigure-args
branch
from
b8fa047 to
8816f3d
Compare
|
Travis fails with |
|
@theuni ping |
|
@theuni Do you want to get this into 0.17.0? |
|
This hit an annoying snag and didn't end up working. Closing. |
theuni
added a commit
to theuni/bitcoin
that referenced
this pull request
Jul 15, 2021
This addresses issues like the one in bitcoin#12467, where some of our compiler flags end up being dropped during the subconfigure of Univalue. Specifically, we're still using the compiler-default c++ version rather than forcing c++11. We can drop the need subconfigure completely in favor of a tighter build integration, where the sources are listed separately from the build recipes, so that they may be included directly by upstream projects. This is similar to the way leveldb build integration works in Core. Upstream benefits of this approach include: Better caching (for ex. ccache and autoconf) No need for a slow subconfigure No more missing compile flags Compile only the objects needed There are no benefits to Univalue itself that I can think of. These changes should be a no-op there, and to downstreams as well until they take advantage of the new sources.mk. Once merged, This Core branch is ready-ish for PR, and takes advantage of the features added here. This also removes the option to use an external univalue to avoid similar ABI issues with mystery binaries.
theuni
added a commit
to theuni/bitcoin
that referenced
this pull request
Jul 15, 2021
Requires bitcoin-core/univalue-subtree#19 . This addresses issues like the one in bitcoin#12467, where some of our compiler flags end up being dropped during the subconfigure of Univalue. Specifically, we're still using the compiler-default c++ version rather than forcing c++11. We can drop the need subconfigure completely in favor of a tighter build integration, where the sources are listed separately from the build recipes, so that they may be included directly by upstream projects. This is similar to the way leveldb build integration works in Core. Upstream benefits of this approach include: Better caching (for ex. ccache and autoconf) No need for a slow subconfigure No more missing compile flags Compile only the objects needed There are no benefits to Univalue itself that I can think of. These changes should be a no-op there, and to downstreams as well until they take advantage of the new sources.mk. Once merged, This Core branch is ready-ish for PR, and takes advantage of the features added here. This also removes the option to use an external univalue to avoid similar ABI issues with mystery binaries.
theuni
added a commit
to theuni/bitcoin
that referenced
this pull request
Jul 15, 2021
Requires bitcoin-core/univalue-subtree#19 . This addresses issues like the one in bitcoin#12467, where some of our compiler flags end up being dropped during the subconfigure of Univalue. Specifically, we're still using the compiler-default c++ version rather than forcing c++17. We can drop the need subconfigure completely in favor of a tighter build integration, where the sources are listed separately from the build recipes, so that they may be included directly by upstream projects. This is similar to the way leveldb build integration works in Core. Core benefits of this approach include: - Better caching (for ex. ccache and autoconf) - No need for a slow subconfigure - Faster autoconf - No more missing compile flags - Compile only the objects needed There are no benefits to Univalue itself that I can think of. These changes should be a no-op there, and to downstreams as well until they take advantage of the new sources.mk. This also removes the option to use an external univalue to avoid similar ABI issues with mystery binaries.
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Aug 6, 2021
Requires bitcoin-core/univalue-subtree#19 . This addresses issues like the one in bitcoin#12467, where some of our compiler flags end up being dropped during the subconfigure of Univalue. Specifically, we're still using the compiler-default c++ version rather than forcing c++17. We can drop the need subconfigure completely in favor of a tighter build integration, where the sources are listed separately from the build recipes, so that they may be included directly by upstream projects. This is similar to the way leveldb build integration works in Core. Core benefits of this approach include: - Better caching (for ex. ccache and autoconf) - No need for a slow subconfigure - Faster autoconf - No more missing compile flags - Compile only the objects needed There are no benefits to Univalue itself that I can think of. These changes should be a no-op there, and to downstreams as well until they take advantage of the new sources.mk. This also removes the option to use an external univalue to avoid similar ABI issues with mystery binaries.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reported by irc user (and maybe github user?) @esotericnonsense.
Building bitcoin as c++11 and univalue as c++03/c++14 (depending on compiler version) is pretty scary. It's surprising that this hasn't caused any issues yet.
I'm afraid the fix is a big pile of autotools nonsense, but the changes are pretty straightforward:
The good news is that this makes it much easier to tweak subconfigure args in the future.