Copilot commented on code in PR #3677: URL: https://github.com/apache/celeborn/pull/3677#discussion_r3354309968
########## rust/celeborn-client-sys/build.rs: ########## @@ -0,0 +1,186 @@ +// 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. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +/// Resolve the directory containing `libceleborn_client.{so,dylib}`. +/// +/// Lookup order (first match wins): +/// 1. `CELEBORN_CPP_PREFIX` env var → use `<prefix>/lib`. Intended for CI / +/// custom installs. +/// 2. In-repo prebuilt artifact at +/// `rust/resource/lib/<target-triple>/libceleborn_client.{so,dylib}`. +/// Drop the matching dylib in and `cargo build` picks it up with no +/// extra environment variable. +/// 3. Fall back to driving `cmake` against the in-repo `cpp/` source tree +/// and installing into `$OUT_DIR/celeborn-cpp-install/lib`. +fn resolve_lib_dir() -> PathBuf { + let target_os = std::env::var("CARGO_CFG_TARGET_OS").unwrap(); + let lib_filename = match target_os.as_str() { + "macos" => "libceleborn_client.dylib", + "linux" => "libceleborn_client.so", + other => panic!("unsupported target_os: {other} (only linux/macos supported)"), + }; + + // 1. Explicit override. + if let Ok(prefix) = std::env::var("CELEBORN_CPP_PREFIX") { + let lib_dir = PathBuf::from(&prefix).join("lib"); + if !lib_dir.join(lib_filename).exists() { + panic!( + "CELEBORN_CPP_PREFIX={prefix} but {} does not exist.", + lib_dir.join(lib_filename).display() + ); + } + eprintln!("cargo:warning=Using prebuilt Celeborn dylib from {prefix}"); + return lib_dir; + } + + // 2. In-repo prebuilt: rust/resource/lib/<target>/libceleborn_client.<ext> + let manifest_dir = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()); + let target = std::env::var("TARGET").unwrap(); + let resource_lib_dir = manifest_dir + .join("../resource/lib") + .join(&target); + if resource_lib_dir.join(lib_filename).exists() { + eprintln!( + "cargo:warning=Using in-repo prebuilt Celeborn dylib at {}", + resource_lib_dir.display() + ); + return resource_lib_dir + .canonicalize() + .expect("failed to canonicalize resource lib dir"); + } + + // 3. Fall back to cmake from source. + let cpp_source_dir = manifest_dir + .join("../../cpp") + .canonicalize() + .unwrap_or_else(|_| { + panic!( + "No prebuilt {lib_filename} found at {}, CELEBORN_CPP_PREFIX is unset, \ + and the in-repo cpp/ directory is not reachable from {}.", + resource_lib_dir.display(), + manifest_dir.display(), + ) + }); + + eprintln!( + "cargo:warning=No prebuilt dylib at {}; building Celeborn C++ from source at {}", + resource_lib_dir.display(), + cpp_source_dir.display() + ); + + cmake_build_cpp(&cpp_source_dir).join("lib") +} + +fn cmake_build_cpp(source_dir: &Path) -> PathBuf { + let out_dir = PathBuf::from(std::env::var("OUT_DIR").unwrap()); + let build_dir = out_dir.join("celeborn-cpp-build"); + let install_dir = out_dir.join("celeborn-cpp-install"); + + std::fs::create_dir_all(&build_dir).expect("failed to create cmake build directory"); + std::fs::create_dir_all(&install_dir).expect("failed to create cmake install directory"); + + let target_os = std::env::var("CARGO_CFG_TARGET_OS").unwrap(); + + let mut configure_cmd = Command::new("cmake"); + configure_cmd + .current_dir(&build_dir) + .arg(source_dir) + .arg(format!("-DCMAKE_INSTALL_PREFIX={}", install_dir.display())) + .arg("-DCMAKE_BUILD_TYPE=Release") + .arg("-DCELEBORN_BUILD_TESTS=OFF"); + + if target_os == "macos" { + let homebrew_prefix = std::env::var("HOMEBREW_PREFIX") + .unwrap_or_else(|_| "/opt/homebrew".to_string()); + configure_cmd.arg(format!("-DCMAKE_PREFIX_PATH={homebrew_prefix}")); + configure_cmd.env( + "OPENSSL_ROOT_DIR", + format!("{homebrew_prefix}/opt/openssl@3"), + ); + } + + let configure_status = configure_cmd + .status() + .expect("failed to execute `cmake` – is cmake installed?"); + if !configure_status.success() { + panic!("cmake configure step failed (exit code: {configure_status})"); + } + + let num_jobs = std::env::var("NUM_JOBS").unwrap_or_else(|_| num_cpus().to_string()); + let build_status = Command::new("cmake") + .current_dir(&build_dir) + .args(["--build", "."]) + .args(["--config", "Release"]) + .args(["--parallel", &num_jobs]) + .status() + .expect("failed to execute cmake --build"); + if !build_status.success() { + panic!("cmake build step failed (exit code: {build_status})"); + } + + let install_status = Command::new("cmake") + .current_dir(&build_dir) + .args(["--install", "."]) + .status() + .expect("failed to execute cmake --install"); + if !install_status.success() { + panic!("cmake install step failed (exit code: {install_status})"); + } + + install_dir +} + +fn num_cpus() -> usize { + std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(4) +} + +fn main() { + let lib_dir = resolve_lib_dir(); + let lib_dir_str = lib_dir.display().to_string(); + + // Single aggregated dylib: libceleborn_client.{so,dylib} bundles every + // internal static lib (including the celeborn_ffi C ABI shim) and pulls + // third-party deps via NEEDED entries that resolve at runtime. The Rust + // crate touches no C++ headers and links nothing else. + println!("cargo:rustc-link-search=native={lib_dir_str}"); + println!("cargo:rustc-link-lib=dylib=celeborn_client"); + + // Re-export lib_dir so the downstream `celeborn-client` crate can pick + // it up via DEP_CELEBORN_CLIENT_LIB_DIR and embed an rpath in its + // examples / tests / binaries. (`cargo:rustc-link-arg` does not + // propagate from a sys crate to dependent crates' artifacts.) + println!("cargo:lib_dir={lib_dir_str}"); Review Comment: The build script is trying to pass `lib_dir` to dependent crates (so they can read it via `DEP_CELEBORN_CLIENT_LIB_DIR`), but `println!("cargo:lib_dir=...")` is not valid Cargo metadata output and will not propagate to dependents. Use the `cargo:metadata=KEY=VALUE` form so Cargo exports `DEP_CELEBORN_CLIENT_LIB_DIR` to downstream build scripts. ########## sbin/start-lifecycle-manager.sh: ########## @@ -0,0 +1,198 @@ +#!/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. + +# Starts a standalone Celeborn LifecycleManager daemon in background. +# The RPC port is randomly selected from 30000~50000 if --port is not specified. +# After successful start, the port is exported as CELEBORN_LM_PORT. + +set -euo pipefail + +if [ -z "${CELEBORN_HOME:-}" ]; then + export CELEBORN_HOME="$(cd "$(dirname "$0")/.."; pwd)" +fi + +usage() { + cat <<EOF +Usage: $0 --app-id <id> --master-endpoints <ep1,ep2,...> [--port <port>] [--host <host>] [--properties-file <file>] + + --app-id REQUIRED unique application id + --master-endpoints REQUIRED comma-separated host:port of Celeborn Masters + --port OPTIONAL fixed RPC port to bind (default: random available port in 30000~50000) + --host OPTIONAL bind host (default: hostname) + --properties-file OPTIONAL path to celeborn-defaults.conf +EOF + exit 1 +} + +# Probe whether `port` is currently bound on ANY local interface. +# Returns 0 when the port appears free, non-zero when something is listening. +# Prefer lsof / ss / netstat (cover every interface, IPv4 and IPv6) and only +# fall back to /dev/tcp loopback probing — the latter misses listeners bound +# to a specific non-loopback interface, which would let us hand out a port +# the daemon cannot actually bind. +port_is_free() { + local port="$1" + if command -v lsof >/dev/null 2>&1; then + ! lsof -nP -iTCP:"${port}" -sTCP:LISTEN >/dev/null 2>&1 + return + fi + if command -v ss >/dev/null 2>&1; then + ! ss -ltn "sport = :${port}" 2>/dev/null | awk 'NR>1 {found=1} END {exit !found}' + return + fi + if command -v netstat >/dev/null 2>&1; then + ! netstat -anL 2>/dev/null | awk -v p=":${port}" '$1 ~ /^tcp/ && $4 ~ p"$" {found=1} END {exit !found}' + return + fi Review Comment: `netstat -anL` is not a portable invocation (e.g., Linux net-tools uses `-l`, and macOS uses different flags). With `set -euo pipefail`, a failing `netstat` invocation can make `port_is_free` incorrectly treat the port as free, potentially selecting a port that is already in use. Use a more widely supported `netstat -an` and filter for LISTEN instead of relying on `-L`. -- 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]
