build: tighter Univalue integration, remove `--with-system-univalue`

fanquake commented at 3:14 am on August 6, 2021: member

This PR more tightly integrates building Univalue into our build system. This follows the same approach we use for LevelDB, (Makefile.leveldb.include), and CRC32C (Makefile.crc32c.include), and will be the same approach we use for minisketch; see #23114.

This approach yields a number of benefits, including:

Faster configuration due to one less subconfigure being run during ./configure i.e 22s with this PR vs 26s
Faster autoconf i.e 13s with this PR vs 17s
Improved caching
No more issues with compiler flags i.e #12467
More direct control means we can build exactly the objects we want

There might be one argument against making this change, which is that builders should have the option to use “proper shared/system libraries”. However, I think that falls down for a few reasons. The first being that we already don’t support building with a number of system libraries (secp256k1, leveldb, crc32c); some for good reason. Univalue is really the odd one out at the moment.

Note that the only fork of Core I’m aware of, that actively patches in support for using system libs, also explicitly marks them as “DANGEROUS” and “NOT SUPPORTED”. So it would seem they exist more to satisfy a distro requirement, as opposed to something that anyone should, or would actually use in practice.

PRs like #22412 highlight the “issue” with us operating with our own Univalue fork, where we actively fix bugs, and make improvements, when upstream (https://github.com/jgarzik/univalue) may not be taking those improvements, and by all accounts, is not currently actively maintained. Bitcoin Core should not be hamstrung into not being able to fix bugs in a library, and/or have to litter our source with “workarounds”, i.e #22412, for bugs we’ve already fixed, based on the fact that an upstream project is not actively being maintained. Allowing builders to use system libs is really only exacerbating this problem, with little benefit to our project. Bitcoin Core is not quite like your average piece of distro packaged software.

There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.

fanquake added the label Brainstorming on Aug 6, 2021

fanquake added the label Build system on Aug 6, 2021

DrahtBot commented at 9:31 am on August 6, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23114 (Add minisketch subtree and integrate into build/test by fanquake)
#23049 ([WIP] net: implement a StratumV2 Template Provider in core by Fi3)
#20201 (build: pkg-config related cleanup by hebasto)

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.

jnewbery commented at 9:47 am on August 6, 2021: member

Strong concept ACK!

As well as the technical benefits listed in the PR description, a strong reason to go in this direction is to lower our maintenance and testing burden. Being able to build with a system univalue library really doesn’t provide any benefit to users, but results in issues like #22412 which suck up reviewer and maintainer time. Maintaining support for different versions of univalue also expands the matrix of different build/configuration combinations which we really should (but don’t) test if we claim support.

ryanofsky commented at 10:22 am on August 6, 2021: member

I know that the forking process already began before this PR, and I think that forking in open source can be very good. But I also think that when you fork, it is better practically and ethically to rename the fork. In the case of a library, this doesn’t need to involve renaming functions and classes in the library, but the forked library should at least have different library filenames (libunivalue.a) and include filenames (in this case just univalue.h) so it is not a nightmare if downstream projects have code requiring both libraries as dependencies. Also it would be good to rename the source directory src/univalue/ and update the readme documentation src/univalue/README.md to explicity say the library has been forked, the forks are incompatible, problems and discussions about the fork should be directed at the new issue tracker / mailing list / irc channel / whatever, not at the original project.

The idea is that by renaming a directory and two files, you can make things less ambiguous, confusing, and potentially painful for downstream developers, while also being kinder to the author of the original library and letting the name he chose for his project refer unambiguously to his own project instead of a collection of forks.

As for what name to substitute, for an internal fork it could be nonsense like xyzlkq for all it matters, or something like unibtc to be coherent, or something ike jsonvalue to be descriptive, or something like univalue– to be cheeky, whatever.

Please ignore this suggestion if it’s too cumbersome. Just wanted to describe a possible way to make forking less confusing and contentious.

Rspigler commented at 10:24 pm on August 6, 2021: contributor

Concept ACK. More control over our build is better.

