Copilot commented on code in PR #13182:
URL: https://github.com/apache/gluten/pull/13182#discussion_r4220261641
##########
cpp/velox/CMakeLists.txt:
##########
@@ -495,8 +495,37 @@ else()
target_link_libraries(velox PUBLIC external::simdjson)
endif()
+# Restore the platform's default suffixes before looking for optional dynamic
+# dependencies such as librdkafka.
set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES_BCK})
+# Find and link librdkafka for Kafka streaming support
+if(ENABLE_KAFKA)
+ target_compile_definitions(velox PUBLIC ENABLE_KAFKA)
+ find_package(RdKafka CONFIG QUIET)
+ if(RdKafka_FOUND AND TARGET RdKafka::rdkafka++)
+ message(STATUS "Found RdKafka: ${RdKafka_DIR}")
+ target_link_libraries(velox PUBLIC RdKafka::rdkafka++)
+ else()
+ # Try to find librdkafka using pkg-config
+ find_package(PkgConfig QUIET)
+ if(PKG_CONFIG_FOUND)
+ pkg_check_modules(RDKAFKA QUIET IMPORTED_TARGET rdkafka++)
+ if(RDKAFKA_FOUND)
+ message(STATUS "Found librdkafka via pkg-config: ${RDKAFKA_LIBRARIES}")
+ target_link_libraries(velox PUBLIC PkgConfig::RDKAFKA)
+ else()
+ message(FATAL_ERROR "ENABLE_KAFKA is ON but librdkafka was not found.")
+ endif()
+ else()
+ message(
+ FATAL_ERROR
+ "ENABLE_KAFKA is ON but pkg-config was not found to locate
librdkafka."
+ )
+ endif()
+ endif()
+endif()
Review Comment:
The discovery/link logic currently requires the C++ wrapper
(`RdKafka::rdkafka++` / `rdkafka++` pkg-config). Some distributions/package
builds ship only the C library target/pc file (`RdKafka::rdkafka` / `rdkafka`),
which would make `ENABLE_KAFKA=ON` fail even though librdkafka is installed.
Consider accepting/linking the C target as a fallback (and only requiring
rdkafka++ if the code truly depends on the C++ wrapper).
##########
dev/docker/Dockerfile.centos9-dynamic-build:
##########
@@ -35,7 +35,7 @@ RUN set -ex; \
# clang-tidy, installed here so its version is pinned to the image, not to
each CI run.
dnf install -y --setopt=install_weak_deps=False clang-tools-extra; \
echo "check_certificate = off" >> ~/.wgetrc; \
- yum install -y java-${JAVA_VERSION}-openjdk-devel patch wget git perl; \
+ yum install -y java-${JAVA_VERSION}-openjdk-devel patch wget git perl
librdkafka-devel; \
Review Comment:
This installs `librdkafka-devel` unconditionally in the image, even though
Kafka support is an optional build toggle. If the intent is only to support
`--enable_kafka=ON` builds, consider gating this behind a build arg or leaving
it to the build job/setup scripts when Kafka is enabled, to reduce base image
size and dependency surface area.
##########
gluten-ut/tests/test_kafka_setup.py:
##########
@@ -0,0 +1,137 @@
+# 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.
+
+"""Verify non-vcpkg Kafka package setup without installing system packages."""
+
+import os
+from pathlib import Path
+import re
+import subprocess
+import sys
+import tempfile
+import unittest
+
+SETUP_DIR = Path(__file__).resolve().parents[2] / "ep/build-velox/src"
+
+
+class KafkaSetupTest(unittest.TestCase):
+ def setUp(self):
+ self.temp_dir = tempfile.TemporaryDirectory()
+ self.addCleanup(self.temp_dir.cleanup)
+ self.work_dir = Path(self.temp_dir.name)
+ (self.work_dir / "scripts").mkdir()
+ self.package_log = self.work_dir / "packages.log"
+ self.package_log.touch()
+
+ def run_shell(self, commands):
+ # Run the real sed with native in-place syntax, so both Linux and macOS
+ # setup transformations can be exercised on either development host.
+ inplace_args = "-i ''" if sys.platform == "darwin" else "-i"
+ prelude = """
+set -eu
+SUDO=""
+sed() {
+ if [ "$1" = "-i" ]; then
+ shift
+ if [ "$1" = "" ]; then shift; fi
+ command sed INPLACE_ARGS "$@"
+ else
+ command sed "$@"
+ fi
+}
+apt() { printf '%s\\n' "$@" >> "$PACKAGE_LOG"; }
+dnf_install() { apt "$@"; }
+brew() { apt "$@"; }
+""".replace("INPLACE_ARGS", inplace_args)
+ result = subprocess.run(
+ ["bash", "-c", prelude + commands],
+ cwd=self.work_dir,
+ env={**os.environ, "PACKAGE_LOG": str(self.package_log)},
+ capture_output=True,
+ text=True,
+ )
+ self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
Review Comment:
This test hard-depends on `bash` and common Unix utilities; if the unit test
suite can run on non-Unix hosts (e.g., Windows runners), it will fail. Consider
skipping the test when `bash` is unavailable / on Windows (e.g., using
`unittest.skipIf` with a `shutil.which('bash')` check).
##########
cpp/velox/CMakeLists.txt:
##########
@@ -495,8 +495,37 @@ else()
target_link_libraries(velox PUBLIC external::simdjson)
endif()
+# Restore the platform's default suffixes before looking for optional dynamic
+# dependencies such as librdkafka.
set(CMAKE_FIND_LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES_BCK})
+# Find and link librdkafka for Kafka streaming support
+if(ENABLE_KAFKA)
+ target_compile_definitions(velox PUBLIC ENABLE_KAFKA)
+ find_package(RdKafka CONFIG QUIET)
+ if(RdKafka_FOUND AND TARGET RdKafka::rdkafka++)
+ message(STATUS "Found RdKafka: ${RdKafka_DIR}")
+ target_link_libraries(velox PUBLIC RdKafka::rdkafka++)
+ else()
+ # Try to find librdkafka using pkg-config
+ find_package(PkgConfig QUIET)
+ if(PKG_CONFIG_FOUND)
+ pkg_check_modules(RDKAFKA QUIET IMPORTED_TARGET rdkafka++)
+ if(RDKAFKA_FOUND)
+ message(STATUS "Found librdkafka via pkg-config: ${RDKAFKA_LIBRARIES}")
+ target_link_libraries(velox PUBLIC PkgConfig::RDKAFKA)
+ else()
+ message(FATAL_ERROR "ENABLE_KAFKA is ON but librdkafka was not found.")
+ endif()
+ else()
+ message(
+ FATAL_ERROR
+ "ENABLE_KAFKA is ON but pkg-config was not found to locate
librdkafka."
Review Comment:
These fatal errors are a bit non-actionable for users. Consider including
guidance on how to resolve (e.g., which system packages to install, or how to
point CMake at an existing install via `RdKafka_DIR`/`CMAKE_PREFIX_PATH`, or
`PKG_CONFIG_PATH` if using pkg-config).
##########
dev/ci-velox-buildshared-centos-9.sh:
##########
@@ -27,5 +27,8 @@ fi
export VELOX_BUILD_SHARED=ON
+# Temporary verification for Kafka support in the non-vcpkg dynamic build.
+yum install -y librdkafka-devel
+
Review Comment:
This job installs `librdkafka-devel` at runtime even though the CentOS
dynamic-build Dockerfiles in this PR also add `librdkafka-devel`. If CI uses
those images, this is redundant and slows the job; consider relying on the
image layer (or removing the Dockerfile install and keeping the job install),
but avoid doing both.
--
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]
