< bitcoin-git>
[bitcoin] gwillen opened pull request #19356: build: Fix & improve the search for BDB (master...feature-bdb-search-fix-osx) https://github.com/bitcoin/bitcoin/pull/19356
< gwillen>
if anybody loves (or hates) autotools, I would love a look at the PR I just opened ^
< gwillen>
I made one change that I actually need to fix a specific issue on my OS X system, and some general cleanup that I will remove if it's controversial.
< * sipa>
summons cfields ^
< gwillen>
if anybody remembers my comment about mysterious segfaults when building another PR, this PR is the punchline (without it, I end up getting headers from one version of bdb, and libs from another version.)
< cfields>
gwillen: thanks. Looks more correct at a glance, will look in detail tomorrow.
< gwillen>
thanks very much for the quick look cfields!
< * cfields>
considers picking a CFLAGS fight just because gwillen seems to want it
< cfields>
Joking, of course. I agree with you. It's just sometimes hard to balance the fact that everyone wants CFLAGS as a hammer.
< * sipa>
idly suggests switching to maven
< cfields>
sipa: patches welcome!
< fanquake>
😢
< cfields>
(comprehensive patches :p)
< sipa>
haha
< gwillen>
cfields: yeah I am happy to relinquish the CFLAGS/CPPFLAGS part of the change if that will make life easier, but I figured I'd try for a cleanup while I was poking around
< gwillen>
I can understand why people reach for CFLAGS, it is all over documentation everywhere, I would have done the same but I learned the difference in reading the documentation while making this change :-)
< cfields>
gwillen: the reason I think I'm going to have to fight you on it is because pkg-config uses the _CFLAGS convention. So if you do "./configure --help", all of the paths are _CFLAGS. Which is really unfortunate, because _CPPFLAGS would make much more sense for those.
< cfields>
But I'm not completely sure what your problem is independent of the _CFLAGS change. Is it just that the include-order is wrong in the brew case?
< gwillen>
cfields: correct, the change from CFLAGS/LIBS to BDB_CFLAGS/BDB_LIBS fixes the include order in the brew case, it pushes the BDB 4.8 include earlier, to override the inclusion of /usr/local/include elsewhere in the list of paths, which pulls in all kinds of random stuff
< gwillen>
whereas the CFLAGS/CPPFLAGS change is just for fun
< gwillen>
I confess I don't know much at all about pkg-config, I will read up on that
< cfields>
gwillen: many projects (like us) use pkg-config to add dependencies. Doing so automatically hooks up a FOO_CFLAGS and FOO_LIBS that can be used to supply the respective paths for that dependency.
< cfields>
gwillen: so when you run "./configure --help" you see those possible overrides. That's why the _CFLAGS convention is so (annoyingly) common for specifying an include path.
< cfields>
So while wishing to pass BDB_CPPFLAGS is *far* more correct, it's not what anything expects.
< gwillen>
yeah that makes sense, I see that at some point pkg-config apparently grew the ability to deal with CPPFLAGS, but late enough that it's probably locked in at this point
< gwillen>
let me fix it up to work the other way, I think there is a small annoyance I will have to deal with in the process and you can review the result
< cfields>
gwillen: it's up to package maintainers to supply their own flags...
< gwillen>
well the thing that makes it annoying is that automake does require it to be CPPFLAGS for BDB in particular, or CXXFLAGS, since we need it to be present for C++ compilation
< luke-jr>
the spec seems to say there is no cppflags for pkg-config
< gwillen>
luke-jr: it doesn't automatically include BDB_CPPFLAGS, but it does automatically include libbitcoin_server_a_CPPFLAGS, which is what we put BDB_CPPFLAGS into
< cfields>
BITCOIN_INCLUDES += $(UNIVALUE_CFLAGS)
< luke-jr>
gwillen: point is, DEP_CFLAGS is supposed to be the place for -I and such
< gwillen>
anyway, I guess the answer is that someone somewhere stuffs the contents of CFLAGS into something named CPPFLAGS somewhere
< cfields>
gwillen: yep, that.
< gwillen>
hmmm actually I guess I understand luke-jr's point
< gwillen>
although I'm not sure if it quite makes sense for BDB which is itself a C++ library
< luke-jr>
gwillen: this is also how C++ libraries do it.. <.<
< luke-jr>
there is no Cxxflags in pkg-config either
< gwillen>
so basically pkg-config and automake just like, permanently differ on this, I guess?
< luke-jr>
seems so
< gwillen>
(e.g. ax_boost_base.m4 defines AC_SUBST(BOOST_CPPFLAGS), not BOOST_CFLAGS, for where it finds boost)
< cfields>
gwillen: I don't see those exported.
< cfields>
also, boost doesn't ship pkg-config files. Which is why we carry that monstrosity of a .m4.
< fanquake>
The problem is deeper inside libevent, but hopefully can get that fixed up for 0.20.1
< luke-jr>
fanquake: ?
< fanquake>
luke-jr: the problem seems to be with the “hint” flags that libevent is setting before getaddr* calls. The changes in the PR don’t fix the issue for us entirely, as mentioned in the comments there.
< luke-jr>
hrm, been too long since I made that to remember about hint stuff :/
< luke-jr>
strange that it fixes it for some of us and not others
< bitcoin-git>
[bitcoin] Saibato opened pull request #19358: net: Make sure we do not override proxy settings in hidden service. (master...pr351-bitcoin) https://github.com/bitcoin/bitcoin/pull/19358
< sipa>
MarcoFalke: i'm finding it a bit annoying that codebase changes often break existing fuzz tests, which require travis or an entirely separate build to find out
< sipa>
what do you think about "building" the fuzz tests in normal make all mode (but without the actual fuzzing enabled)
< sipa>
one caveat is that the fuzz tests currently already need c++17
< wumpus>
sipa: I'm not sure it should be the default, but I do agree it should be easier (e.g. a single configure option) to build the fuzz tests as well sa everything else
< wumpus>
having to do a second build with either-or exclusive options is kind of a time sink and easy to forget
< sipa>
wumpus: it seems that building the actual fuzz tests within one configure is very hard
< wumpus>
okay
< sipa>
but just something that can test whether the fuzz tests build is great already, i think
< sipa>
because it's really easy to introduce compile errors in them (and this will likely worsen as their coverage increases)
< wumpus>
right
< sipa>
we could even have a minimal main() for the fake-fuzz tests that reads a single input from stdin, and runs the test code on it
< sipa>
or all entries in a directory... meaning it'd be actually useful as a test harness; you just wouldn't be able to fuzz with it
< wumpus>
that's a neat idea, at least the building feels less like dead weight in that case
< sipa>
as the travis fuzz build isn't actually doing any fuzzing, this may even be sufficient there
< bitcoin-git>
[bitcoin] jnewbery opened pull request #19364: net processing: Move orphan reprocessing to a global (master...2020-06-global-orphans) https://github.com/bitcoin/bitcoin/pull/19364
< cfields>
it seems getting them to link as real progs will have linker issues due to missing libs. I guess the linker doesn't go looking for unresolved symbols with no main?
< sipa>
cfields: hmm, what symbols are missing?
< cfields>
sipa: zmq/miniupnpc/etc.
< sipa>
oh
< sipa>
that sounds fixable?
< cfields>
We could tack them on, but that kinda sucks.
< cfields>
Yeah, there's another approach though to avoid linking altogether though, 1 sec.
< sipa>
or maybe it's just not an issue
< sipa>
because if it's not a problem for real fuzzing, why would it here?
< sipa>
perhaps it's due to not having main() indeed
< sipa>
though it can certainly use a note at least that it's subject to users noticing they're being asked to sign multiple times
< luke-jr>
sipa: they might think they're only signing twice to 0.0001 BTC txs, and figure "meh, worst case I lose 0.0002 BTC"
< luke-jr>
(while the fee could end up being 100 BTC or whatever)
< sipa>
a weaker benefit also remains: you can sign without transfering all txn-creating-outputs-being-spent, in case you know they're correct
< sipa>
while pre-segwit you just had to have the full transactions at signing time
< bitcoin-git>
[bitcoin] practicalswift opened pull request #19366: tests: Provide main(...) function in fuzzer. Allow building uninstrumented harnesses with --enable-fuzz. (master...provide-main-function-in-fuzzer) https://github.com/bitcoin/bitcoin/pull/19366
< luke-jr>
sipa: I don't understand what you mean
< sipa>
luke-jr: nvm, i'm wrong
< luke-jr>
sipa: there is a request to make a revision to taproot, and call it tarproot.