practicalswift commented at 6:53 pm on August 15, 2021: contributor

Concept ACK for the reasons @jnewbery gave in #22646 (comment)

laanwj commented at 1:00 pm on August 16, 2021: member

Concept ACK.

Please ignore this suggestion if it’s too cumbersome. Just wanted to de scribe a possible way to make forking less confusing and contentious.

Not sure about this. As said in the OP, we have done a similar thing for the leveldb and crc32c libraries with regard to maintaining our own fork without renaming them. The subsequent repositories have a mention in their github description that they are not the upstream project, this may be enough. To be clear, it is not the intent that other projects use our fork of the library, it is for our subtree use only. Nor is it intended for them to be packaged (e.g. in Linux distributions). Renaming the project might give a false signal that it’s an independent project for general use, somehow.

(Also AFAIK we do still intend to follow upstream changes, and contributing changes upstream, even though they only get merged intermittently if at all, I think it’s better regarded as a patch stack on top than a completely separate project)

MarcoFalke commented at 6:47 am on August 17, 2021: member

Maybe the GitHub projects could be renamed to bitcoin-core/${upstream-name}-subtree to clarify this is a repo only used for bitcoin-core internal subtrees? I agree that renaming the project name itself (in the code) is too much and might even send the wrong impression that this is maintained for someone other than bitcoin-core itself.

luke-jr commented at 12:23 pm on September 2, 2021: member

Concept NACK.

UniValue is not comparable to secp256k1 and LevelDB - the latter are used in consensus-critical code, and so it’s important to be careful about which bugs get fixed in them. Using the system UniValue isn’t and shouldn’t not be dangerous nor unsupported.

There is basically no reason to embed a copy of UniValue at all. If upstream support is lacking, we should just release our fork as a proper fork of the library or not require things upstream has decided not to do. The embedded copy should ideally be removed, and only ever build against the system install.

Other disagree with removing the embedded copy, however. Keeping support for both system and an embedded copy was a reasonable compromise. If we’re not going to support both, however, the direction to go is removing the embedded copy, not dropping support for the correct configuration (system library).

MarcoFalke commented at 1:04 pm on September 2, 2021: member

Concept ACK.

Surely, in a world with enough maintenance resources, univalue could be released and integrated as a separate library. As there are evidently neither resources to maintain it as a separate library, nor maintain the integration of an external univalue, this seems like a good move.

luke-jr commented at 1:21 pm on September 2, 2021: member

It’s literally less effort to maintain it properly.

All the work to integrate it is already done. The bugs recently introduced, have also been fixed aside from gatekeepers blocking the fixes from being merged.

laanwj commented at 6:46 pm on September 16, 2021: member

Maybe the GitHub projects could be renamed to bitcoin-core/${upstream-name}-subtree to clarify this is a repo only used for bitcoin-core internal subtrees?

Sure, sounds good to me.

Edit: ok, i’ve renamed:

bitcoin-core/univalue → bitcoin-core/univalue-subtree
bitcoin-core/leveldb → bitcoin-core/leveldb-subtree
bitcoin-core/crc32c → bitcoin-core/crc32c-subtree

The “Subtrees” section in the developer notes likely needs some updates after this.

fanquake referenced this in commit 69a66dcd0d on Sep 30, 2021

jnewbery commented at 8:26 am on October 4, 2021: member

https://github.com/bitcoin-core/univalue-subtree/pull/19 is merged. Does this need rebasing?

fanquake renamed this:
~~[POC] Tighter Univalue integration, remove `--with-system-univalue`~~
build: tighter Univalue integration, remove `--with-system-univalue`
on Oct 4, 2021

fanquake force-pushed on Oct 4, 2021

fanquake commented at 11:36 am on October 4, 2021: member

bitcoin-core/univalue-subtree#19 is merged. Does this need rebasing?

I’ve rebased on master and dropped the cherry-picks in favour of a subtree update. I’ve also fixed up the integration commit so that make dist is working properly, and made some other minor changes. Also updated the PR description.

