Add UniValue as subtree #6637

pull jonasschnelli wants to merge 5 commits into bitcoin:master from jonasschnelli:2015/09/univalue_subtree changing 87 files +632 −48
  1. jonasschnelli commented at 2:45 pm on September 4, 2015: contributor

    This PR adds UniValue over a git subtree. Main repository is currently https://github.com/jgarzik/univalue.

    Edit: You need to re-do ./autogen.sh and ./configure after this is merged

  2. jgarzik commented at 2:47 pm on September 4, 2015: contributor
    concept ACK, of course :)
  3. sipa commented at 2:55 pm on September 4, 2015: member
    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.
  4. jonasschnelli force-pushed on Sep 4, 2015
  5. jonasschnelli commented at 3:12 pm on September 4, 2015: contributor
    @sipa: done.
  6. luke-jr commented at 4:11 pm on September 4, 2015: member
    Better, but this should really be an external shared library… Concept ACK
  7. dcousens commented at 1:25 am on September 5, 2015: contributor
    Is there any modern package management solution for C++?
  8. luke-jr commented at 1:32 am on September 5, 2015: member
    pkg-config works great.
  9. jonasschnelli commented at 7:27 am on September 5, 2015: contributor

    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.

  10. luke-jr commented at 7:39 am on September 5, 2015: member
    @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)
  11. jonasschnelli commented at 8:03 am on September 5, 2015: contributor
    @luke-jr: ok, fair enough.
  12. sipa commented at 11:21 am on September 5, 2015: member
    @jonasschnelli: can you explain why the univalue tests cannot be included? I don’t see how the current code in Bitcoin Core matters, as it is replaced by upstream UniValue.
  13. jonasschnelli commented at 6:09 pm on September 5, 2015: contributor

    @sipa: Recently we have coupled UniValue with utilstrencodings.cpp/h (see https://github.com/bitcoin/bitcoin/commit/c02309204b8195476945f7066e8d96c60246db08#diff-0f1b401041a14398229cf7e31b6db7eeR13). To re-acchive independence from other sources, i have added the depending numparse functions in UniValue (see https://github.com/jgarzik/univalue/commit/d0ec6ad70f2a1590138adbcbb78ab8f0c1f8a47e and https://github.com/jgarzik/univalue/commit/4b288b380a364737c7488b6cc9bb552f98d7f890). 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.

  14. jgarzik commented at 6:11 pm on September 5, 2015: contributor

    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.

  15. sipa commented at 6:12 pm on September 5, 2015: member
    Well that’s not good, libunivalue should not depend on any code from bitcoin. And apparently, it doesn’t even need to because it has its own number conversion functions. Why not just use those?
  16. jgarzik commented at 6:17 pm on September 5, 2015: contributor

    @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.

  17. sipa commented at 6:20 pm on September 5, 2015: member
    How about renaming the one in libunivalue, or putting it in another namespace, so they don’t clash. I don’t care about duplication of such a tiny function.
  18. jonasschnelli commented at 6:21 pm on September 5, 2015: contributor

    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.

  19. sipa commented at 6:36 pm on September 5, 2015: member
    Got it. Yeah, that seems like the cleanest approach.
  20. jonasschnelli force-pushed on Sep 7, 2015
  21. jonasschnelli force-pushed on Sep 7, 2015
  22. jonasschnelli force-pushed on Sep 7, 2015
  23. jonasschnelli commented at 1:58 pm on September 7, 2015: contributor

    Updated this PR:

    • Refreshed subtree to https://github.com/jgarzik/univalue/commit/a243b3d2e67cdb4550c8801beed987a089afe1a5 (–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).
  24. jonasschnelli commented at 3:09 pm on September 7, 2015: contributor
    Travis issue is because of missing -fPIC support in UniValue. This PR (https://github.com/jgarzik/univalue/pull/6) should fix it (add libtool script).
  25. jgarzik commented at 8:31 pm on September 7, 2015: contributor
    @jonasschnelli What build object needs -fPIC? UniValue has been built as a *.a library without problems…
  26. jonasschnelli commented at 8:40 pm on September 7, 2015: contributor

    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

  27. jonasschnelli force-pushed on Sep 8, 2015
  28. laanwj added the label Refactoring on Sep 8, 2015
  29. jonasschnelli force-pushed on Sep 23, 2015
  30. jonasschnelli force-pushed on Sep 24, 2015
  31. jonasschnelli force-pushed on Sep 24, 2015
  32. laanwj commented at 10:31 am on September 24, 2015: member
    @theuni can you take a look here please?
  33. jonasschnelli commented at 2:24 pm on September 24, 2015: contributor

    Updated. Now runs unit tests (univalue’s make check) and also passes Win32/64 travis.

    UniValue has now a stable ABI and versioning.

    Because it’s not available over package management systems, i think adding it over “/depends” makes no sense yet.

    Adding it over a git subtree would split off discussions and PR for UniValue. If other projects start using UniValue, Bitcoin-Core could also benefit from maintenance and new features/optimizations.

  34. dcousens commented at 6:51 am on September 25, 2015: contributor
    What is the motivation for this? Won’t this just be one more thing to keep in sync?
  35. sipa commented at 6:53 am on September 25, 2015: member
    It’s already one more thing to keep in sync, as univalue is being developed outside of Bitcoin Core. Using a subtree is just one way of keeping track.
  36. dcousens commented at 8:34 am on September 25, 2015: contributor

    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 :+1:

  37. jonasschnelli force-pushed on Sep 25, 2015
  38. laanwj commented at 8:16 am on October 1, 2015: member
    @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.
  39. remove univalue, prepare for subtree 0917306fdf
  40. Squashed 'src/univalue/' content from commit 87d9045 
    git-subtree-dir: src/univalue
git-subtree-split: 87d90455ff5e87dedc304353aa23ace47ffb6c1c
    2f9f082b5e
  41. Merge commit '2f9f082b5ef3c495c70598ef23383effef675f9a' as 'src/univalue' 6e16a41313
  42. jonasschnelli force-pushed on Oct 1, 2015
  43. jonasschnelli commented at 8:39 am on October 1, 2015: contributor
    Rebased and force-pushed, … waiting for travis now.
  44. jonasschnelli force-pushed on Oct 1, 2015
  45. [Univalue] add univalue over subtree 
    similar to secp256k1 include and compile univalue over a subtree
    9623e93473
  46. jonasschnelli force-pushed on Oct 1, 2015
  47. jgarzik commented at 9:27 am on October 1, 2015: contributor
    Upstream univalue tree has all the changes from @theuni - should be ready for merge.
  48. jgarzik commented at 9:28 am on October 1, 2015: contributor
    “make[3]: *** No rule to make target `libunivalue.la’. Stop.” hrm, might need LT_INIT in bitcoin configure.ac.
  49. jonasschnelli force-pushed on Oct 1, 2015
  50. jonasschnelli force-pushed on Oct 1, 2015
  51. jonasschnelli force-pushed on Oct 1, 2015
  52. remove $(@F) and subdirs from univalue make 95acf3cc6d
  53. jonasschnelli force-pushed on Oct 1, 2015
  54. jonasschnelli commented at 1:20 pm on October 1, 2015: contributor
    Travis is happy now. IMO this is ready to merge.
  55. laanwj commented at 1:49 pm on October 1, 2015: member

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

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

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

    Edit: ACK

  56. btcdrak commented at 2:21 pm on October 1, 2015: contributor
    ACK
  57. laanwj merged this on Oct 1, 2015
  58. laanwj closed this on Oct 1, 2015
  59. laanwj referenced this in commit f297042cae on Oct 1, 2015
  60. jgarzik commented at 4:55 pm on October 1, 2015: contributor
    “git clean -dfx” is your friend. :)
  61. zkbot referenced this in commit eaaa5f625f on Feb 10, 2017
  62. MarcoFalke locked this on Sep 8, 2021


jonasschnelli jgarzik sipa luke-jr dcousens laanwj btcdrak

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-10 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me