Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18770 )
Change subject: KUDU-3374 Add support for M1 and macOS Monterey ...................................................................... Patch Set 13: Code-Review+1 (8 comments) overall looks good to me, just a few nits http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt@168 PS11, Line 168: > Fixed: Previously I couldn't get kudu to build with glog 0.6.0 and gcc. The Thank you for the clarification! http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/security/test/mini_kdc.cc File src/kudu/security/test/mini_kdc.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/security/test/mini_kdc.cc@108 PS11, Line 108: arm > Homebrew uses different install prefix on intel than on arm macs. On intel Indeed, it seems they changes the location. It seems they just realized that /opt is a better one once they started working on arm port just recently, and they still use /usr/local just for backward compatibility on Intel-based macs. I guess homebrew is influenced and borrows some stuff from MacPorts, and vice-versa: in recent versions, MacPorts installs its stuff under /opt/local :) http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/tablet/concurrent_btree.h File src/kudu/tablet/concurrent_btree.h: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/tablet/concurrent_btree.h@96 PS11, Line 96: for (int i = 0; i < size; i += CACHELINE_SIZE) { > Without the if check, during sanitizer builds, the following error is prese Heh, that might be pointing to a real issue, IIUC. It seems there is a programming error: something about calling PrefetchMemory() on a nullptr. I'd suggest adding DCHECK() here and moving the 'if()' condition out of this method to the corresponding call sites, so PrefetchMemory() wouldn't be called with nullptr. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/async_logger.h File src/kudu/util/async_logger.h: PS11: > The upper level google::base::Logger Write api changed. We must include it Ah, indeed -- I missed that the Write() method overrides one from google::base::Logger's API. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-definitions.sh@741 PS11, Line 741: --disable-pop3 > Fix: In the previous revision, to get a successful build on arm macs, in re All right, thanks for the explanation. http://gerrit.cloudera.org:8080/#/c/18770/13/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/18770/13/thirdparty/build-definitions.sh@438 PS13, Line 438: nit: wrong indent? http://gerrit.cloudera.org:8080/#/c/18770/13/thirdparty/build-definitions.sh@453 PS13, Line 453: -DCMAKE_CXX_FLAGS="$EXTRA_CXXFLAGS -I$PREFIX/include" \ : -DCMAKE_EXE_LINKER_FLAGS="$EXTRA_LDFLAGS $EXTRA_LIBS -L$PREFIX/lib -Wl,-rpath,$PREFIX/lib" \ : -DCMAKE_MODULE_LINKER_FLAGS="$EXTRA_LDFLAGS $EXTRA_LIBS -L$PREFIX/lib -Wl,-rpath,$PREFIX/lib" \ : nit: wrong indent ? http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-thirdparty.sh@183 PS11, Line 183: for homebrew_openssl_dir in "${homebrew_openssl_dirs[@]}"; do : if [ -d $homebrew_openssl_dir ]; then : OPENSSL_CFLAGS="-I$homebrew_openssl_dir/include" : OPENSSL_LDFLAGS="-L$homebrew_openssl_dir/lib" : fi > This is only a macos related change, linux variant is unchanged. Yes, I understand that it's macOS-specific. I just noticed that maybe we may need to cleanup this piece a bit since I know that OPENSSL_XXXFLAGS and the actual process of finding OpenSSL to build, say, curl are sort of independent, and I recently hit that issue on my mac laptop. Anyways, that's not a concern in this patch, and addressing those concerns should be separated into its own changelist. -- To view, visit http://gerrit.cloudera.org:8080/18770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9877f95340b969308c317a6bac50665ff78e329e Gerrit-Change-Number: 18770 Gerrit-PatchSet: 13 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 03 Aug 2022 19:17:14 +0000 Gerrit-HasComments: Yes
