Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24060 )
Change subject: [WIP] KUDU-3740 1/n Upgrade to LLVM 16.0.6 ...................................................................... Patch Set 2: (8 comments) A first glance over the patch. http://gerrit.cloudera.org:8080/#/c/24060/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24060/2//COMMIT_MSG@18 PS2, Line 18: + LLVM 16 libc++ requires C++20. Ubuntu 18 g++ is no longer : able to compile it. So we check whether the given compiler : is C++20 compatible and fall back to the already built : clang if it is not. I'm not sure I understand where do you expect to have an 'already built CLANG' in this case. Is this about requiring clang package on Ubuntu18 once this patch is pushed to the repo? Would CLANG9 on Ubuntu18 be enough to build LLVM16 libc++? AFAIK, CLANG doesn't support all of the C++20 standard yet: https://clang.llvm.org/cxx_status.html Is this to say that LLVM16 libc++ requires some of the C++20 features supported by the compiler? >From the other side, I'm suspecting we are about to drop support for Ubuntu18 >soon, maybe even in 1.19.0 release. FWIW, I know that Impala has just dropped >support of Ubuntu18 in the main branch: >https://github.com/apache/impala/commit/bcc9c1526daab9caa00397bde574b37d453a5b32 http://gerrit.cloudera.org:8080/#/c/24060/2/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/24060/2/build-support/jenkins/build-and-test.sh@209 PS2, Line 209: cleanup_thirdparty() { : git clenan -xfd $THIRDPARTY_DIR : } Perhaps, a better approach is to check for the presence of a special tag in the commit description (e.g., CLEAN_THIRDPARTY or similar). It's similar to handling of already existing DONT_BUILD label: if the tag is present in the very beginning of a line the commit description, then do this clean up before and after the build. Please move this into a separate changelist, and then you could re-base this patch on top of that one and use the newly introduced functionality for this patch. Otherwise, this unconditional cleanup might eventually slip trough and be committed with the rest of the patch, and then we will wasting compute resources on rebuilding 3rd-party even if we don't need doing so. http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/build-definitions.sh@172 PS2, Line 172: return Is this intentional? If yes, then maybe it's a good idea to remove all the irrelevant code in build_libcxx() function then? http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/download-thirdparty.sh@349 PS2, Line 349: 3 nit: we could start with 1 here; for _PATCHLEVEL, the important piece is to increase the number when progressing at the same component's version; it doesn't have to be the number of currently applied patches http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/package-llvm.sh File thirdparty/package-llvm.sh: http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/package-llvm.sh@28 PS2, Line 28: # 1. Unpack the llvm tarball : # 9. Unpack the IWYU tarball in tools/clang/tools/include-what-you-use : # 10. Create new tarball from the resulting source tree Fix the numbering? http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/package-llvm.sh@37 PS2, Line 37: ${VERSION:-} nit for here and elsewhere: why to use the default assignment for the variable -- wouldn't it automatically evaluate to an empty string if it's not defined? http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/package-llvm.sh@44 PS2, Line 44: tar xf What is this? http://gerrit.cloudera.org:8080/#/c/24060/2/thirdparty/package-llvm.sh@67 PS2, Line 67: - nit: remove the dash to keep the same notion for tar arguments across the script? -- To view, visit http://gerrit.cloudera.org:8080/24060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd06007924ae7810adb760660905abe387a9e5d7 Gerrit-Change-Number: 24060 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 03 Mar 2026 04:11:45 +0000 Gerrit-HasComments: Yes
