Marton Greber 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: (24 comments) http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@26 PS11, Line 26: location > consistent ? Done http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@30 PS11, Line 30: ng upgrade it now links against libatomic in TSAN builds. In : dist > how about: Done http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@32 PS11, Line 32: was > drop 'fix' ? Done http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@32 PS11, Line 32: f new TSAN races : came up with LLVM regexes, added those to the sanitizer suppressi > How about: Done http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@36 PS11, Line 36: > races Done http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@43 PS11, Line 43: [7] https://github.com/Tencent/rapidjson/pull/757 : [8] https://github.com/google/glog/issues/54 : : Change-Id: I9877f95340b969308c317a6bac50665ff78e329e > Could you change this to the corresponding gerrit review links? Done 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: > Why? What has changed in this patch so that now we explicitly use LLVM ins Fixed: Previously I couldn't get kudu to build with glog 0.6.0 and gcc. The reason has been identified. The build definition for glog had missing linker flags. In the new patch revision, the glog linker flags are fixed. This code section is removed. http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt@170 PS11, Line 170: Compiler flags > I'm not sure I understand the reasoning here. Could you please rephrases t Fixed: Removed this piece of code. http://gerrit.cloudera.org:8080/#/c/18770/11/cmake_modules/FindGLog.cmake File cmake_modules/FindGLog.cmake: http://gerrit.cloudera.org:8080/#/c/18770/11/cmake_modules/FindGLog.cmake@31 PS11, Line 31: NO_SYSTEM_ENVIRONMENT_PATH) : find_library(GLOG_STATIC_LIB libglog.a : NO_CMAKE_SYSTEM_PATH : NO_SYSTEM_ENVIRONMENT_PATH) : > Could you add a comment explaining why to override the default settings for Fixed: Piece of code, which was experimental. In the end, it is not needed. I removed it. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/common/row_operations-test.cc@302 PS11, Line 302: reinterpret_cast > Could this be static_cast<> here as well similar to line 330? Fixed: gcc did not like the static_cast, so I changed both occurrences to reinterpret_cast. Thereby getting it to work with gcc and clang as well. 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 > Is it indeed something related to the architecture, not the version of home Homebrew uses different install prefix on intel than on arm macs. On intel it is /usr/local and on arm it is /opt/homebrew (https://docs.brew.sh/Installation). It is indeed architectural. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/server/diagnostics_log.cc@21 PS11, Line 21: #include <cstdint> > nit: remove the extra empty line Done 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) { > Isn't it a programming error if this addr is nullptr? If so, maybe add DCH Without the if check, during sanitizer builds, the following error is presented: "concurrent_btree.h:96:53: runtime error: applying non-zero offset 64 to null pointer". I tried adding a DCHECK(), but the error stayed. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/async_logger.h File src/kudu/util/async_logger.h: PS11: > Could you separate this and the corresponding changes in async_logger.cc in The upper level google::base::Logger Write api changed. We must include it in this change list to get a successful build. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/logging.h@21 PS11, Line 21: #include <iosfwd> > nit: remove the extra line Done http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.h File src/kudu/util/pstack_watcher.h: PS11: > Could you separate changes in this and in the corresponding cc file this in Removed pstach_watcher from this change list, as it is not essential. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc File src/kudu/util/pstack_watcher.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc@20 PS11, Line 20: #include <unistd. > nit: move this into the C++ headers section Removed pstack_watcher from this change list, as it is not essential. http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc@170 PS11, Line 170: return > nit: wrong indent Removed pstack_watcher from this change list, as it is not essential. 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 > Why is this needed? IIRC, this flag is needed to specify custom location o Fix: In the previous revision, to get a successful build on arm macs, in release config, the curl upgrade was necessary. However as the compiler fix is introduced in this revision, we can use the system compiler as usual. Using the system compiler on arm mac (Xcode 13.1) resulted in successful release build, without the curl upgrade. Therefore I removed the curl upgrade from the change list. 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 > What if both directories are present? Is the end result deterministic? This is only a macos related change, linux variant is unchanged. Homebrew uses different directories according to the mac architecture. It is /usr/local on intel and /opt/homebrew on arm macs (https://docs.brew.sh/Installation). Basically both directories shouldn't exist. The above changes only account for the hombrew/mac architecture location. Existing logic is not changed. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh@a310 PS11, Line 310: > I didn't see this file removed among the list of updated files. Fix: In the previous revision, to get a successful build on arm macs, in release config, the curl upgrade was necessary. However as the compiler fix is introduced in this revision, we can use the system compiler as usual. Using the system compiler on arm mac (Xcode 13.1) resulted in successful release build, without the curl upgrade. Therefore I removed the curl upgrade from the change list. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh@173 PS11, Line 173: 2 > Why 0? Usually, this stands for the number of patches applied. Fix: Good catch, changed it to reflect the number of actual patches. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/preflight.py File thirdparty/preflight.py: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/preflight.py@141 PS11, Line 141: homebrew_openssl_dirs=["/usr/local/opt/openssl/include", "/opt/homebrew/opt/[email protected]/include"] : for homebrew_openssl_dir in homebrew_openssl_dirs: : if os.path.isdir(homebrew_openssl_dir): > If this a part of curl-related changes? If so, could you separate this int This change is not related to curl. This is only a macos related change, linux variant is unchanged(basically on linux we shouldn't hit the else branch as that contains hombrew related paths). Homebrew uses different directories according to the mac architecture. It is /usr/local on intel and /opt/homebrew on arm macs (https://docs.brew.sh/Installation). Basically both directories shouldn't exist. The above changes only account for the hombrew/mac architecture location. Existing logic is not changed. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/vars.sh File thirdparty/vars.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/vars.sh@125 PS11, Line 125: CURL_VERSION=7.68.0 > Why to upgrade curl as well? Could you move this into its own separate cha Fix: In the previous revision, to get a successful build on arm macs, in release config, the curl upgrade was necessary. However as the compiler fix is introduced in this revision, we can use the system compiler as usual. Using the system compiler on arm mac (Xcode 13.1) resulted in successful release build, without the curl upgrade. Therefore I removed the curl upgrade from the change list. -- 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: Mon, 01 Aug 2022 17:12:57 +0000 Gerrit-HasComments: Yes
