Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24583 )
Change subject: KUDU-3788 an option to use pre-built thirdparty artifacts ...................................................................... Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/24583/14/thirdparty/prebuilt-utils.sh File thirdparty/prebuilt-utils.sh: http://gerrit.cloudera.org:8080/#/c/24583/14/thirdparty/prebuilt-utils.sh@172 PS14, Line 172: local plevel="${!plevel_var}" nit: `$version` is validated just above (errors if empty), but `$plevel` isn't. If a future component gets a `xxx_VERSION` but no `xxx_PATCHLEVEL`, this silently emits `name-version.p` (empty patch level) and we'd fetch/create a mis-named tarball rather than failing fast. Worth an `[ -z "$plevel" ]` guard symmetric with the version check? http://gerrit.cloudera.org:8080/#/c/24583/14/thirdparty/prebuilt-utils.sh@330 PS14, Line 330: return 0 This `return 0` (combined with the callers at lines 369 and 410 invoking this function as `if ! invoke_component_build ...`) silently swallows build failures. When a function is run in an `if !` condition, bash disables `set -e` for the *entire* call chain, so a failing `make` / `make install` inside `"$build_func"` does not abort; execution falls through to the unconditional `return 0`, which reports success. I verified this: set -e f() { false; echo reached; return 0; } if ! f; then echo err; fi # prints "reached", never "err", exit 0 Consequences in the build-from-source path of fetch_prebuilt_or_build: a broken build is treated as success, we then run create_prebuilt_tarball over an incomplete/empty $staging_root (tar succeeds), cache a corrupt tarball, extract it, and continue. Even a clean nonzero exit from the build func is masked by the trailing `return 0`. Can we drop `return 0` (let the function's status propagate) and, more importantly, avoid calling it under `if !` so `set -e` stays active during the component build? e.g. call it directly and check `$?`, or wrap the build in a subshell that re-enables `set -e`. -- To view, visit http://gerrit.cloudera.org:8080/24583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55b5b99fcbe60eec3f6ae19fa4f2e335f361d143 Gerrit-Change-Number: 24583 Gerrit-PatchSet: 14 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Fri, 31 Jul 2026 11:45:49 +0000 Gerrit-HasComments: Yes
