Skip to content

fix(agfs): refactor Makefile for cross-platform compatibility#571

Merged
MaojiaSheng merged 2 commits intomainfrom
fix/agfs
Mar 14, 2026
Merged

fix(agfs): refactor Makefile for cross-platform compatibility#571
MaojiaSheng merged 2 commits intomainfrom
fix/agfs

Conversation

@chuanbao666
Copy link
Collaborator

  • 修复agfs跨平台编译问题
  • 支持python 3.14预编译

Type of Change

  • New feature (feat)
  • [] Bug fix (fix)
  • Documentation (docs)
  • Refactoring (refactor)
  • Other

Testing

Related Issues

Checklist

  • Code follows project style guidelines
  • Tests added for new functionality
  • Documentation updated (if needed)
  • All tests pass

@github-actions
Copy link

github-actions bot commented Mar 13, 2026

PR Reviewer Guide 🔍

(Review updated until commit a218b21)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

563 - PR Code Verified

Compliant requirements:

  • Fixed Makefile OS detection by using $(OS) variable instead of shell command
  • Added Python 3.14 to workflow and classifiers
  • Refactored build-lib target to use make conditionals for cross-platform support

Requires further human verification:

  • Verify the build works on native Windows (not just MSYS2)
  • Confirm Python 3.14 wheels are correctly built and installable
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

OS Detection Robustness

Verify PLATFORM is properly initialized for all supported OSes. The current code leaves PLATFORM empty if uname -s returns an unexpected value, which could cause silent failures.

# OS detection
ifeq ($(OS),Windows_NT)
    PLATFORM := windows
else
    UNAME_S := $(shell uname -s)
    ifeq ($(UNAME_S),Linux)
        PLATFORM := linux
    endif
    ifeq ($(UNAME_S),Darwin)
        PLATFORM := darwin
    endif
endif
Windows Compiler Toolchain

Confirm the hardcoded MinGW toolchain (x86_64-w64-mingw32-*) is available and works in the target Windows build environments.

else ifeq ($(PLATFORM),windows)
	CC=x86_64-w64-mingw32-gcc CXX=x86_64-w64-mingw32-g++ AR=x86_64-w64-mingw32-ar GOOS=windows GOARCH=amd64 CGO_ENABLED=1 $(GO) build -buildmode=c-shared -o $(BUILD_DIR)/libagfsbinding.dll cmd/pybinding/main.go
Build Dependencies Scope

Verify the new 'build' dependency group is correctly used and does not introduce unintended runtime dependencies.

build = [
    "setuptools>=61.0",
    "setuptools-scm>=8.0",
    "pybind11>=2.13.0",
    "cmake>=3.15",
    "wheel",
    "build",
]

@github-actions
Copy link

github-actions bot commented Mar 13, 2026

PR Code Suggestions ✨

Latest suggestions up to a218b21
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add MSYS/Cygwin/Mingw OS detection

Improve OS detection to handle MSYS/Cygwin/Mingw environments by checking UNAME_S
for relevant substrings. This restores compatibility with Windows environments that
use these Unix-like shells, which was present in the original code.

third_party/agfs/agfs-server/Makefile [11-22]

 # OS detection
 ifeq ($(OS),Windows_NT)
     PLATFORM := windows
 else
     UNAME_S := $(shell uname -s)
     ifeq ($(UNAME_S),Linux)
         PLATFORM := linux
-    endif
-    ifeq ($(UNAME_S),Darwin)
+    else ifeq ($(UNAME_S),Darwin)
         PLATFORM := darwin
+    else
+        # Check for MSYS/Cygwin/Mingw environments
+        ifneq (,$(findstring MSYS,$(UNAME_S)))
+            PLATFORM := windows
+        else ifneq (,$(findstring CYGWIN,$(UNAME_S)))
+            PLATFORM := windows
+        else ifneq (,$(findstring MINGW,$(UNAME_S)))
+            PLATFORM := windows
+        endif
     endif
 endif
Suggestion importance[1-10]: 7

__

Why: The suggestion restores compatibility with MSYS/Cygwin/Mingw environments that was present in the original code, as the rewritten OS detection only checks $(OS) for Windows_NT and exact matches for Linux/Darwin. This addresses a potential build failure in those Unix-like Windows shells.

Medium

Previous suggestions

Suggestions up to commit 14609d9
CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore requires-python constraint

Restore the requires-python field that was accidentally removed. This field ensures
the package is only installed on compatible Python versions. The constraint >=3.10
is appropriate, as the workflow includes Python 3.10-3.14.

pyproject.toml [19-20]

 license = { text = "Apache-2.0" }
+requires-python = ">=3.10"
 classifiers = [
Suggestion importance[1-10]: 8

__

Why: Restoring the requires-python field ensures the package is only installed on compatible Python versions (>=3.10, matching the workflow's Python 3.10-3.14 range), preventing potential installation issues on incompatible versions.

Medium
General
Validate OS detection result

Add validation to ensure PLATFORM is set after OS detection. If an unsupported OS is
encountered, the Makefile will fail early with a clear error message instead of
proceeding with an undefined PLATFORM variable.

third_party/agfs/agfs-server/Makefile [11-22]

 # OS detection
 ifeq ($(OS),Windows_NT)
     PLATFORM := windows
 else
     UNAME_S := $(shell uname -s)
     ifeq ($(UNAME_S),Linux)
         PLATFORM := linux
     endif
     ifeq ($(UNAME_S),Darwin)
         PLATFORM := darwin
     endif
 endif
+# Validate detected platform
+ifndef PLATFORM
+    $(error Unsupported OS: $(or $(UNAME_S),$(OS)))
+endif
Suggestion importance[1-10]: 6

__

Why: Adding validation for the PLATFORM variable ensures the Makefile fails early with a clear error message on unsupported OSes, instead of proceeding with an undefined variable.

Low

@qin-ctx qin-ctx self-requested a review March 13, 2026 08:21
@chuanbao666 chuanbao666 marked this pull request as draft March 13, 2026 09:15
@chuanbao666 chuanbao666 marked this pull request as ready for review March 13, 2026 10:42
@github-actions
Copy link

Persistent review updated to latest commit a218b21

@MaojiaSheng MaojiaSheng merged commit 84d206d into main Mar 14, 2026
60 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 14, 2026
@MaojiaSheng MaojiaSheng deleted the fix/agfs branch March 14, 2026 09:45
CHW0n9 pushed a commit to CHW0n9/OpenViking that referenced this pull request Mar 15, 2026
…gine#571)

* fix(agfs): refactor Makefile for cross-platform compatibility

* build: support python 3.14
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.

[Bug]: pip install fails on Windows: AGFS binding Makefile calls 'OS' as shell command instead of env variable

3 participants