Copilot commented on code in PR #13190:
URL: https://github.com/apache/trafficserver/pull/13190#discussion_r3284581477
##########
tools/build_openssl_h3_tools.sh:
##########
@@ -27,17 +27,22 @@ readonly WORKDIR
cd "${WORKDIR}"
-# Update this as the draft we support updates.
-OPENSSL_BRANCH=${OPENSSL_BRANCH:-"openssl-3.1.7+quic"}
+# 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"}
+NGTCP2_TAG=${NGTCP2_TAG:-"v1.21.0"}
+NGHTTP2_TAG=${NGHTTP2_TAG:-"v1.68.0"}
# Set these, if desired, to change these to your preferred installation
# directory
BASE=${BASE:-"/opt/h3-tools-openssl"}
OPENSSL_BASE=${OPENSSL_BASE:-"${BASE}/openssl-quic"}
Review Comment:
`OPENSSL_BASE`/install path still uses the `openssl-quic` name (e.g.,
`${BASE}/openssl-quic`), but the script now clones/builds upstream OpenSSL
rather than quictls. Renaming this default path (and related echo text) would
avoid implying quictls is involved and reduce confusion when multiple
toolchains are installed.
##########
tools/build_h3_tools.sh:
##########
@@ -20,9 +20,10 @@
# limitations under the License.
-# The whole idea is to end up with two set of tools, a borinssgl toolset and an
-# openssl one. The first one can be used to build
ATS+Boringssl+quiche(borinssl) while the
-# later one will give the base to build ATS on top of
openssl/quictls+quiche(openssl/quictls).
+# The whole idea is to end up with two set of tools, a boringssl toolset and an
+# openssl one. The first one can be used to build
ATS+Boringssl+quiche(boringssl)
+# while the later one will give the base to build ATS on top of
Review Comment:
The introductory comment has a couple of typos/grammar issues: "two set of
tools" should be "two sets of tools", and "later" should be "latter" (it refers
to the second toolset). Cleaning this up helps avoid confusion for readers.
##########
tools/build_boringssl_h3_tools.sh:
##########
@@ -137,17 +142,31 @@ if [ $retVal -eq 1 ]; then
fi
set -e
+# Check compiler flags before passing them to CMake. GCC errors on some
+# Clang-only -Wno-error= flags, including -Wcharacter-conversion.
Review Comment:
The new comment mentions "-Wcharacter-conversion" but the flag you’re
actually probing/adding is `-Wno-error=character-conversion`. Consider updating
the comment to match the flag being tested so it’s clear what GCC is expected
to reject.
##########
tools/build_openssl_h3_tools.sh:
##########
@@ -90,9 +95,9 @@ else
fi
echo "Building OpenSSL with QUIC support"
-[ ! -d openssl-quic ] && git clone -b ${OPENSSL_BRANCH} --depth 1
https://github.com/quictls/openssl.git openssl-quic
-cd openssl-quic
-./config enable-tls1_3 --prefix=${OPENSSL_PREFIX}
+[ ! -d openssl ] && git clone -b ${OPENSSL_TAG} --depth 1
https://github.com/openssl/openssl.git openssl
+cd openssl
+./config enable-tls1_3 --prefix=${OPENSSL_PREFIX} --libdir=lib
${MAKE} -j ${num_threads}
sudo ${MAKE} install_sw
Review Comment:
The comment says OpenSSL will install into lib or lib64 depending on
architecture, but the script now forces `./config ... --libdir=lib`, so the
install directory will always be `lib`. Either drop the `--libdir=lib` override
(and keep the lib/lib64 detection), or update the comment + detection logic to
match the forced layout to avoid confusion for users relying on lib64 installs.
--
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]
