I have been manually testing this branch on the macos-0 CI worker, to bypass the (in this branch) non-working build.sh. It is set up per #2674, ie. automake and libtool installed from brew, but using the compiler that came on the machine with Xcode (llvm 9.0.0 / clang 900). Here are the commands I've been using:

NO_RUST=1 NO_PROTON=1 make -C depends/ V=1
./autogen.sh
./configure --prefix=/Users/zcbbworker/test/zcash/depends/x86_64-apple-darwin16.7.0/ --disable-hardening --disable-rust --disable-proton CXXFLAGS='-fwrapv -fno-strict-aliasing -Werror -g'
make

Removing the WIP label because we could merge this as-is. It doesn't get MacOS compiling, but it makes significant headway (e.g. libsnark compiles with clang).

I don't understand the reverse_lock stuff (which wraps Boost's reverse_lock), in particular this comment:

To avoid those problems, simply swap the parent lock's contents with a dummy for the duration of the lock. That will ensure that any undefined behavior is caught at the call-site rather than the reverse lock's destruction.

Specifically I don't see how this guarantees to catch the incorrect behaviour of retaking a lock while its reverse-lock is held. The upstream commit is unenlightening.

Re: f79ccfb , we don't depend on qrencode — but I guess this change is harmless anyway. However, if we use --location and --fail in e32bacd , why aren't we also using it in f79ccfb ?

Re: e430b42 , we use a different version of BDB than upstream, so I can't tell whether this patch will apply correctly or is still needed.

I don't understand the reverse_lock stuff (which wraps Boost's reverse_lock), in particular this comment:

To avoid those problems, simply swap the parent lock's contents with a dummy for the duration of the lock. That will ensure that any undefined behavior is caught at the call-site rather than the reverse lock's destruction.

Specifically I don't see how this guarantees to catch the incorrect behaviour of retaking a lock while its reverse-lock is held. The upstream commit is unenlightening.

reverse_lock swaps the state of the provided lock with a dummy lock. Thus after the parent passes its lock in, its lock's internal state is a dummy state, and any "retaking a lock" the parent tries to do will not affect the reverse_lock (which now holds the real lock state inside its dummy lock). The reverse_lock constructor can then lock the state it took, and swap it back to the parent. A similar implementation-specific operation is performed inside Boost's reverse_lock (just taking the mutex instead of swapping the entire state).

What I can't figure out is why this would even result in catching undefined behaviour. AFAICT, if the parent calls lock.lock() after passing lock to reverse_lock (so dummy state is locked), and then the latter is destructed, the dummy state is swapped back into the dummy lock, which is then itself destructed at the end of the reverse_lock destructor, causing the dummy lock to be unlocked if it is currently owned (which it would be). So to me, it looks like this commit means that the parent retaking a lock while the reverse-lock is held becomes a no-op instead of UB. Maybe @theuni can shed some light on their comment?

Regardless, IMHO this approach is better than our version of the bugfix in 108650a (which just disables noexcept).

Re: f79ccfb , we don't depend on qrencode — but I guess this change is harmless anyway. However, if we use --location and --fail in e32bacd , why aren't we also using it in f79ccfb ?

We are? f79ccfbe700465f61e195ea87ec3c7e1b23ebf7c alters depends/builders/linux.mk to use curl, and then e32bacd416566ff3b7a3a157d7b5d675f28c9efc further alters it to use --location and --fail.

Re: e430b42 , we use a different version of BDB than upstream, so I can't tell whether this patch will apply correctly or is still needed.

I altered this commit to apply to our BDB version. This bug is still present in BDB 6.2.23; item 7 of the BDB 6.2.32 changelog suggests the problem has been fixed there.

The reverse lock is only used in scheduler.cpp which is a task manager + thread pool. We might be able to rewrite this to avoid the use of the reverse lock in the first place. Still, the commit says that it fixes a problem of throwing from the reverse lock destructor which cause the app to terminate so I'm inclined to include this.

With regards to the change to wallet.h, I think we could drop commit 9c720fb79db808234b26fba9de1f8a7a97e79236 "c++11: CAccountingEntry must be defined before use in a list". It's not doing anything useful other than removing a forward declaration by shifting a block of code around.

I haven't been able to test this on a Mac so can't comment on whether or not all the build changes achieve their goal or not.

The reverse lock is only used in scheduler.cpp which is a task manager + thread pool. We might be able to rewrite this to avoid the use of the reverse lock in the first place. Still, the commit says that it fixes a problem of throwing from the reverse lock destructor which cause the app to terminate so I'm inclined to include this.

If we did rewrite it, that would be for another PR, and should probably also be done after we pull in upstream's changes to it (for compatibility with other upstream PRs).

With regards to the change to wallet.h, I think we could drop commit 9c720fb "c++11: CAccountingEntry must be defined before use in a list". It's not doing anything useful other than removing a forward declaration by shifting a block of code around.

The rest of the commit message reads:

c++11ism. This fixes builds against libc++.

Thus it is necessary for building on at least MacOS 10.12, which requires using libc++ for C++11.

This has sufficient ACKs, but I have bumped it to 1.0.14 because it depends on #2696 (otherwise we won't be able to see any effect of this PR), which still needs some work.

☔ The latest upstream changes (presumably #2739) made this pull request unmergeable. Please resolve the merge conflicts.

Rebased on master to fix merge conflict.

This is blocked by the Gitian build failure at #2696 (comment) .

Rebased on master to fix merge conflicts.

#2696 has been merged.

@zkbot try

⌛ Trying commit c6b39fb with merge 33f7145fb216dbe18d7a424fbb91da9fc2685e61...

💔 Test failed - pr-try

Updated workers to all have curl.

@zkbot retry

⌛ Trying commit c6b39fb with merge b2399c1...

☀️ Test successful - pr-try
State: approved= try=True

Per the above test, this PR enables all currently-enabled dependencies to build on MacOS (before, after). It also makes changes that improve later parts of the build system, but that are not visible because they require changes to zcutil/build.sh (specifically, disabling hardening, and removing -Wno-builtin-declaration-mismatch). I will address these in a follow-up PR.

@zkbot r+

📌 Commit c6b39fb has been approved by str4d

⌛ Testing commit c6b39fb with merge d3ca270...

💔 Test failed - pr-merge

Timeout in prioritisetransaction.py on debian8, which seems like a transient failure.

@zkbot retry

⌛ Testing commit c6b39fb with merge 02c4467...

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 02c4467 to master...