github-actions[bot] commented on code in PR #68689: URL: https://github.com/apache/doris/pull/68689#discussion_r4152389761
########## thirdparty/lance-install.sh: ########## @@ -0,0 +1,38 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Keep the check independent of the installed prefix: an external compilation image +# can carry old vars.sh alongside an ABI-compatible but behaviorally stale archive. +lance_c_install_fingerprint() ( + local definitions="$1" + local TP_DIR="${definitions}" + source "${definitions}/vars.sh" || return 1 + local patch_checksum + patch_checksum="$(cksum < "${definitions}/patches/lance-c-foyer.patch")" || return 1 + printf '%s\n' "${LANCE_C_SOURCE}" "${LANCE_C_MD5SUM}" "${patch_checksum}" Review Comment: [P2] Include the archive locator in this preflight fingerprint. An external tree can have this revision's `LANCE_C_SOURCE` and `LANCE_C_MD5SUM` but retain the old `LANCE_C_NAME`/`LANCE_C_DOWNLOAD`; this comparison passes, `build.sh` deletes the full `installed` prefix, and the downloader then rejects the old archive against the new MD5. Check both locator fields before deletion and add a mixed-vars fixture. This is distinct from the earlier missing-helper thread because every required file is present. ########## .github/workflows/build-thirdparty.yml: ########## @@ -82,6 +82,9 @@ jobs: run: | thirdparty/test/adbc-jni-config-test.sh + - name: Test Lance installation Review Comment: [P2] Trigger this harness for `build.sh` changes. The test extracts the dependency gate directly from `build.sh`, yet the `changes` filters omit that file; a PR changing only the gate makes both outputs false and skips `script_test`, including this new step. Add `build.sh` to `focused_test_changes`. The earlier thread about wiring the test is resolved, but its direct input remains outside the trigger. ########## thirdparty/build-thirdparty.sh: ########## @@ -2507,9 +2509,14 @@ build_lance_c() { env "${cargo_env[@]}" "${cargo_bin}" "${cargo_args[@]}" mkdir -p "${TP_INSTALL_DIR}/include" "${TP_INSTALL_DIR}/lib64" + # Invalidate before publishing either file so interrupted installs cannot reuse + # a matching marker with a partial header/archive pair. + rm -f "${TP_INSTALL_DIR}/lib64/.lance-c-fingerprint" rm -rf "${TP_INSTALL_DIR}/include/lance" cp -av include/lance "${TP_INSTALL_DIR}/include/" install_rust_archive "${BUILD_DIR}/release/liblance_c.a" + printf '%s\n' "${LANCE_C_INSTALL_FINGERPRINT}" > "${TP_INSTALL_DIR}/lib64/.lance-c-fingerprint.tmp" Review Comment: [P2] Verify the Foyer patch before publishing this fingerprint. `build.sh` accepts an external `download-thirdparty.sh` based only on file presence, but that script is the only place applying the patch. If it is stale while vars/helper/builder are current, `build_lance_c` builds plain upstream, copies nonempty headers/archive, writes this matching marker, and the postcheck accepts an install missing the Foyer APIs. Tie publication to the patched-source marker or otherwise validate the external downloader and installed API; cover that mixed-tree case. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
