Copilot commented on code in PR #808: URL: https://github.com/apache/incubator-graphar/pull/808#discussion_r2617745740
########## rust/build.rs: ########## @@ -0,0 +1,91 @@ +// 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. + +// Portions adapted from `https://github.com/kuzudb/kuzu/blob/master/tools/rust_api/build.rs` (MIT License). +use std::env; +use std::path::{Path, PathBuf}; + +fn link_libraries() { + // Link to system Arrow, `libarrow` is under `/usr/lib/x86_64-linux-gnu/` on Ubuntu + println!("cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu/"); Review Comment: The hardcoded path "/usr/lib/x86_64-linux-gnu/" is Ubuntu/Debian-specific and will fail on other Linux distributions (e.g., Fedora, Arch) or architectures (e.g., aarch64). Consider using environment variables like ARROW_LIB_DIR or pkg-config to detect Arrow library paths dynamically, or document this limitation clearly. ```suggestion // Add pkg-config for dynamic library path detection extern crate pkg_config; fn link_libraries() { // Try to get Arrow library path from ARROW_LIB_DIR env var if let Ok(lib_dir) = env::var("ARROW_LIB_DIR") { println!("cargo:rustc-link-search=native={}", lib_dir); } else { // Try to use pkg-config to find Arrow match pkg_config::Config::new().probe("arrow") { Ok(library) => { for path in library.link_paths { println!("cargo:rustc-link-search=native={}", path.display()); } } Err(_) => { // Fallback to Ubuntu/Debian-specific path with a warning println!("cargo:warning=Could not find Arrow library via ARROW_LIB_DIR or pkg-config, falling back to /usr/lib/x86_64-linux-gnu/ (Ubuntu/Debian-specific)"); println!("cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu/"); } } } ``` ########## rust/Dockerfile: ########## @@ -0,0 +1,52 @@ +# 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. + +# Derived from Graphar +# https://github.com/apache/incubator-graphar/blob/main/.devcontainer/Dockerfile + +# A script to install dependencies required for release +# verification on Ubuntu. +FROM ubuntu:24.04 + +RUN apt-get update && \ + apt-get install -y --no-install-recommends \ + build-essential \ + cmake \ + curl \ + wget \ + git ca-certificates \ + lsb-release \ + clang clangd clang-format vim + +RUN wget https://apache.jfrog.io/artifactory/arrow/$(lsb_release --id --short | tr 'A-Z' 'a-z')/apache-arrow-apt-source-latest-$(lsb_release --codename --short).deb \ + && apt-get install -y ./apache-arrow-apt-source-latest-$(lsb_release --codename --short).deb \ + && apt-get update \ + && apt-get install -y \ + libarrow-dev \ + libarrow-dataset-dev \ + libarrow-acero-dev \ + libparquet-dev \ + libarrow-compute-dev \ Review Comment: The Dockerfile installs the latest Arrow packages without version pinning, while the CI workflow (rust.yaml) pins Arrow to version 22.0.0-1. This inconsistency can lead to different build environments and unexpected build failures. Consider either pinning versions in the Dockerfile to match CI, or documenting why the versions differ. ```suggestion libarrow-dev=22.0.0-1 \ libarrow-dataset-dev=22.0.0-1 \ libarrow-acero-dev=22.0.0-1 \ libparquet-dev=22.0.0-1 \ libarrow-compute-dev=22.0.0-1 \ ``` ########## rust/build.rs: ########## @@ -0,0 +1,91 @@ +// 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. + +// Portions adapted from `https://github.com/kuzudb/kuzu/blob/master/tools/rust_api/build.rs` (MIT License). +use std::env; +use std::path::{Path, PathBuf}; + +fn link_libraries() { + // Link to system Arrow, `libarrow` is under `/usr/lib/x86_64-linux-gnu/` on Ubuntu + println!("cargo:rustc-link-search=native=/usr/lib/x86_64-linux-gnu/"); + println!("cargo:rustc-link-lib=dylib=arrow_compute"); + println!("cargo:rustc-link-lib=dylib=arrow_dataset"); + println!("cargo:rustc-link-lib=dylib=arrow_acero"); + println!("cargo:rustc-link-lib=dylib=arrow"); + + println!("cargo:rustc-link-lib=graphar"); +} + +fn build_ffi(bridge_file: &str, out_name: &str, source_file: &str, include_paths: &Vec<PathBuf>) { Review Comment: The parameter `include_paths` should use a slice reference (&[PathBuf]) instead of a vector reference (&Vec<PathBuf>) following Rust best practices, as it's more flexible and idiomatic. ########## .github/workflows/rust.yaml: ########## @@ -0,0 +1,153 @@ +# 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. + +name: GraphAr Rust CI + +on: + push: + branches: + - "main" + paths: + - "rust/**" + - "cpp/**" + - ".github/workflows/rust.yaml" + pull_request: + branches: + - "main" + paths: + - "rust/**" + - "cpp/**" + - ".github/workflows/rust.yaml" + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +env: + RUSTFLAGS: -Dwarnings + RUST_BACKTRACE: 1 + CI: true + +defaults: + run: + shell: bash + +jobs: + ubuntu: + name: Ubuntu latest Rust + runs-on: ubuntu-24.04 + # run on draft + if: ${{ github.event_name != 'pull_request' || !contains(github.event.pull_request.title, 'WIP') }} + env: + GAR_TEST_DATA: ${{ github.workspace }}/testing/ + defaults: + run: + working-directory: "rust" + steps: + - uses: actions/checkout@v4 + with: + submodules: true + + - uses: Swatinem/rust-cache@v2 + with: + workspaces: "rust -> target" + + - name: Install cargo-nextest + uses: taiki-e/install-action@v2 + with: + tool: [email protected] + + - name: Install cargo-machete + uses: taiki-e/install-action@v2 + with: + tool: [email protected],[email protected] + + - name: Install taplo-cli + uses: taiki-e/install-action@v2 + with: + tool: [email protected] + + - name: Install cargo-llvm-cov + uses: taiki-e/install-action@v2 + with: + tool: [email protected] + Review Comment: [email protected] is installed twice - once in the "Install cargo-machete" step (line 77) and again in the "Install cargo-llvm-cov" step (line 87). Remove the duplicate installation to improve CI efficiency. ```suggestion ``` -- 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]