fanquake added the label DrahtBot Guix build requested on Oct 4, 2021

fanquake force-pushed on Oct 4, 2021

MarcoFalke commented at 12:18 pm on October 4, 2021: member

The changes without the last commit will break building with git-bisect? If yes, the changes should probably be squashed into the previous merge commit (git reset --soft HEAD~ && git commit --amend).

fanquake force-pushed on Oct 4, 2021

fanquake commented at 11:34 pm on October 4, 2021: member

Pushed a fix for the CI failures. The fail*.json sources were missing from the dist tarball, which was causing unitester to fail.

The changes without the last commit will break building with git-bisect?

They shouldn’t do. After the subtree pull, univalue is still buildable using autotools. We atomically swap from building univalue using autotools, to integrating univalue into our build in the last commit.

fanquake force-pushed on Oct 5, 2021

fanquake commented at 1:06 am on October 5, 2021: member

Similar to what we are doing with the minisketch integration, pushed a change to skip building the univalue tests if fuzzing is enabled.

fanquake commented at 2:23 am on October 5, 2021: member

At least one issue to fix from a sanitizer CI:

 0********* Start testing of URITests *********
 1Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.3.0), ubuntu 21.04
 2PASS   : URITests::initTestCase()
 3PASS   : URITests::uriTests()
 4PASS   : URITests::cleanupTestCase()
 5Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1ms
 6********* Finished testing of URITests *********
 7********* Start testing of RPCNestedTests *********
 8Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.3.0), ubuntu 21.04
 9PASS   : RPCNestedTests::initTestCase()
