[macOS] Darwin build patches #2820

str4d · 2017-12-20T16:53:43Z

Part of #2246.

daira

utACK. Not tested at all, including on Mac.

str4d · 2017-12-22T01:14:29Z

This PR will go through several "try-commit" cycles in order for changes here to sequentially "unlock" further issues on the MacOS CI builder, making it progressively "less red" rather than immediately green 😄

@zkbot try

zkbot · 2017-12-22T01:14:33Z

⌛ Trying commit 4999cc4 with merge e19e88e4356b80d2b1d8e88f67b5fca33613542a...

str4d · 2017-12-22T01:17:12Z

configure.ac

+  if test x$BUILD_OS = xdarwin; then
+    # Xcode's ld (at least ld64-302.3) doesn't support -z
+    AX_CHECK_LINK_FLAG([[-Wl,-z,relro]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-z,relro"],[AC_MSG_WARN(Cannot enable RELRO)])
+    AX_CHECK_LINK_FLAG([[-Wl,-z,now]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,-z,now"],[AC_MSG_WARN(Cannot enable BIND_NOW)])


Added the security label because these changes need further investigation (perhaps in a separate issue though, if this is merged). It's currently necessary to not require these on Darwin (or clang will not build), but are there other flags we could try instead? Or are these flags maybe not present because they are not necessary? As a datapoint, Bitcoin Core does not require these flags.

zkbot · 2017-12-22T02:27:02Z

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

str4d · 2017-12-22T20:49:17Z

@zkbot try

zkbot · 2017-12-22T20:49:21Z

⌛ Trying commit 3363ceb with merge b118e1233f1f5d6df2c4609a9091aad60bef9003...

zkbot · 2017-12-22T21:58:31Z

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

str4d · 2017-12-22T22:11:10Z

@zkbot try

zkbot · 2017-12-22T22:11:14Z

⌛ Trying commit 20d0d42 with merge 9b26bac6bb07f0bcc3192840a755517d20255533...

zkbot · 2017-12-22T23:22:27Z

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

str4d · 2017-12-22T23:50:59Z

@zkbot try

zkbot · 2017-12-22T23:51:04Z

⌛ Trying commit 95dc0d4 with merge d403af4c827dafe35ba1b98e82f72ae1d94a1182...

str4d · 2017-12-23T00:32:11Z

The next build error comes from a warning:

In file included from txdb.cpp:6:
In file included from ./txdb.h:9:
In file included from ./coins.h:9:
In file included from ./compressor.h:9:
In file included from ./primitives/transaction.h:20:
In file included from ./zcash/JoinSplit.hpp:8:
./zcash/IncrementalMerkleTree.hpp:132:16: error: instantiation of variable 'libzcash::IncrementalMerkleTree<29, libzcash::SHA256Compress>::emptyroots' required here, but no definition is available [-Werror,-Wundefined-var-template]
        return emptyroots.empty_root(Depth);
               ^
txdb.cpp:76:40: note: in instantiation of member function 'libzcash::IncrementalMerkleTree<29, libzcash::SHA256Compress>::empty_root' requested here
    if (rt == ZCIncrementalMerkleTree::empty_root()) {
                                       ^
./zcash/IncrementalMerkleTree.hpp:140:42: note: forward declaration of template entity is here
    static EmptyMerkleRoots<Depth, Hash> emptyroots;
                                         ^
./zcash/IncrementalMerkleTree.hpp:132:16: note: add an explicit instantiation declaration to suppress this warning if 'libzcash::IncrementalMerkleTree<29, libzcash::SHA256Compress>::emptyroots' is explicitly instantiated in another translation unit
        return emptyroots.empty_root(Depth);
               ^
1 error generated.

The corresponding zcash-apple patch disables the warning, which seems to be a common approach. But there may be a way to refactor this to avoid the warning.

zkbot · 2017-12-23T01:02:24Z

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

str4d · 2017-12-23T01:04:00Z

@zkbot try

zkbot · 2017-12-23T01:04:07Z

⌛ Trying commit a3731f2 with merge 00cabf5a85c25c5938d7065279379a76f25d4cb7...

zkbot · 2017-12-23T02:06:43Z

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

str4d · 2017-12-23T10:29:07Z

@zkbot try

str4d · 2018-03-28T16:32:05Z

Rebased the branch to bring in the improved MerklePath serialization commit. I also collected the UniValue uint64_t commits at the end, so it is easier to replace them if we decide to instead patch UniValue.

…iValue size_t is ambiguous for serialization, and UniValue doesn't have an API for handling this internally.

str4d · 2018-03-29T15:19:04Z

Force-pushed the final three commits to use static_cast<uint64_t> instead of (uint64_t).

@zkbot try

zkbot · 2018-03-29T15:19:12Z

⌛ Trying commit b96f7d6 with merge 1f8fcb515313a67c99d073fd1d06a1ad7e77a93b...

zkbot · 2018-03-29T16:46:47Z

💔 Test failed - pr-try

str4d · 2018-03-30T16:15:39Z

The one failure was transient (ccache tripping up). All other supported builders passed, and the MacOS builder remains at the linker error.

I am inclined to merge this as-is, handling the UniValue casts at their source rather than inside the UniValue code.

braddmiller · 2018-03-30T16:42:11Z

I built and ran this on OSX High Sierra earlier this week. Just tried to build it again with --disable-libs flag passed resulting in the following error:

clang: error: argument unused during compilation: '-pie' [-Werror,-Wunused-command-line-argument]
make[2]: *** [zcashd] Error 1
make[2]: *** Waiting for unfinished jobs....
mv -f test/.deps/test_bitcoin-accounting_tests.Tpo test/.deps/test_bitcoin-accounting_tests.Po
mv -f wallet/test/.deps/test_test_bitcoin-wallet_tests.Tpo wallet/test/.deps/test_test_bitcoin-wallet_tests.Po
mv -f test/.deps/test_bitcoin-rpc_wallet_tests.Tpo test/.deps/test_bitcoin-rpc_wallet_tests.Po
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

str4d · 2018-03-30T22:54:01Z

@braddmiller could you please try again with -j1 and provide a few lines above the error (so we can see what command is triggering it)? I have just run a manual build on our MacOS CI worker with --disable-libs, and it appears to build fine (with some warnings). The CI worker is using clang-900.0.38.

braddmiller · 2018-03-30T22:59:43Z

Command run: ./zcutil/build.sh --disable-rust --disable-libs -j1

Result:

/Library/Developer/CommandLineTools/usr/bin/ranlib libbitcoin_wallet.a
/bin/sh ../libtool  --tag=CXX   --mode=link /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/share/../native/bin/ccache /Library/Developer/CommandLineTools/usr/bin/clang++ -mmacosx-version-min=10.8 -stdlib=libc++ -std=c++11  -Wformat -Wformat-security -Wstack-protector -fstack-protector-all -Werror -fPIE -pipe -O2 -g -fwrapv -fno-strict-aliasing  -pthread   -pie  -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/share/../lib  -Wl,-headerpad_max_install_names -Wl,-dead_strip -o zcashd zcashd-bitcoind.o  libbitcoin_server.a libbitcoin_common.a univalue/libunivalue.la libbitcoin_util.a crypto/libbitcoin_crypto.a libzcash.a snark/libsnark.a ./leveldb/libleveldb.a ./leveldb/libmemenv.a secp256k1/libsecp256k1.la libbitcoin_zmq.a -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib -lzmq -lstdc++ libbitcoin_wallet.a -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/share/../lib -lboost_system -lboost_filesystem -lboost_program_options -lboost_thread -lboost_chrono -ldb_cxx-6.2 -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib -lssl -lcrypto -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib -lcrypto -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib -levent_pthreads -levent -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib -levent crypto/libbitcoin_crypto.a -lgmp -lgmpxx -lboost_system -lcrypto -lsodium
libtool: link: /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/share/../native/bin/ccache /Library/Developer/CommandLineTools/usr/bin/clang++ -mmacosx-version-min=10.8 -stdlib=libc++ -std=c++11 -Wformat -Wformat-security -Wstack-protector -fstack-protector-all -Werror -fPIE -pipe -O2 -g -fwrapv -fno-strict-aliasing -pthread -pie -Wl,-headerpad_max_install_names -Wl,-dead_strip -o zcashd zcashd-bitcoind.o -Wl,-bind_at_load  -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/share/../lib libbitcoin_server.a libbitcoin_common.a univalue/.libs/libunivalue.a -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.4.0/share/../lib libbitcoin_util.a libzcash.a snark/libsnark.a ./leveldb/libleveldb.a ./leveldb/libmemenv.a secp256k1/.libs/libsecp256k1.a libbitcoin_zmq.a -L/Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libzmq.a -lpthread -lstdc++ libbitcoin_wallet.a -lboost_filesystem -lboost_program_options -lboost_thread -lboost_chrono -ldb_cxx-6.2 -lssl /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libevent_pthreads.a /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libevent.a crypto/libbitcoin_crypto.a /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libgmpxx.a /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libgmp.a -lboost_system -lcrypto /Users/bradleymiller/Development/zcash/depends/x86_64-apple-darwin17.5.0/lib/libsodium.a -pthread
clang: error: argument unused during compilation: '-pie' [-Werror,-Wunused-command-line-argument]
make[2]: *** [zcashd] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

str4d · 2018-03-30T23:22:34Z

Per @braddmiller elsewhere, the version of clang used above was Apple LLVM version 9.1.0 (clang-902.0.39.1). Looks like an Xcode update has enabled newer errors. We should update our CI worker as well. There's no need to support older Xcode versions, given that we currently don't support any 😄

mdr0id

I just ran: ./build.sh --disable-rust --disable-libs -j1 with no issues.

Another reference point for CI worker update:

➜ src git:(darwin-build-patches) clang --version Apple LLVM version 9.0.0 (clang-900.0.39.2) Target: x86_64-apple-darwin17.2.0 Thread model: posix InstalledDir: /Library/Developer/CommandLineTools/usr/bin

mdr0id · 2018-04-01T14:01:04Z

Tested ACK: (see above comment for details related to clang ver)

daira

utACK

daira · 2018-04-03T19:26:42Z

src/wallet/asyncrpcoperation_mergetoaddress.cpp

@@ -825,10 +825,10 @@ UniValue AsyncRPCOperation_mergetoaddress::perform_joinsplit(
    UniValue arrInputMap(UniValue::VARR);
    UniValue arrOutputMap(UniValue::VARR);
    for (size_t i = 0; i < ZC_NUM_JS_INPUTS; i++) {
-        arrInputMap.push_back(inputMap[i]);
+        arrInputMap.push_back(static_cast<uint64_t>(inputMap[i]));


I would mildly prefer the "extend UniValue to support size_t" approach, but I don't object to these casts since we can clean it up later.

daira · 2018-04-03T19:29:13Z

src/zcash/IncrementalMerkleTree.hpp

@@ -21,8 +21,38 @@ class MerklePath {

    template <typename Stream, typename Operation>
    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
-        READWRITE(authentication_path);
-        READWRITE(index);
+        std::vector<std::vector<unsigned char>> pathBytes;


What's the status of this: can this PR be merged as-is without #3107?

str4d · 2018-04-03T19:33:35Z

Alright, let's merge this, and address any subsequent MacOS issues in follow-up PRs. Thanks everyone!

@zkbot r+

zkbot · 2018-04-03T19:33:35Z

📌 Commit b96f7d6 has been approved by str4d

zkbot · 2018-04-03T19:44:27Z

⌛ Testing commit b96f7d6 with merge 599c847...

[macOS] Darwin build patches Part of #2246.

zkbot · 2018-04-03T21:08:53Z

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 599c847 to master...

str4d added O-macos Operating system: macOS portability labels Dec 20, 2017

str4d mentioned this pull request Dec 20, 2017

add OpenMP compile flags based on availability #2805

Closed

daira previously approved these changes Dec 21, 2017

View reviewed changes

str4d added the I-SECURITY Problems and improvements related to security. label Dec 22, 2017

str4d commented Dec 22, 2017

View reviewed changes

str4d added the work-in-progress label Dec 23, 2017

str4d force-pushed the darwin-build-patches branch from b9ad57b to 7a84046 Compare March 28, 2018 16:29

str4d added 3 commits March 28, 2018 19:51

Cast ZCIncrementalMerkleTree::size() to uint64_t before passing to Un…

8e95a12

…iValue size_t is ambiguous for serialization, and UniValue doesn't have an API for handling this internally.

rpcwallet.cpp: Cast size_t to uint64_t before passing to UniValue

4ff92bb

wallet: Cast size_t to uint64_t before passing to UniValue

b96f7d6

str4d force-pushed the darwin-build-patches branch from 7a84046 to b96f7d6 Compare March 29, 2018 15:18

str4d requested a review from mdr0id March 30, 2018 16:16

mdr0id reviewed Apr 1, 2018

View reviewed changes

str4d mentioned this pull request Apr 3, 2018

[macOS] fix -Wreturn-type error in CurrentEpoch (clang-902.0.39.1) #3138

Closed

daira approved these changes Apr 3, 2018

View reviewed changes

str4d removed request for bitcartel and charlieok April 3, 2018 19:32

zkbot added a commit that referenced this pull request Apr 3, 2018

Auto merge of #2820 - str4d:darwin-build-patches, r=str4d

599c847

[macOS] Darwin build patches Part of #2246.

zkbot merged commit b96f7d6 into zcash:master Apr 3, 2018

str4d deleted the darwin-build-patches branch April 3, 2018 21:22

str4d mentioned this pull request Sep 19, 2019

fix type ambiguity errors on non-linux platforms #1800

Closed

[macOS] Darwin build patches #2820

[macOS] Darwin build patches #2820

Conversation

str4d commented Dec 20, 2017 • edited Loading

daira left a comment

Choose a reason for hiding this comment

str4d commented Dec 22, 2017

zkbot commented Dec 22, 2017

str4d Dec 22, 2017

Choose a reason for hiding this comment

zkbot commented Dec 22, 2017

str4d commented Dec 22, 2017

zkbot commented Dec 22, 2017

zkbot commented Dec 22, 2017

str4d commented Dec 22, 2017

zkbot commented Dec 22, 2017

zkbot commented Dec 22, 2017

str4d commented Dec 22, 2017

zkbot commented Dec 22, 2017

str4d commented Dec 23, 2017

zkbot commented Dec 23, 2017

str4d commented Dec 23, 2017

zkbot commented Dec 23, 2017

zkbot commented Dec 23, 2017

str4d commented Dec 23, 2017

str4d commented Mar 28, 2018

str4d commented Mar 29, 2018

zkbot commented Mar 29, 2018

zkbot commented Mar 29, 2018

str4d commented Mar 30, 2018

braddmiller commented Mar 30, 2018

str4d commented Mar 30, 2018

braddmiller commented Mar 30, 2018

str4d commented Mar 30, 2018

mdr0id left a comment

Choose a reason for hiding this comment

mdr0id commented Apr 1, 2018

daira left a comment

Choose a reason for hiding this comment

daira Apr 3, 2018

Choose a reason for hiding this comment

daira Apr 3, 2018

Choose a reason for hiding this comment

str4d commented Apr 3, 2018

zkbot commented Apr 3, 2018

zkbot commented Apr 3, 2018

zkbot commented Apr 3, 2018

str4d commented Dec 20, 2017 •

edited

Loading