Copilot commented on code in PR #427:
URL: https://github.com/apache/trafficserver-ci/pull/427#discussion_r3284788180
##########
docker/fedora44/build_openssl_h3_tools.sh:
##########
@@ -69,9 +71,9 @@ elif [ -e /etc/debian_version ]; then
echo
"+-------------------------------------------------------------------------+"
echo "| You probably need to run this, or something like this, for your
system: |"
echo "|
|"
- echo "| sudo apt -y install libev-dev libjemalloc-dev python2-dev
libxml2-dev |"
- echo "| sudo apt -y install libpython2-dev libc-ares-dev libsystemd-dev
|"
- echo "| sudo apt -y install libevent-dev libjansson-dev zlib1g-dev cargo
|"
+ echo "| sudo apt -y install libev-dev libjemalloc-dev python3-dev
libxml2-dev |"
+ echo "| sudo apt -y install libpython3-dev libc-ares-dev libsystemd-dev
|"
+ echo "| sudo apt -y install libevent-dev libjansson-dev zlib1g-dev
libpsl-dev |"
Review Comment:
The Debian/Ubuntu package install hint no longer includes `cargo`, but this
script invokes `cargo build` for quiche. The guidance will be incorrect for
users running this script outside the Docker image; add `cargo` (and any other
required Rust tooling) back to the apt install suggestion.
##########
docker/fedora44/Dockerfile:
##########
@@ -202,6 +202,7 @@ RUN <<EOF
cd /root/src/abi
git clone https://github.com/lvc/installer.git
cd installer
+ perl -0pi -e 's{qx/tar -xf archive\.tar\.gz/;}{qx/python3 -m tarfile -e
archive.tar.gz/;}' installer.pl
Review Comment:
The Perl one-liner rewrites installer.pl but doesn’t verify that the
substitution actually happened. If upstream changes the exact `qx/tar -xf
archive.tar.gz/;` snippet, this command will succeed while leaving GNU tar in
use (which the PR is trying to avoid), potentially reintroducing the ENOSYS
failure. Consider failing the build if the replacement didn’t occur (e.g.,
check the file content after the edit or have Perl assert a non-zero
substitution count).
##########
docker/fedora44/build_openssl_h3_tools.sh:
##########
@@ -36,6 +36,8 @@ WORKDIR="$(pwd)"
# OPENSSL_BRANCH is kept for compatibility with older local invocations.
OPENSSL_TAG=${OPENSSL_TAG:-${OPENSSL_BRANCH:-"openssl-3.5.5"}}
+QUICHE_TAG=${QUICHE_TAG:-"0.28.0"}
+CURL_TAG=${CURL_TAG:-"curl-8_20_0"}
NGHTTP3_TAG=${NGHTTP3_TAG:-"v1.15.0"}
Review Comment:
This PR is described as changing archive extraction to avoid GNU tar, but
this script also introduces new default component versions
(QUICHE_TAG/CURL_TAG) and updates the curl/quiche checkouts/build behavior. If
those version bumps are intentional, they should be called out in the PR
description (or split into a separate PR) because they can change the resulting
toolchain and affect CI behavior.
##########
docker/fedora44/build_boringssl_h3_tools.sh:
##########
@@ -67,9 +71,9 @@ elif [ -e /etc/debian_version ]; then
echo
"+-------------------------------------------------------------------------+"
echo "| You probably need to run this, or something like this, for your
system: |"
echo "|
|"
- echo "| sudo apt -y install libev-dev libjemalloc-dev python2-dev
libxml2-dev |"
- echo "| sudo apt -y install libpython2-dev libc-ares-dev libsystemd-dev
|"
- echo "| sudo apt -y install libevent-dev libjansson-dev zlib1g-dev cargo
|"
+ echo "| sudo apt -y install libev-dev libjemalloc-dev python3-dev
libxml2-dev |"
+ echo "| sudo apt -y install libpython3-dev libc-ares-dev libsystemd-dev
|"
+ echo "| sudo apt -y install libevent-dev libjansson-dev zlib1g-dev
libpsl-dev |"
Review Comment:
The Debian/Ubuntu package install hint no longer includes `cargo`, but this
script runs `cargo build` for quiche. Add `cargo` back to the apt install
suggestion so the printed guidance matches the actual dependencies when running
outside the Docker image.
##########
docker/fedora44/build_boringssl_h3_tools.sh:
##########
@@ -44,6 +44,10 @@ echo "Building boringssl H3 dependencies in ${WORKDIR}.
Installation will be don
CFLAGS=${CFLAGS:-"-O3 -g"}
CXXFLAGS=${CXXFLAGS:-"-O3 -g"}
BORINGSSL_PATH="${BASE}/boringssl"
+GO_VERSION=${GO_VERSION:-"1.26.2"}
+BORINGSSL_COMMIT=${BORINGSSL_COMMIT:-"c3ffc3300a9450cf8e396c7880be7c6cadc16a4a"}
+QUICHE_TAG=${QUICHE_TAG:-"0.28.0"}
+CURL_TAG=${CURL_TAG:-"curl-8_20_0"}
NGHTTP3_TAG=${NGHTTP3_TAG:-"v1.15.0"}
NGTCP2_TAG=${NGTCP2_TAG:-"v1.21.0"}
NGHTTP2_TAG=${NGHTTP2_TAG:-"v1.68.0"}
Review Comment:
This PR is described as avoiding GNU tar for source archive extraction, but
this script also changes several default pinned versions (GO_VERSION,
BORINGSSL_COMMIT, QUICHE_TAG, CURL_TAG). If those bumps are intentional, they
should be explicitly mentioned in the PR description (or split out) since they
can change build outputs and CI behavior beyond the tar workaround.
--
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]