10univalue/lib/univalue_read.cpp:292:13: runtime error: implicit conversion from type 'int' of value -5 (32-bit, signed) to type 'unsigned int' changed the value to 4294967291 (32-bit, unsigned)
11    [#0](/bitcoin-bitcoin/0/) 0x55978052c2bf in UniValue::read(char const*, unsigned long) univalue/lib/univalue_read.cpp:292:13
12    [#1](/bitcoin-bitcoin/1/) 0x55977fcabbbd in UniValue::read(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ./univalue/include/univalue.h:153:16
13    [#2](/bitcoin-bitcoin/2/) 0x5597804a66c6 in ParseNonRFCJSONValue(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) rpc/client.cpp:234:15
14    [#3](/bitcoin-bitcoin/3/) 0x5597804a6cc8 in RPCConvertValues(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) rpc/client.cpp:252:30
15    [#4](/bitcoin-bitcoin/4/) 0x55977f0920c0 in RPCConsole::RPCParseCommandLine(interfaces::Node*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, WalletModel const*) qt/rpcconsole.cpp:328:47
16    [#5](/bitcoin-bitcoin/5/) 0x55977ee8a3dc in RPCConsole::RPCExecuteCommandLine(interfaces::Node&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, WalletModel const*) ./qt/rpcconsole.h:48:16
17    [#6](/bitcoin-bitcoin/6/) 0x55977ee8a3dc in RPCNestedTests::rpcNestedTests() qt/test/rpcnestedtests.cpp:62:5
18    [#7](/bitcoin-bitcoin/7/) 0x55977ef214f2 in RPCNestedTests::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) qt/test/moc_rpcnestedtests.cpp:72:21
19    [#8](/bitcoin-bitcoin/8/) 0x7f83e3593d9a in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2c3d9a)
20    [#9](/bitcoin-bitcoin/9/) 0x7f83e3294552  (/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1c552)
21    [#10](/bitcoin-bitcoin/10/) 0x7f83e3294ffb  (/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1cffb)
22    [#11](/bitcoin-bitcoin/11/) 0x7f83e32955e3  (/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1d5e3)
23    [#12](/bitcoin-bitcoin/12/) 0x7f83e3295b33 in QTest::qRun() (/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1db33)
24    [#13](/bitcoin-bitcoin/13/) 0x7f83e3295f2f in QTest::qExec(QObject*, int, char**) (/lib/x86_64-linux-gnu/libQt5Test.so.5+0x1df2f)
25    [#14](/bitcoin-bitcoin/14/) 0x55977eea9a74 in main qt/test/test_main.cpp:92:9
26    [#15](/bitcoin-bitcoin/15/) 0x7f83e1a32564 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28564)
27    [#16](/bitcoin-bitcoin/16/) 0x55977edc350d in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x18ba50d)
28
29SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change univalue/lib/univalue_read.cpp:292:13 in 
30FAIL qt/test/test_bitcoin-qt (exit status: 1)

DrahtBot removed the label DrahtBot Guix build requested on Oct 5, 2021

MarcoFalke commented at 10:21 am on October 6, 2021: member

You can fix the CI error with this diff:

 0diff --git a/src/univalue/lib/univalue_read.cpp b/src/univalue/lib/univalue_read.cpp
 1index f8a5d4d76f..d68d263858 100644
 2--- a/src/univalue/lib/univalue_read.cpp
 3+++ b/src/univalue/lib/univalue_read.cpp
 4@@ -244,7 +244,7 @@ enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
 5     }
 6 }
 7 
 8-enum expect_bits {
 9+enum expect_bits : unsigned {
10     EXP_OBJ_NAME = (1U << 0),
11     EXP_COLON = (1U << 1),
12     EXP_ARR_VALUE = (1U << 2),

See also https://github.com/bitcoin/bitcoin/pull/22232/files#diff-da9625bc8e6928c0ec3239921e739c32b9d0abdfbca9773c550a3cdd818230b8 which did the same thing for another enum.

MarcoFalke deleted a comment on Oct 6, 2021

MarcoFalke added the label DrahtBot Guix build requested on Oct 7, 2021

MarcoFalke commented at 12:49 pm on October 7, 2021: member

Aw sorry, looks like I missed some more:

 0diff --git a/src/univalue/lib/univalue_read.cpp b/src/univalue/lib/univalue_read.cpp
 1index d68d263858..be39bfe57a 100644
 2--- a/src/univalue/lib/univalue_read.cpp
 3+++ b/src/univalue/lib/univalue_read.cpp
 4@@ -227,7 +227,7 @@ enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
 5             }
 6 
 7             else {
 8-                writer.push_back(*raw);
 9+                writer.push_back(static_cast<unsigned char>(*raw));
10                 raw++;
11             }
12         }
13diff --git a/src/univalue/lib/univalue_write.cpp b/src/univalue/lib/univalue_write.cpp
14index db039fcb00..3a2c580c7f 100644
15--- a/src/univalue/lib/univalue_write.cpp
16+++ b/src/univalue/lib/univalue_write.cpp
17@@ -13,13 +13,13 @@ static std::string json_escape(const std::string& inS)
18     outS.reserve(inS.size() * 2);
19 
20     for (unsigned int i = 0; i < inS.size(); i++) {
21-        unsigned char ch = inS[i];
22+        unsigned char ch = static_cast<unsigned char>(inS[i]);
23         const char *escStr = escapes[ch];
24 
25         if (escStr)
26             outS += escStr;
27         else
28-            outS += ch;
29+            outS += static_cast<char>(ch);
30     }
31 
32     return outS;

fanquake force-pushed on Oct 8, 2021

MarcoFalke commented at 7:30 am on October 8, 2021: member

all green now :green_apple: :green_book:

fanquake referenced this in commit 7d3b9756c1 on Oct 8, 2021

DrahtBot removed the label DrahtBot Guix build requested on Oct 10, 2021

fanquake referenced this in commit b680cdd087 on Oct 10, 2021

fanquake referenced this in commit 24ba142c15 on Oct 11, 2021

fanquake referenced this in commit 135254331e on Oct 11, 2021

fanquake referenced this in commit a44caf65fe on Oct 11, 2021

Squashed 'src/univalue/' changes from 98fadc0909..a44caf65fe

a44caf65fe Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331e Import fixes for sanitizer reported issues
d5fb86940e refactor: use c++11 range based for loop in checkObject
ff9c379304 refactor: Use nullptr (c++11) instead of NULL
08a99754d5 build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce7 Merge bitcoin-core/univalue#29: ci: travis -> cirrus
808d487292 ci: travis -> cirrus
c390ac375f Merge bitcoin-core/univalue#19: Split sources for easier buildsystem integration
4a5b0a1c65 build: Move source entries out to sources.mk
6c7d94b33c build: cleanup wonky gen usage
a222637c6d Merge #23: Merge changes from jgarzik/univalue@1ae6a23
f77d0f718d Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
1ae6a231a0 Merge pull request #57 from MarcoFalke/test_fix
92bdd11f0b univalue_write: remove unneeded sstream.h include
ffb621c130 Merge pull request #56 from drodil/remove_sstream_header
f33acf9fe8 Merge commit '7890db9~' into HEAD
66e0adec4d Remove unnecessary sstream header from univalue.h
88967f6586 Version 1.0.4
1dc113dbef Merge pull request #50 from luke-jr/pushKV_bool
72392fb227 [tests] test pushKV for boolean values
c23132bcf4 Pushing boolean value to univalue correctly
81faab26a1 Merge pull request #48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634ef24 Update URLs to MIT license.
88ab64f6b5 Merge pull request #46 from jasonbcox/master
35ed96da31 Merge pull request #44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226290 Merge pull request #45 from MarcoFalke/Mf1710-univalue-revert-test

git-subtree-dir: src/univalue
git-subtree-split: a44caf65fe55b9dd8ddb08f04c0f70409efd53b3

9b49ed656f

Update univalue subtree to latest upstream 3043193675

Integrate univalue into our buildsystem

This addresses issues like the one in #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.

Co-authored-by: fanquake <fanquake@gmail.com>

0f95247246

fanquake force-pushed on Oct 11, 2021

fanquake removed the label Brainstorming on Oct 11, 2021

fanquake added the label DrahtBot Guix build requested on Oct 11, 2021

fanquake commented at 10:01 pm on October 11, 2021: member

Guix build:

 0bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 11ede0a3901a0cbb7f226f83e1fda345886fe3255abd9a5a9113f8da222adb51d  guix-build-0f9524724634/output/aarch64-linux-gnu/SHA256SUMS.part
 2ce62c2d422394b6fd35522aa247aeb8bd5938df1709aa387316fadf71c2feec0  guix-build-0f9524724634/output/aarch64-linux-gnu/bitcoin-0f9524724634-aarch64-linux-gnu-debug.tar.gz
 3b7e98167962f40abe1d7d50e20d4318e8bf930f9951c381ef74a7327f8493e30  guix-build-0f9524724634/output/aarch64-linux-gnu/bitcoin-0f9524724634-aarch64-linux-gnu.tar.gz
 49d2fb31508eb0c9dd30a1d680d85de16f3a6898fa6641f4c7dccff44fec02b4b  guix-build-0f9524724634/output/arm-linux-gnueabihf/SHA256SUMS.part
 5e879bc679620120adbcb0a934e0db42af146223f79251c841546c0651037ebf7  guix-build-0f9524724634/output/arm-linux-gnueabihf/bitcoin-0f9524724634-arm-linux-gnueabihf-debug.tar.gz
 6d28fa83e93e523940974a9f01fe77db1ddaba114c0fdff6e6e1f64d3da9528c1  guix-build-0f9524724634/output/arm-linux-gnueabihf/bitcoin-0f9524724634-arm-linux-gnueabihf.tar.gz
 7e4ef169a3253afadc627837e70fa477cb4643550282f63ba517263d77b838ad6  guix-build-0f9524724634/output/dist-archive/bitcoin-0f9524724634.tar.gz
 8f2136388285e26750e7a17f3d7f728709fd559f56e9ff00494a7c1675bb69605  guix-build-0f9524724634/output/powerpc64-linux-gnu/SHA256SUMS.part
 9d6b2b13472d840431de2621e9f827378ef62ba848faf5edee14772d008254643  guix-build-0f9524724634/output/powerpc64-linux-gnu/bitcoin-0f9524724634-powerpc64-linux-gnu-debug.tar.gz
108d7f4b5796d56648cc85c2f1f6c61266a99fa48acf90c2c7b63fec04dfc4ae84  guix-build-0f9524724634/output/powerpc64-linux-gnu/bitcoin-0f9524724634-powerpc64-linux-gnu.tar.gz
11af7d3f21bbf6fc0d5d2e74eed25410a250f0c4eade5cc68f6b3e40e363eeba59  guix-build-0f9524724634/output/powerpc64le-linux-gnu/SHA256SUMS.part
12f7926a69b2c2b4774a924f87c6d8bf6704fe3082ab46fc5b8b26bf52d70a180c  guix-build-0f9524724634/output/powerpc64le-linux-gnu/bitcoin-0f9524724634-powerpc64le-linux-gnu-debug.tar.gz
1309192bd4e861a707e72dabb6aad7637d60a8640f87de25d1cfc36f9ff5636f8e  guix-build-0f9524724634/output/powerpc64le-linux-gnu/bitcoin-0f9524724634-powerpc64le-linux-gnu.tar.gz
14e934e54ae3766244a0f55a4f759e616a1a7deee757890fcc2a494675ad17cbd5  guix-build-0f9524724634/output/riscv64-linux-gnu/SHA256SUMS.part
15d35764811b4bfd26b1862a0fb7d045181563b5f2d7bba569a6f27b491869c0a7  guix-build-0f9524724634/output/riscv64-linux-gnu/bitcoin-0f9524724634-riscv64-linux-gnu-debug.tar.gz
168a9b125c670742c306cd5e30926f69432ba302767c14aef90a54e8598774653f  guix-build-0f9524724634/output/riscv64-linux-gnu/bitcoin-0f9524724634-riscv64-linux-gnu.tar.gz
1796d863d9e473c3455d26246b3731fd71a497cf96feb95d43728dd05686059372  guix-build-0f9524724634/output/x86_64-apple-darwin19/SHA256SUMS.part
186b5ab0a63602996b22429f01ce9c8ab5b2102a08484fa80a9f42f4a951579caf  guix-build-0f9524724634/output/x86_64-apple-darwin19/bitcoin-0f9524724634-osx-unsigned.dmg
19be865c2a64f1e1ef7641c8f8c5991da8ba38345f3f9d4d220084c39bbd03fedd  guix-build-0f9524724634/output/x86_64-apple-darwin19/bitcoin-0f9524724634-osx-unsigned.tar.gz
205d3395125c2a2fd16d98e6d304927f68e3bac24f5e70594eec9da540c05742c4  guix-build-0f9524724634/output/x86_64-apple-darwin19/bitcoin-0f9524724634-osx64.tar.gz
2143fd87227d553f91d54abe83422d07f0c03b39c0c2b0b4569f1bd2bfb2e48458  guix-build-0f9524724634/output/x86_64-linux-gnu/SHA256SUMS.part
22c28fd32e286c19cc98bbe7c8256b7b7088b1eb1d2e1c5f68b23c52339787e2cd  guix-build-0f9524724634/output/x86_64-linux-gnu/bitcoin-0f9524724634-x86_64-linux-gnu-debug.tar.gz
23606ae78d717a56e0d1bcb0a6d3c9bb5b46ce483364d135ae37bd1510b6fb6916  guix-build-0f9524724634/output/x86_64-linux-gnu/bitcoin-0f9524724634-x86_64-linux-gnu.tar.gz
24745a98dbe3f9940086de8e5968f3f3f40eb82cc01b7d10c9da190f6481694ee6  guix-build-0f9524724634/output/x86_64-w64-mingw32/SHA256SUMS.part
25de34cdbfd95aef0e6c6151fcd066d97732aae77b89b1a33752f3bf7e4982602a  guix-build-0f9524724634/output/x86_64-w64-mingw32/bitcoin-0f9524724634-win-unsigned.tar.gz
26d74eb253616fccbc2fcd98b52ba4d2e6c8be2177f7cf594d1410523d0ca3876b  guix-build-0f9524724634/output/x86_64-w64-mingw32/bitcoin-0f9524724634-win64-debug.zip
27a2fb734b61852ece5f11bed2e0f1582ad773ae5e99e0fd0a737e1225a2c9e314  guix-build-0f9524724634/output/x86_64-w64-mingw32/bitcoin-0f9524724634-win64-setup-unsigned.exe
2814bf9de8564e99fb4ec544b452b0938cc07575d9c9f8d29924337497f3222b23  guix-build-0f9524724634/output/x86_64-w64-mingw32/bitcoin-0f9524724634-win64.zip

MarcoFalke deleted a comment on Oct 12, 2021

in src/Makefile.univalue.include:5 in 0f95247246

0@@ -0,0 +1,6 @@
1+include univalue/sources.mk
2+
3+LIBUNIVALUE = libunivalue.la
4+noinst_LTLIBRARIES += $(LIBUNIVALUE)
5+libunivalue_la_SOURCES = $(UNIVALUE_LIB_SOURCES_INT) $(UNIVALUE_DIST_HEADERS_INT) $(UNIVALUE_LIB_HEADERS_INT) $(UNIVALUE_TEST_FILES_INT)

dongcarl commented at 2:44 pm on October 12, 2021:

Would it be appropriate to split this into other Automake primaries like DATA, HEADERS, etc?

theuni commented at 4:30 pm on October 19, 2021:

Wouldn’t hurt, but it doesn’t matter a ton for now since it’s all noinst_.

I suspect we will end up needing to make a distinction between headers and source files when we get to c++20 and modules, though.

fanquake commented at 2:58 am on October 20, 2021:

I’m going to save this for a followup.

dongcarl commented at 2:47 pm on October 12, 2021: member

ACK 0f95247246 less my comment above, always nice to have an include-able sources.mk which makes integration easier.

DrahtBot commented at 11:11 pm on October 17, 2021: member

Guix builds

File	commit 6419bdfeb130b20ccfed229d9ba7eca7f385d036(master)	commit adc92847897c09fa661948fc76609cddbb8e752e(master and this pull)
SHA256SUMS.part	`57430d4cfea534e7...`	`90eacc0a4d12e195...`
*-aarch64-linux-gnu-debug.tar.gz	`9ac5c89fbf2e958a...`	`0c7a67a6165fd738...`
*-aarch64-linux-gnu.tar.gz	`fb6ed9d3e3a06da1...`	`0b7e9c4dc94df909...`
*-arm-linux-gnueabihf-debug.tar.gz	`bcfe755361d4f4a0...`	`1d91aea5d456f9bc...`
*-arm-linux-gnueabihf.tar.gz	`7ae3be5da7e23338...`	`d0330582b15eb013...`
*-osx-unsigned.dmg	`e52e0699c7d05349...`	`e85d267e1ff01bad...`
*-osx-unsigned.tar.gz	`209f2f2ac192db09...`	`d130e76ed4560675...`
*-osx64.tar.gz	`43cc4669c131c052...`	`3ec532ea9fde0a4b...`
*-powerpc64-linux-gnu-debug.tar.gz	`31273cba71ae7a63...`	`b3a0128e22d8bacc...`
*-powerpc64-linux-gnu.tar.gz	`8459fb85502d8ca1...`	`bcbdfc167eb9d0e1...`
*-powerpc64le-linux-gnu-debug.tar.gz	`b15379f1d540d808...`	`542bee347777c6a6...`
*-powerpc64le-linux-gnu.tar.gz	`1f8bc012eb1c68ac...`	`9c375c743824b5e1...`
*-riscv64-linux-gnu-debug.tar.gz	`13b101bd639bbcb5...`	`265f85d5ab827cc6...`
*-riscv64-linux-gnu.tar.gz	`48d281ceade35519...`	`189f1f62af262a96...`
*-win-unsigned.tar.gz	`ef39a755917cf4a8...`	`36e5cd05ece06895...`
*-win64-debug.zip	`1a513a2f3564f9d7...`	`0e3d1088b6840df3...`
*-win64-setup-unsigned.exe	`2da290f0d9af8e78...`	`e9b1b65c0a143b3f...`
*-win64.zip	`7405c69d9e088c0a...`	`c7055eff4e4abd34...`
*-x86_64-linux-gnu-debug.tar.gz	`67c96b9fe3f5c21e...`	`4ff19d3d73c6fe7e...`
*-x86_64-linux-gnu.tar.gz	`bfaa676a253b1279...`	`91ab2e11b97f90dd...`
*.tar.gz	`9362f69e52311e85...`	`aba92a8bd6ece6ca...`
guix_build.log	`0cd4e97f279ba8fb...`	`bd39fcce35f4505e...`
guix_build.log.diff		`e1226a5b02123413...`

DrahtBot removed the label DrahtBot Guix build requested on Oct 17, 2021

theuni approved

theuni commented at 4:38 pm on October 19, 2021: member

ACK 0f95247246344510c9a51810c14c633abb382e95. Thanks fanquake for keeping this going.

There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.

After some discussion in #secp256k1, this probably isn’t going to happen. It would require us to essentially copy/paste secp256k1’s configure since it deals with c as opposed to c++. I proposed wrapping secp256k1 in a .cpp, but that idea was NACK’d quickly by the upstream maintainers. So I think that subconfigure will probably stay as-is.

fanquake commented at 3:00 am on October 20, 2021: member

I’m going to go ahead and merge this. In regards to the comments earlier in this PR, we’ve done some work to make it more clear that our fork is an internal (for our use only) subtree, by renaming the repository, and I’ve also opened a PR to adjust the README to mention that our API has diverged from upstream: https://github.com/bitcoin-core/univalue-subtree/pull/30.

After some discussion in #secp256k1, this probably isn’t going to happen.

That is slightly disappointing, but at least now we’ll be down to only a single extra configure invocation.

Edit: @theuni note that I modified your comment to remove my @ mention so that it wouldn’t be included in the merge message.

fanquake merged this on Oct 20, 2021

fanquake closed this on Oct 20, 2021

fanquake deleted the branch on Oct 20, 2021

sidhujag referenced this in commit 4ded13d83f on Oct 20, 2021

jnewbery commented at 11:37 am on October 20, 2021: member

post-merge ACK 0f95247246344510c9a51810c14c633abb382e95

I’ve tested the changes and confirmed that the subconfigure isn’t run (and that it’s slightly faster). I’m not an autotools expert so can only lightly review the diff, but it all looks reasonable to me.

fanquake referenced this in commit 78e36700a0 on Nov 8, 2021

fanquake referenced this in commit ddc90293c1 on Nov 8, 2021

sidhujag referenced this in commit 1fd96f2bc8 on Nov 8, 2021

hebasto commented at 9:15 am on November 21, 2021: member

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

0/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x890): undefined reference to `__imp_pthread_mutex_lock'
1/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x8b0): undefined reference to `__imp_pthread_mutex_unlock'

during linking univalue/test/object.exe, univalue/test/unitester.exe and univalue/test/no_nul.exe.

fanquake commented at 9:19 am on November 21, 2021: member

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

This is the same as #19772.

hebasto commented at 2:27 pm on November 27, 2021: member

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

This is the same as #19772.

The fix has been suggested on #23612.

kittywhiskers referenced this in commit bf8b3ec8b2 on May 1, 2022

PastaPastaPasta referenced this in commit b47d4843bf on May 2, 2022

martinus referenced this in commit 2c8d661137 on May 9, 2022

MarcoFalke referenced this in commit 8035b5c80d on Jun 16, 2022

dekm referenced this in commit c61f8f651d on Oct 27, 2022

DrahtBot locked this on Nov 27, 2022

build: tighter Univalue integration, remove --with-system-univalue #22646

Conflicts

Guix builds

build: tighter Univalue integration, remove `--with-system-univalue` #22646