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

0f95247 Integrate univalue into our buildsystem (Cory Fields)
9b49ed6 Squashed 'src/univalue/' changes from 98fadc0909..a44caf65fe (fanquake)

Pull request description:

  This PR more tightly integrates building Univalue into our build system. This follows the same approach we use for [LevelDB](https://github.com/bitcoin-core/leveldb/), ([`Makefile.leveldb.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.leveldb.include)), and [CRC32C](https://github.com/bitcoin-core/crc32c) ([`Makefile.crc32c.include`](https://github.com/bitcoin/bitcoin/blob/master/src/Makefile.crc32c.include)), and will be the same approach we use for [minisketch](https://github.com/sipa/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 bitcoin/bitcoin#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"](https://github.com/bitcoinknots/bitcoin/blob/a886811721ce66eb586871706b3f5dd27518ac3e/configure.ac#L1430) and ["NOT SUPPORTED"](https://github.com/bitcoinknots/bitcoin/blob/a886811721ce66eb586871706b3f5dd27518ac3e/configure.ac#L1312). 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.

ACKs for top commit:
  dongcarl:
    ACK 0f95247 less my comment above, always nice to have an include-able `sources.mk` which makes integration easier.
  theuni:
    ACK 0f95247. Thanks fanquake for keeping this going.

Tree-SHA512: a7f2e41ee7cba06ae72388638e86b264eca1b9a8b81c15d1d7b45df960c88c3b91578b4ade020f8cc61d75cf8d16914575f9a78fa4cef9c12be63504ed804b99

Author: fanquake

ci/test/06_script_b.sh

@@ -9,14 +9,12 @@ export LC_ALL=C.UTF-8
 if [[ $HOST = *-mingw32 ]]; then
   # Generate all binaries, so that they can be wrapped
   DOCKER_EXEC make $MAKEJOBS -C src/secp256k1 VERBOSE=1
-  DOCKER_EXEC make $MAKEJOBS -C src/univalue VERBOSE=1
   DOCKER_EXEC "${BASE_ROOT_DIR}/ci/test/wrap-wine.sh"
 fi
 
 if [ -n "$QEMU_USER_CMD" ]; then
   # Generate all binaries, so that they can be wrapped
   DOCKER_EXEC make $MAKEJOBS -C src/secp256k1 VERBOSE=1
-  DOCKER_EXEC make $MAKEJOBS -C src/univalue VERBOSE=1
   DOCKER_EXEC "${BASE_ROOT_DIR}/ci/test/wrap-qemu.sh"
 fi
 

configure.ac

@@ -261,12 +261,6 @@ if test "x$use_asm" = xyes; then
   AC_DEFINE(USE_ASM, 1, [Define this symbol to build in assembly routines])
 fi
 
-AC_ARG_WITH([system-univalue],
-  [AS_HELP_STRING([--with-system-univalue],
-  [Build with system UniValue (default is no)])],
-  [system_univalue=$withval],
-  [system_univalue=no]
-)
 AC_ARG_ENABLE([zmq],
   [AS_HELP_STRING([--disable-zmq],
   [disable ZMQ notifications])],
@@ -1522,34 +1516,6 @@ if test "x$use_zmq" = xyes; then
   esac
 fi
 
-dnl univalue check
-
-need_bundled_univalue=yes
-if test x$build_bitcoin_wallet$build_bitcoin_cli$build_bitcoin_tx$build_bitcoin_util$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench = xnononononononono; then
-  need_bundled_univalue=no
-else
-  if test x$system_univalue != xno; then
-    PKG_CHECK_MODULES([UNIVALUE], [libunivalue >= 1.0.4], [found_univalue=yes], [found_univalue=no])
-    if test x$found_univalue = xyes; then
-      system_univalue=yes
-      need_bundled_univalue=no
-    elif test x$system_univalue = xyes; then
-      AC_MSG_ERROR([univalue not found])
-    else
-      system_univalue=no
-    fi
-  fi
-
-  if test x$need_bundled_univalue = xyes; then
-    UNIVALUE_CFLAGS='-I$(srcdir)/univalue/include'
-    UNIVALUE_LIBS='univalue/libunivalue.la'
-  fi
-fi
-
-AM_CONDITIONAL([EMBEDDED_UNIVALUE],[test x$need_bundled_univalue = xyes])
-AC_SUBST(UNIVALUE_CFLAGS)
-AC_SUBST(UNIVALUE_LIBS)
-
 dnl libmultiprocess library check
 
 libmultiprocess_found=no
@@ -1912,10 +1878,6 @@ PKGCONFIG_LIBDIR_TEMP="$PKG_CONFIG_LIBDIR"
 unset PKG_CONFIG_LIBDIR
 PKG_CONFIG_LIBDIR="$PKGCONFIG_LIBDIR_TEMP"
 
-if test x$need_bundled_univalue = xyes; then
-  AC_CONFIG_SUBDIRS([src/univalue])
-fi
-
 ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental"
 AC_CONFIG_SUBDIRS([src/secp256k1])
 

contrib/guix/libexec/build.sh

@@ -297,7 +297,7 @@ mkdir -p "$DISTSRC"
                     ${HOST_CXXFLAGS:+CXXFLAGS="${HOST_CXXFLAGS}"} \
                     ${HOST_LDFLAGS:+LDFLAGS="${HOST_LDFLAGS}"}
 
-    sed -i.old 's/-lstdc++ //g' config.status libtool src/univalue/config.status src/univalue/libtool
+    sed -i.old 's/-lstdc++ //g' config.status libtool
 
     # Build Bitcoin Core
     make --jobs="$JOBS" ${V:+V=1}

src/Makefile.am

@@ -6,7 +6,7 @@
 print-%: FORCE
 	@echo '$*'='$($*)'
 
-DIST_SUBDIRS = secp256k1 univalue
+DIST_SUBDIRS = secp256k1
 
 AM_LDFLAGS = $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(GPROF_LDFLAGS) $(SANITIZER_LDFLAGS)
 AM_CXXFLAGS = $(DEBUG_CXXFLAGS) $(HARDENED_CXXFLAGS) $(WARN_CXXFLAGS) $(NOWARN_CXXFLAGS) $(ERROR_CXXFLAGS) $(GPROF_CXXFLAGS) $(SANITIZER_CXXFLAGS)
@@ -15,18 +15,7 @@ AM_LIBTOOLFLAGS = --preserve-dup-deps
 PTHREAD_FLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_LIBS)
 EXTRA_LIBRARIES =
 
-if EMBEDDED_UNIVALUE
-LIBUNIVALUE = univalue/libunivalue.la
-
-$(LIBUNIVALUE): $(wildcard univalue/lib/*) $(wildcard univalue/include/*)
-	$(AM_V_at)$(MAKE) $(AM_MAKEFLAGS) -C $(@D) $(@F)
-else
-LIBUNIVALUE = $(UNIVALUE_LIBS)
-endif
-
-BITCOIN_INCLUDES=-I$(builddir) -I$(srcdir)/secp256k1/include $(BDB_CPPFLAGS) $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS)
-
-BITCOIN_INCLUDES += $(UNIVALUE_CFLAGS)
+BITCOIN_INCLUDES=-I$(builddir) -I$(srcdir)/secp256k1/include -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT) $(BDB_CPPFLAGS) $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS)
 
 LIBBITCOIN_SERVER=libbitcoin_server.a
 LIBBITCOIN_COMMON=libbitcoin_common.a
@@ -80,6 +69,7 @@ EXTRA_LIBRARIES += \
   $(LIBBITCOIN_ZMQ)
 
 lib_LTLIBRARIES = $(LIBBITCOINCONSENSUS)
+noinst_LTLIBRARIES =
 
 bin_PROGRAMS =
 noinst_PROGRAMS =
@@ -797,7 +787,6 @@ $(top_srcdir)/$(subdir)/config/bitcoin-config.h.in:  $(am__configure_deps)
 
 clean-local:
 	-$(MAKE) -C secp256k1 clean
-	-$(MAKE) -C univalue clean
 	-rm -f leveldb/*/*.gcda leveldb/*/*.gcno leveldb/helpers/memenv/*.gcda leveldb/helpers/memenv/*.gcno
 	-rm -f config.h
 	-rm -rf test/__pycache__
@@ -875,3 +864,5 @@ endif
 if ENABLE_QT_TESTS
 include Makefile.qttest.include
 endif
+
+include Makefile.univalue.include

src/Makefile.test.include

@@ -350,8 +350,26 @@ if ENABLE_BENCH
 endif
 endif
 	$(AM_V_at)$(MAKE) $(AM_MAKEFLAGS) -C secp256k1 check
-if EMBEDDED_UNIVALUE
-	$(AM_V_at)$(MAKE) $(AM_MAKEFLAGS) -C univalue check
+
+if !ENABLE_FUZZ
+UNIVALUE_TESTS = univalue/test/object univalue/test/unitester univalue/test/no_nul
+noinst_PROGRAMS += $(UNIVALUE_TESTS)
+TESTS += $(UNIVALUE_TESTS)
+
+univalue_test_unitester_SOURCES = $(UNIVALUE_TEST_UNITESTER_INT)
+univalue_test_unitester_LDADD = $(LIBUNIVALUE)
+univalue_test_unitester_CPPFLAGS = -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT) -DJSON_TEST_SRC=\"$(srcdir)/$(UNIVALUE_TEST_DATA_DIR_INT)\"
+univalue_test_unitester_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
+
+univalue_test_no_nul_SOURCES = $(UNIVALUE_TEST_NO_NUL_INT)
+univalue_test_no_nul_LDADD = $(LIBUNIVALUE)
+univalue_test_no_nul_CPPFLAGS = -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)
+univalue_test_no_nul_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
+
+univalue_test_object_SOURCES = $(UNIVALUE_TEST_OBJECT_INT)
+univalue_test_object_LDADD = $(LIBUNIVALUE)
+univalue_test_object_CPPFLAGS = -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)
+univalue_test_object_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 endif
 
 %.cpp.test: %.cpp

src/Makefile.univalue.include

@@ -0,0 +1,6 @@
+include univalue/sources.mk
+
+LIBUNIVALUE = libunivalue.la
+noinst_LTLIBRARIES += $(LIBUNIVALUE)
+libunivalue_la_SOURCES = $(UNIVALUE_LIB_SOURCES_INT) $(UNIVALUE_DIST_HEADERS_INT) $(UNIVALUE_LIB_HEADERS_INT) $(UNIVALUE_TEST_FILES_INT)
+libunivalue_la_CPPFLAGS = $(AM_CPPFLAGS) -I$(srcdir)/$(UNIVALUE_INCLUDE_DIR_INT)

src/univalue/.cirrus.yml

@@ -0,0 +1,44 @@
+env:
+  MAKEJOBS: "-j4"
+  RUN_TESTS: "true"
+  BASE_OUTDIR: "$CIRRUS_WORKING_DIR/out_dir_base"
+  DEBIAN_FRONTEND: "noninteractive"
+
+task:
+  container:
+    image: ubuntu:focal
+    cpu: 1
+    memory: 1G
+    greedy: true  # https://medium.com/cirruslabs/introducing-greedy-container-instances-29aad06dc2b4
+
+  matrix:
+    - name: "gcc"
+      env:
+        CC: "gcc"
+        CXX: "g++"
+        APT_PKGS: "gcc"
+    - name: "clang"
+      env:
+        CC: "clang"
+        CXX: "clang++"
+        APT_PKGS: "clang"
+    - name: "mingw"
+      env:
+        CC: ""
+        CXX: ""
+        UNIVALUE_CONFIG: "--host=x86_64-w64-mingw32"
+        APT_PKGS: "g++-mingw-w64-x86-64 gcc-mingw-w64-x86-64 binutils-mingw-w64-x86-64"
+        RUN_TESTS: "false"
+
+  install_script:
+    - apt update
+    - apt install -y pkg-config build-essential libtool autotools-dev automake bsdmainutils
+    - apt install -y $APT_PKGS
+  autogen_script:
+     - ./autogen.sh
+  configure_script:
+     - ./configure --cache-file=config.cache --bindir=$BASE_OUTDIR/bin --libdir=$BASE_OUTDIR/lib $UNIVALUE_CONFIG
+  make_script:
+     - make $MAKEJOBS V=1
+  test_script:
+     - if [ "$RUN_TESTS" = "true" ]; then make $MAKEJOBS distcheck; fi

src/univalue/.travis.yml

@@ -1,20 +1,17 @@
+include sources.mk
 ACLOCAL_AMFLAGS = -I build-aux/m4
-.PHONY: gen
+.PHONY: gen FORCE
 .INTERMEDIATE: $(GENBIN)
 
-include_HEADERS = include/univalue.h
-noinst_HEADERS = lib/univalue_escapes.h lib/univalue_utffilter.h
+include_HEADERS = $(UNIVALUE_DIST_HEADERS_INT)
+noinst_HEADERS = $(UNIVALUE_LIB_HEADERS_INT)
 
 lib_LTLIBRARIES = libunivalue.la
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = pc/libunivalue.pc
 
-libunivalue_la_SOURCES = \
-	lib/univalue.cpp \
-	lib/univalue_get.cpp \
-	lib/univalue_read.cpp \
-	lib/univalue_write.cpp
+libunivalue_la_SOURCES = $(UNIVALUE_LIB_SOURCES_INT)
 
 libunivalue_la_LDFLAGS = \
 	-version-info $(LIBUNIVALUE_CURRENT):$(LIBUNIVALUE_REVISION):$(LIBUNIVALUE_AGE) \
@@ -30,89 +27,32 @@ $(GENBIN): $(GEN_SRCS)
 	@echo Building $@
 	$(AM_V_at)c++ -I$(top_srcdir)/include -o $@ $<
 
-gen: lib/univalue_escapes.h $(GENBIN)
-	@echo Updating $<
+gen: $(GENBIN) FORCE
+	@echo Updating lib/univalue_escapes.h
 	$(AM_V_at)$(GENBIN) > lib/univalue_escapes.h
 
 noinst_PROGRAMS = $(TESTS) test/test_json
 
-TEST_DATA_DIR=test
-
-test_unitester_SOURCES = test/unitester.cpp
+test_unitester_SOURCES = $(UNIVALUE_TEST_UNITESTER_INT)
 test_unitester_LDADD = libunivalue.la
-test_unitester_CXXFLAGS = -I$(top_srcdir)/include -DJSON_TEST_SRC=\"$(srcdir)/$(TEST_DATA_DIR)\"
+test_unitester_CXXFLAGS = -I$(top_srcdir)/include -DJSON_TEST_SRC=\"$(srcdir)/$(UNIVALUE_TEST_DATA_DIR_INT)\"
 test_unitester_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
-test_test_json_SOURCES = test/test_json.cpp
+test_test_json_SOURCES = $(UNIVALUE_TEST_JSON_INT)
 test_test_json_LDADD = libunivalue.la
 test_test_json_CXXFLAGS = -I$(top_srcdir)/include
 test_test_json_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
-test_no_nul_SOURCES = test/no_nul.cpp
+test_no_nul_SOURCES = $(UNIVALUE_TEST_NO_NUL_INT)
 test_no_nul_LDADD = libunivalue.la
 test_no_nul_CXXFLAGS = -I$(top_srcdir)/include
 test_no_nul_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
-test_object_SOURCES = test/object.cpp
+test_object_SOURCES = $(UNIVALUE_TEST_OBJECT_INT)
 test_object_LDADD = libunivalue.la
 test_object_CXXFLAGS = -I$(top_srcdir)/include
 test_object_LDFLAGS = -static $(LIBTOOL_APP_LDFLAGS)
 
-TEST_FILES = \
-	$(TEST_DATA_DIR)/fail10.json \
-	$(TEST_DATA_DIR)/fail11.json \
-	$(TEST_DATA_DIR)/fail12.json \
-	$(TEST_DATA_DIR)/fail13.json \
-	$(TEST_DATA_DIR)/fail14.json \
-	$(TEST_DATA_DIR)/fail15.json \
-	$(TEST_DATA_DIR)/fail16.json \
-	$(TEST_DATA_DIR)/fail17.json \
-	$(TEST_DATA_DIR)/fail18.json \
-	$(TEST_DATA_DIR)/fail19.json \
-	$(TEST_DATA_DIR)/fail1.json \
-	$(TEST_DATA_DIR)/fail20.json \
-	$(TEST_DATA_DIR)/fail21.json \
-	$(TEST_DATA_DIR)/fail22.json \
-	$(TEST_DATA_DIR)/fail23.json \
-	$(TEST_DATA_DIR)/fail24.json \
-	$(TEST_DATA_DIR)/fail25.json \
-	$(TEST_DATA_DIR)/fail26.json \
-	$(TEST_DATA_DIR)/fail27.json \
-	$(TEST_DATA_DIR)/fail28.json \
-	$(TEST_DATA_DIR)/fail29.json \
-	$(TEST_DATA_DIR)/fail2.json \
-	$(TEST_DATA_DIR)/fail30.json \
-	$(TEST_DATA_DIR)/fail31.json \
-	$(TEST_DATA_DIR)/fail32.json \
-	$(TEST_DATA_DIR)/fail33.json \
-	$(TEST_DATA_DIR)/fail34.json \
-	$(TEST_DATA_DIR)/fail35.json \
-	$(TEST_DATA_DIR)/fail36.json \
-	$(TEST_DATA_DIR)/fail37.json \
-	$(TEST_DATA_DIR)/fail38.json \
-	$(TEST_DATA_DIR)/fail39.json \
-	$(TEST_DATA_DIR)/fail40.json \
-	$(TEST_DATA_DIR)/fail41.json \
-	$(TEST_DATA_DIR)/fail42.json \
-	$(TEST_DATA_DIR)/fail44.json \
-	$(TEST_DATA_DIR)/fail45.json \
-	$(TEST_DATA_DIR)/fail3.json \
-	$(TEST_DATA_DIR)/fail4.json \
-	$(TEST_DATA_DIR)/fail5.json \
-	$(TEST_DATA_DIR)/fail6.json \
-	$(TEST_DATA_DIR)/fail7.json \
-	$(TEST_DATA_DIR)/fail8.json \
-	$(TEST_DATA_DIR)/fail9.json \
-	$(TEST_DATA_DIR)/pass1.json \
-	$(TEST_DATA_DIR)/pass2.json \
-	$(TEST_DATA_DIR)/pass3.json \
-	$(TEST_DATA_DIR)/pass4.json \
-	$(TEST_DATA_DIR)/round1.json \
-	$(TEST_DATA_DIR)/round2.json \
-	$(TEST_DATA_DIR)/round3.json \
-	$(TEST_DATA_DIR)/round4.json \
-	$(TEST_DATA_DIR)/round5.json \
-	$(TEST_DATA_DIR)/round6.json \
-	$(TEST_DATA_DIR)/round7.json
-
-EXTRA_DIST=$(TEST_FILES) $(GEN_SRCS)
+TEST_FILES = $(UNIVALUE_TEST_FILES_INT)
+
+EXTRA_DIST=$(UNIVALUE_TEST_FILES_INT) $(GEN_SRCS)