concept ACK, of course :)

Please use --squash when modifying subtrees. It avoids the confusing incorporation of the other project's commits in our history, and allows the contrib/devtools/git-subtree-check.sh tool to verify that the claimed included commit actually has the intended contents.

@sipa: done.

Better, but this should really be an external shared library... Concept ACK

Is there any modern package management solution for C++?

pkg-config works great.

Wouldn't dynamic library and or pkg-config require broad support in various distributions over their package management?

Personally I would recommend static linking of the JSON stack. It's a critical library and an a hacked JSON stack could lead to coin stealing.

@jonasschnelli It's not a consensus-critical library, which is the only exception where static linking makes sense. A hacked glibc, or even any other software on the PC can lead to coin stealing - there is nothing special about the JSON library. (obviously the gitian binaries and depends system would be used to continue to static link regardless of proper dependency handling)

@luke-jr: ok, fair enough.

@sipa: Recently we have coupled UniValue with utilstrencodings.cpp/h (see c023092#diff-0f1b401041a14398229cf7e31b6db7eeR13).
To re-acchive independence from other sources, i have added the depending numparse functions in UniValue (see jgarzik/univalue@d0ec6ad and jgarzik/univalue@4b288b3). Not sure if this was a good step. I think copy and namespace the numparse function to UniValue could have made more sense. But also not sure if it would be a good idea either (duplicate code in bitcoin).

For bitcoin-core, we compile univalue without the number parse functions because we have them already over utilstrencodings.cpp/h. This is why univalue gets compiled with --with-numfunc=no.
But without the number parse functions, univalue itself cannot run the tests because the number parse functions are essential.
Maybe there is a way to tell the UniValue subtrees make check to link with utilstrencodings.o. But this would somehow break the independence of the subtree.

FWIW, the numberparse code is very nicely C/C++ standard and not tied to Univalue or bitcoin at all.

For the univalue tests, we could create an optional libtest for this case, if no better solution appears.

@sipa It's not a dependency so much as a commonality. univalue and bitcoin both use ParseInt32

test/util_tests.cpp uses ParseInt32, ParseInt64 and ParseDouble, but that is a circular unit test dependency to be ignored.

UniValue doesn't depend on any code. It now just have an option (--with-numfunc=no) to avoid linking numfunc.o (consuming application of libunivalue can provide own number parse functions [3 functions as Jeff mentioned above] to avoid duplicated code).

I think it would make sense to just namespace the ParseInt32, ParseInt64 and ParseDouble into UniValue:: and compile/link them always.
Will do a PR upstream.

Updated this PR:

• Refreshed subtree to jgarzik/univalue@a243b3d (--squashed)
• Number parse functions are now independently implemented in UniValue over a anonymous namespace
• Global make check does now also covers the UniValue make check
• Replaced all #include "univalue/univalue.h" with #include <univalue.h> (was necessary because UniValues uses now univalue/lib/* as folder structure).

Travis issue is because of missing -fPIC support in UniValue. This PR (jgarzik/univalue#6) should fix it (add libtool script).

@jonasschnelli What build object needs -fPIC? UniValue has been built as a *.a library without problems...

I think it's recommended to build a *.la (libtool) library to gain more flexibility.

Travis complains about missing -fPIC recompile support:
https://travis-ci.org/bitcoin/bitcoin/jobs/79127433#L1480

Secp256k1 also uses libtool and fPIC:
https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L920

I hadn't used subtrees before, was confused as to the intention.
I had the impression we were just copying the code in.

After some further research, concept ACK 👍

@jonasschnelli @theuni @jgarzik Is this ready for merge? If so I think we should do a rebase and then immediately merge, to prevent it getting out of sync due to large number of files touched.

Rebased and force-pushed, ... waiting for travis now.

Upstream univalue tree has all the changes from @theuni - should be ready for merge.

"make[3]: *** No rule to make target `libunivalue.la'. Stop." hrm, might need LT_INIT in bitcoin configure.ac.

Travis is happy now.
IMO this is ready to merge.

Note: As usual after large build system changes, if you get:

Making all in src
make[1]: Entering directory `/store/orion/projects/bitcoin/bitcoin/src'
make[2]: Entering directory `/store/orion/projects/bitcoin/bitcoin/src'
make[3]: Entering directory `/store/orion/projects/bitcoin/bitcoin/src/univalue'
make[3]: *** No targets specified and no makefile found.  Stop.
make[3]: Leaving directory `/store/orion/projects/bitcoin/bitcoin/src/univalue'
make[2]: *** [univalue/lib/libunivalue.la] Error 2
make[2]: Leaving directory `/store/orion/projects/bitcoin/bitcoin/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/store/orion/projects/bitcoin/bitcoin/src'
make: *** [all-recursive] Error 1

You need to re-do ./autogen.sh and ./configure. The automatic triggering of those is not enough.

Edit: ACK

ACK

"git clean -dfx" is your friend. :)