RFC: Add autotools buildsystem #8

theuni · 2019-01-16T20:46:54Z

Should be functionally equivalent to the old Makefile ~~, minus the logic to handle debug builds, as it's unclear how they should work ideally~~.

Sources are split into a separate file for easier inclusion in downstream projects ~~, though a simple sed will be needed to correct for paths~~(no longer needed). This will allow Bitcoin Core (and other projects) to incorporate sources into their buildsystems similar to the way that leveldb is currently handled, rather than requiring a subconfigure.

Ideally (imo), univalue and libsecp256k1 would get the same treatment.

theuni · 2019-01-16T20:49:29Z

configure.ac

+## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
+## unknown options if any other warning is produced. Test the -Wfoo case, and
+## set the -Wno-foo case if it works.
+AX_CHECK_COMPILE_FLAG([-Wshift-count-overflow],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-shift-count-overflow"],,[[$CXXFLAG_WERROR]])


This is here because of a loud clang warning, which I assume is caused by some unreachable code. Rather than hunting it down, I just silenced it here for now. I'll open a separate issue for that warning.

theuni · 2019-01-16T20:50:42Z

src/fields/clmul_1byte.cpp

@@ -6,7 +6,7 @@

 /* This file was substantially auto-generated by doc/gen_params.sage. */

-#ifdef __x86_64
+#ifdef HAVE_CLMUL


Now that these files are conditionally built, is there any reason for these guards?

I guess not.

theuni · 2019-01-16T22:40:52Z

Fixed up a few problems, now ready for testing.

I also finally found the secret sauce that allows the sources.mk to be used downstream with no modification (Automake's %reldir%).

Here's a quick POC to show how trivial the Bitcoin Core integration is: theuni/bitcoin@1e779c8 . To test, just copy the contents of this branch to Core at src/minisketch and make.

sipa · 2019-01-17T00:52:35Z

It looks like this does not build the test object code with -DMINISKETCH_VERIFY.

theuni · 2019-01-17T00:55:38Z

It wasn't clear to me how that should be handled. Is it sufficient to compile only the test files that way, or does everything need to be rebuilt?

sipa · 2019-01-17T00:57:39Z

@theuni All the object files that include sketch_impl.h should be built with -DMINISKETCH_VERIFY for tests (which probably means separate .a files for testing etc...).

theuni · 2019-01-17T00:58:30Z

Got it, will do.

sipa · 2019-01-17T00:58:53Z

Awesome, thanks!

theuni · 2019-01-17T01:36:52Z

@sipa fixed.

sipa · 2019-01-17T01:44:49Z

  CXXLD    test-exhaust
/usr/bin/ld: src/test_exhaust-test-exhaust.o: in function `TestAll(int, int, int, unsigned int)':
/usr/include/c++/8/thread:131: undefined reference to `pthread_create'
/usr/bin/ld: /usr/include/c++/8/thread:131: undefined reference to `pthread_create'
collect2: error: ld returned 1 exit status
make: *** [Makefile:973: test-exhaust] Error 1

theuni · 2019-01-17T02:14:39Z

@sipa Should be fixed. I'm currently testing on macOS where the flags aren't required, so I can't be 100%.

sipa · 2019-01-17T20:33:25Z

We don't actually use assert anymore anywhere in the codebase, so NDEBUG shouldn't be needed.

theuni · 2019-01-17T20:39:00Z

Ok, removed it but left the machinery to add others later.

gmaxwell · 2019-01-20T07:19:52Z

Very cool. Would it be crazy to also build a test binary linked against the normal library (just so it catches any issues that are accidentally fixed by the verify macro?)?

theuni · 2019-01-21T23:15:35Z

@gmaxwell Added for the sake of discussion, though I'm not sure I agree.

I wonder if we can somehow constrain the macros to be side-effect free. Maybe using function attributes (const and pure, for example) and/or trivial callbacks that are optimized away?

gmaxwell · 2019-01-27T09:11:55Z

meh, part of the goal of having tests is catching bugs introduced by miscompilation (or undefined behaviour, which looks a lot like miscompilation). I think it would be a bad idea to never test the code the applications use... If building two weren't an option I'd rather just not compile with the test instrumentation except in special test builds... the tests still run fine against the release library, they're just not as sensitive.

sipa · 2019-02-05T00:16:59Z

Concept ACK, I think this is fine (including the separate the two binaries for testing).

fanquake · 2019-02-05T15:52:22Z

tested 6eaccf6 on macOS 10.14.3

./autogen.sh
./configure
make

works as expected.

Ran make install:

 /usr/local/bin/gmkdir -p '/usr/local/lib'
 /bin/sh ./libtool   --mode=install /usr/local/bin/ginstall -c   libminisketch.la '/usr/local/lib'
libtool: install: /usr/local/bin/ginstall -c .libs/libminisketch.0.dylib /usr/local/lib/libminisketch.0.dylib
libtool: install: (cd /usr/local/lib && { ln -s -f libminisketch.0.dylib libminisketch.dylib || { rm -f libminisketch.dylib && ln -s libminisketch.0.dylib libminisketch.dylib; }; })
libtool: install: /usr/local/bin/ginstall -c .libs/libminisketch.lai /usr/local/lib/libminisketch.la
libtool: install: /usr/local/bin/ginstall -c .libs/libminisketch.a /usr/local/lib/libminisketch.a
libtool: install: chmod 644 /usr/local/lib/libminisketch.a
libtool: install: ranlib /usr/local/lib/libminisketch.a
 /usr/local/bin/gmkdir -p '/usr/local/include'
 /usr/local/bin/ginstall -c -m 644 include/minisketch.h '/usr/local/include'

then the example:

gcc -std=c99 -Wall -Wextra -o example ./doc/example.c -Lsrc/ -lminisketch -lstdc++ && ./example
3010 is in only one of the two sets
3011 is in only one of the two sets
3000 is in only one of the two sets
3001 is in only one of the two sets

./bench:

recover[ms]	  9	   0.36986	         -	   0.26691	
create[ns]	  9	   2.72464	         -	   6.51640	
recover[ms]	 10	   0.51630	   0.44340	   0.46058	
<snip>	
create[ns]	 63	   7.18044	         -	   7.35585	
recover[ms]	 64	  19.06820	   3.63432	         -	
create[ns]	 64	   6.79974	   8.18448	         -

Currently running make check:

tail -f test-exhaust.log
bits=11 count=2 below_bound=[1,2047,2094081] above_bound=[] nodecode=[0.500244]
bits=22 count=1 below_bound=[1,4194303] above_bound=[] nodecode=[0]
bits=23 count=1 below_bound=[1,8388607] above_bound=[] nodecode=[0]
bits=4 count=6 below_bound=[1,15,105,455,1365,3003,5005] above_bound=[] nodecode=[0.999407]
bits=6 count=4 below_bound=[1,63,1953,39711,595665] above_bound=[1512/7028847,63/67945521,9/553270671] nodecode=[0.961914]
bits=8 count=3 below_bound=[1,255,32385,2731135] above_bound=[5355/172061505,51/8637487551] nodecode=[0.834944]
bits=12 count=2 below_bound=[1,4095,8382465] above_bound=[1365/11436476415] nodecode=[0.500041]
bits=24 count=1 below_bound=[1,16777215] above_bound=[] nodecode=[0]
bits=5 count=5 below_bound=[1,31,465,4495,31465,169911] above_bound=[217/736281] nodecode=[0.993843]
bits=25 count=1 below_bound=[1,33554431] above_bound=[] nodecode=[0]
bits=13 count=2 below_bound=[1,8191,33542145] above_bound=[] nodecode=[0.500061]
bits=26 count=1 below_bound=[1,67108863] above_bound=[] nodecode=[0]

real-or-random · 2019-02-26T12:31:53Z

src/Makefile

@@ -1,95 +0,0 @@
-default: test-exhaust bench
-
-DEFINES := -DHAVE_CLZ -O2 -g0


HAVE_CLZ is not covered by the PR

Thanks for catching that! Fixed.

Don't CLZ test with zero, it's undefined behaviour. :)

Heh, thanks. Fixed.

sipa · 2019-03-04T22:41:39Z

Tiny nit: the _debug suffix on the library files makes it sound like they're versions with debug symbols included or something. I think _verify or _test is probably better.

Otherwise, looks good. Please squash?

theuni · 2019-03-05T00:02:54Z

Rebased and squashed. New commits added before squashing:

Addressed @gmaxwell's comment
Renamed debug -> verify
Dropped test-exhaust-release and added test-exhaust-verify

For reference, the unrebased/unsquashed branch:
https://github.com/theuni/minisketch/commits/buildsystem-unsquashed

Worth noting, as-is, this builds everything several times. We should consider setting a default static/shared to avoid duplicating all objects. Also, non-verify binaries (and objects) could be disabled by default.

real-or-random · 2019-03-06T14:41:41Z

Funnily, make fails when configured with CC=clang, even though no C compiler is required here. (I found this when I wanted to test with clang but typed CC instead of CXX out of habit.) The reason is that all the checks are made against the C compiler because AC_LANG(C++) is missing.

ACK with AC_LANG(C++) added

real-or-random · 2019-03-06T14:56:45Z

Funnily, make fails when configured with CC=clang, even though no C compiler is required here. (I found this when I wanted to test with clang but typed CC instead of CXX out of habit.) The reason is that all the checks are made against the C compiler because AC_LANG(C++) is missing.

Hm, actually I'm not sure not. AC_LANG(C++) is certainly right but even with that set there are some checks performed against the C compiler (instead of C++), e.g.,

checking whether the C compiler works... yes
checking for C compiler default output file name... a.out

I can imagine that this is on purpose but I'm not an expert enough to tell if it's correct or not.

theuni · 2019-03-06T23:36:11Z

@real-or-random Huh. Thanks for noticing!

We use LT_LANG([C++]), which I always assumed also set AC_LANG(C++) internally, but apparently it doesn't! Very unexpected, but I can confirm different (correct) results when AC_LANG is set as well. TIL.

I'll add that one-liner and re-squash.

Should be functionally equivalent to the old Makefile, minus the logic to handle debug builds. Sources are split into a separate file for easier inclusion in downstream projects.

sipa · 2019-03-06T23:46:56Z

utACK 7a05881

7a05881 Add autotools buildsystem (Cory Fields) Pull request description: Should be functionally equivalent to the old Makefile ~~, minus the logic to handle debug builds, as it's unclear how they should work ideally~~. Sources are split into a separate file for easier inclusion in downstream projects ~~, though a simple sed will be needed to correct for paths~~(no longer needed). This will allow Bitcoin Core (and other projects) to incorporate sources into their buildsystems similar to the way that leveldb is currently handled, rather than requiring a subconfigure. Ideally (imo), univalue and libsecp256k1 would get the same treatment. ACKs for commit 7a0588: sipa: utACK 7a05881 Tree-SHA512: 52f99861ca06c83920bd352ffef9c7cdce9cd7cc83ed57c1dcd4570b9cace46b0025ab1c16b6bc021c7afac41f22d495d11b0524b82407be26f64c72334e9ff0

edited #Building to account for sipa#8

4a5b0a1 build: Move source entries out to sources.mk (Cory Fields) 6c7d94b build: cleanup wonky gen usage (Cory Fields) Pull request description: This addresses issues like the one in bitcoin/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. A similar split was recently added for minisketch: sipa/minisketch#8 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 here, and to downstreams as well until they take advantage of the new sources.mk. Once merged, [This Core branch](https://github.com/theuni/bitcoin/commits/univalue-split) is ready-ish for PR, and takes advantage of the features added here. libsecp256k1 will get the same treatment once this is done. ACKs for top commit: jnewbery: Tested ACK 4a5b0a1 fanquake: ACK 4a5b0a1 Tree-SHA512: 7c9237b5a4eea8d07da0b58471fabc6860e3dfe253f505bbf83366bb28d377c967a1c811bee04913e7da2121a7e0eaff31dd9681c3595d70e29c4ec08dbb2105

theuni commented Jan 16, 2019

View reviewed changes

theuni force-pushed the buildsystem branch from 5e2e563 to 1800f4a Compare January 16, 2019 22:33

theuni force-pushed the buildsystem branch from 1800f4a to ab46bcc Compare January 17, 2019 01:36

theuni force-pushed the buildsystem branch from bb73066 to 6eaccf6 Compare January 21, 2019 23:34

real-or-random reviewed Feb 26, 2019

View reviewed changes

theuni force-pushed the buildsystem branch from 1e87754 to 7b05b33 Compare March 4, 2019 23:54

theuni force-pushed the buildsystem branch from 7b05b33 to 7a05881 Compare March 6, 2019 23:39

Add autotools buildsystem

7a05881

Should be functionally equivalent to the old Makefile, minus the logic to handle debug builds. Sources are split into a separate file for easier inclusion in downstream projects.

sipa merged commit 7a05881 into sipa:master Jun 10, 2019

theuni mentioned this pull request Jun 17, 2019

Split sources for easier buildsystem integration bitcoin-core/univalue-subtree#19

Merged

mal2 added a commit to mal2/minisketch that referenced this pull request Nov 25, 2019

Update README.md

6a03b8c

edited #Building to account for sipa#8

fanquake mentioned this pull request Jul 16, 2021

build: minor build changes #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Add autotools buildsystem #8

RFC: Add autotools buildsystem #8

theuni commented Jan 16, 2019 •

edited

Loading

theuni Jan 16, 2019

theuni Jan 16, 2019

sipa Jan 16, 2019

theuni commented Jan 16, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

gmaxwell commented Jan 20, 2019

theuni commented Jan 21, 2019

gmaxwell commented Jan 27, 2019 •

edited

Loading

sipa commented Feb 5, 2019

fanquake commented Feb 5, 2019

real-or-random Feb 26, 2019

theuni Feb 27, 2019

gmaxwell Mar 2, 2019

theuni Mar 4, 2019

sipa commented Mar 4, 2019

theuni commented Mar 5, 2019

real-or-random commented Mar 6, 2019

real-or-random commented Mar 6, 2019

theuni commented Mar 6, 2019

sipa commented Mar 6, 2019

		@@ -1,95 +0,0 @@
		default: test-exhaust bench

		DEFINES := -DHAVE_CLZ -O2 -g0

RFC: Add autotools buildsystem #8

RFC: Add autotools buildsystem #8

Conversation

theuni commented Jan 16, 2019 • edited Loading

theuni Jan 16, 2019

Choose a reason for hiding this comment

theuni Jan 16, 2019

Choose a reason for hiding this comment

sipa Jan 16, 2019

Choose a reason for hiding this comment

theuni commented Jan 16, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

sipa commented Jan 17, 2019

theuni commented Jan 17, 2019

gmaxwell commented Jan 20, 2019

theuni commented Jan 21, 2019

gmaxwell commented Jan 27, 2019 • edited Loading

sipa commented Feb 5, 2019

fanquake commented Feb 5, 2019

real-or-random Feb 26, 2019

Choose a reason for hiding this comment

theuni Feb 27, 2019

Choose a reason for hiding this comment

gmaxwell Mar 2, 2019

Choose a reason for hiding this comment

theuni Mar 4, 2019

Choose a reason for hiding this comment

sipa commented Mar 4, 2019

theuni commented Mar 5, 2019

real-or-random commented Mar 6, 2019

real-or-random commented Mar 6, 2019

theuni commented Mar 6, 2019

sipa commented Mar 6, 2019

theuni commented Jan 16, 2019 •

edited

Loading

gmaxwell commented Jan 27, 2019 •

edited

Loading