Skip to content

Fix ci#1307

Merged
MaojiaSheng merged 2 commits intomainfrom
fix_ci
Apr 8, 2026
Merged

Fix ci#1307
MaojiaSheng merged 2 commits intomainfrom
fix_ci

Conversation

@zhoujh01
Copy link
Copy Markdown
Collaborator

@zhoujh01 zhoujh01 commented Apr 8, 2026

Description

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🏅 Score: 80
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Change maturin invocation to use python -m maturin

Relevant files:

  • setup.py
  • pyproject.toml
  • .github/workflows/_build.yml

Sub-PR theme: Update pyo3 version and adjust GIL handling

Relevant files:

  • crates/ragfs-python/Cargo.toml
  • crates/ragfs-python/src/lib.rs

Sub-PR theme: Add tests for build configuration

Relevant files:

  • tests/misc/test_root_docker_image_packaging.py

⚡ Recommended focus areas for review

Possible Compilation Error

Replaced Python::with_gil with Python::attach; Python::attach is not a standard pyo3 API. This will cause a compilation failure.

fn get_capabilities(&self) -> PyResult<HashMap<String, Py<PyAny>>> {
    Python::attach(|py| {
        let mut m = HashMap::new();
        m.insert("version".to_string(), "ragfs-python".into_pyobject(py)?.into_any().unbind());
        let features = vec!["memfs", "kvfs", "queuefs", "sqlfs"];
        m.insert("features".to_string(), features.into_pyobject(py)?.into_any().unbind());
        Ok(m)
    })
Possible Compilation Error

Replaced Python::with_gil with Python::attach in multiple places; this function does not exist in pyo3, leading to build failure.

Python::attach(|py| {
    let list = PyList::empty(py);
    for entry in &entries {
        let dict = file_info_to_py_dict(py, entry)?;
        list.append(dict)?;
    }
    Ok(list.into())
})
Possible Compilation Error

Another instance of Python::attach being used instead of Python::with_gil, which will break the build.

Python::attach(|py| {
    Ok(PyBytes::new(py, &data).into())
})
Possible Compilation Error

Yet another use of non-existent Python::attach instead of Python::with_gil; this will cause compilation to fail.

Python::attach(|py| {
    let dict = file_info_to_py_dict(py, &info)?;
    Ok(dict.into())
})
Missing ragfs-python Build

Removed the explicit ragfs-python build and extraction steps from CI; the new setup.py logic may not build and extract the library into openviking/lib/ during wheel build, leading to missing functionality.

    echo "Resolved OpenViking version: $OPENVIKING_VERSION"

- name: Build Rust CLI (Linux)
  run: cargo build --release --target ${{ matrix.arch == 'aarch64' && 'aarch64-unknown-linux-gnu' || 'x86_64-unknown-linux-gnu' }} -p ov_cli

- name: Copy Rust CLI binary (Linux)
  run: |
    mkdir -p openviking/bin
    cp target/${{ matrix.arch == 'aarch64' && 'aarch64-unknown-linux-gnu' || 'x86_64-unknown-linux-gnu' }}/release/ov openviking/bin/
    chmod +x openviking/bin/ov

- name: Clean workspace (force ignore dirty)
  shell: bash
  run: |
    # Back up pre-built artifacts before cleaning
    cp -a openviking/bin /tmp/_ov_bin || true
    git reset --hard HEAD
    git clean -fd
    rm -rf openviking/_version.py openviking.egg-info
    # Restore pre-built artifacts
    cp -a /tmp/_ov_bin openviking/bin || true

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 8, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix invalid PyO3 GIL acquisition call

PyO3 0.27 does not have a Python::attach method. Use Python::with_gil (the standard
PyO3 GIL acquisition API) instead to avoid compile/runtime failures.

crates/ragfs-python/src/lib.rs [141-277]

 fn get_capabilities(&self) -> PyResult<HashMap<String, Py<PyAny>>> {
-    Python::attach(|py| {
+    Python::with_gil(|py| {
         let mut m = HashMap::new();
         m.insert("version".to_string(), "ragfs-python".into_pyobject(py)?.into_any().unbind());
         let features = vec!["memfs", "kvfs", "queuefs", "sqlfs"];
         m.insert("features".to_string(), features.into_pyobject(py)?.into_any().unbind());
         Ok(m)
     })
 }
 
 ...
 
 fn ls(&self, path: String) -> PyResult<Py<PyAny>> {
     let fs = self.fs.clone();
     let entries = self.rt.block_on(async move {
         fs.read_dir(&path).await
     }).map_err(to_py_err)?;
 
-    Python::attach(|py| {
+    Python::with_gil(|py| {
         let list = PyList::empty(py);
         for entry in &entries {
             let dict = file_info_to_py_dict(py, entry)?;
             list.append(dict)?;
         }
         Ok(list.into())
     })
 }
 
 ...
 
-Python::attach(|py| {
+Python::with_gil(|py| {
     Ok(PyBytes::new(py, &data).into())
 })
 
 ...
 
-Python::attach(|py| {
+Python::with_gil(|py| {
     let dict = file_info_to_py_dict(py, &info)?;
     Ok(dict.into())
 })
Suggestion importance[1-10]: 9

__

Why: PyO3 0.27 does not include Python::attach, reverting to Python::with_gil fixes a critical compile error. This addresses a major functional issue in the Rust binding code.

High

@MaojiaSheng MaojiaSheng merged commit b174deb into main Apr 8, 2026
40 of 45 checks passed
@MaojiaSheng MaojiaSheng deleted the fix_ci branch April 8, 2026 16:05
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants