-
Notifications
You must be signed in to change notification settings - Fork 70
Add Windows (CPU) build support #476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
Summary of ChangesHello @doringeman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the project's build capabilities by introducing comprehensive support for Windows. It enables building on both AMD64 and ARM64 architectures, leveraging the LLVM/clang toolchain. AMD64 builds benefit from Vulkan GPU acceleration, while ARM64 builds are CPU-only. The changes streamline the build process across different operating systems by centralizing OS detection and adapting build scripts to handle platform-specific requirements, including automated dependency management for Windows. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds Windows build support for both AMD64 and ARM64 architectures by introducing a common Makefile for OS detection and updating existing Makefiles to handle Windows-specific commands and paths. The changes are well-structured, particularly the abstraction of OS-specifics. My review focuses on improving robustness and maintainability by addressing hardcoded paths and code duplication in the Makefiles. I've suggested using variables for toolchain and SDK paths and refactoring repeated command blocks.
| PATH="C:/Program Files/LLVM/bin;$$PATH" cmake -B $(BUILD_DIR) \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DGGML_NATIVE=OFF \ | ||
| -DGGML_OPENMP=OFF \ | ||
| -DDDLLAMA_BUILD_UTILS=ON \ | ||
| -GNinja \ | ||
| -DLLAMA_CURL=OFF \ | ||
| -DBUILD_SHARED_LIBS=ON \ | ||
| -DGGML_BACKEND_DL=ON \ | ||
| -DGGML_CPU_ALL_VARIANTS=ON \ | ||
| -DGGML_VULKAN=ON \ | ||
| "-DCMAKE_C_COMPILER=C:/Program Files/LLVM/bin/clang.exe" \ | ||
| "-DCMAKE_CXX_COMPILER=C:/Program Files/LLVM/bin/clang++.exe" \ | ||
| "-DCMAKE_RC_COMPILER=C:/Program Files/LLVM/bin/llvm-rc.exe" \ | ||
| -S $(NATIVE_DIR) | ||
| @echo "Building..." | ||
| PATH="C:/Program Files/LLVM/bin;$$PATH" cmake --build $(BUILD_DIR) --config Release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path to the LLVM toolchain is hardcoded. This can cause issues if LLVM is installed in a different location. It's better to define a variable for the LLVM path at the top of the file (e.g., LLVM_PATH ?= C:/Program Files/LLVM), which can be easily overridden if needed.
PATH="$(LLVM_PATH)/bin;$$PATH" cmake -B $(BUILD_DIR) \
-DCMAKE_BUILD_TYPE=Release \
-DGGML_NATIVE=OFF \
-DGGML_OPENMP=OFF \
-DDDLLAMA_BUILD_UTILS=ON \
-GNinja \
-DLLAMA_CURL=OFF \
-DBUILD_SHARED_LIBS=ON \
-DGGML_BACKEND_DL=ON \
-DGGML_CPU_ALL_VARIANTS=ON \
-DGGML_VULKAN=ON \
"-DCMAKE_C_COMPILER=$(LLVM_PATH)/bin/clang.exe" \
"-DCMAKE_CXX_COMPILER=$(LLVM_PATH)/bin/clang++.exe" \
"-DCMAKE_RC_COMPILER=$(LLVM_PATH)/bin/llvm-rc.exe" \
-S $(NATIVE_DIR)
@echo "Building..."
PATH="$(LLVM_PATH)/bin;$$PATH" cmake --build $(BUILD_DIR) --config Release
| cmake -B $(BUILD_DIR) \ | ||
| -G Ninja \ | ||
| -DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang.exe" \ | ||
| -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang++.exe" \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DGGML_NATIVE=OFF \ | ||
| -DLLAMA_CURL=OFF \ | ||
| -S $(NATIVE_DIR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the amd64 build, the path to the LLVM toolchain is hardcoded here. This should be replaced with a variable (e.g., LLVM_PATH) to make the build script more robust and configurable.
cmake -B $(BUILD_DIR) \
-G Ninja \
-DCMAKE_C_COMPILER="$(LLVM_PATH)/bin/clang.exe" \
-DCMAKE_CXX_COMPILER="$(LLVM_PATH)/bin/clang++.exe" \
-DCMAKE_BUILD_TYPE=Release \
-DGGML_NATIVE=OFF \
-DLLAMA_CURL=OFF \
-S $(NATIVE_DIR)
| @powershell -Command "[System.Environment]::SetEnvironmentVariable('PATH', 'C:\\Program Files\\LLVM\\bin;' + [System.Environment]::GetEnvironmentVariable('PATH', 'Machine'), 'Machine')" || echo "LLVM PATH update attempted" | ||
| @if [ -d "/c/VulkanSDK/$(VULKAN_VERSION)" ]; then \ | ||
| echo "Vulkan SDK $(VULKAN_VERSION) already installed"; \ | ||
| else \ | ||
| echo "Installing Vulkan SDK $(VULKAN_VERSION)..."; \ | ||
| curl.exe -o "$$TEMP/VulkanSDK-Installer.exe" -L "https://sdk.lunarg.com/sdk/download/$(VULKAN_VERSION)/windows/vulkansdk-windows-X64-$(VULKAN_VERSION).exe"; \ | ||
| "$$TEMP/VulkanSDK-Installer.exe" --accept-licenses --default-answer --confirm-command install; \ | ||
| powershell -Command "[System.Environment]::SetEnvironmentVariable('VULKAN_SDK', 'C:\\VulkanSDK\\$(VULKAN_VERSION)', 'User')"; \ | ||
| powershell -Command "[System.Environment]::SetEnvironmentVariable('PATH', [System.Environment]::GetEnvironmentVariable('PATH', 'User') + ';C:\\VulkanSDK\\$(VULKAN_VERSION)\\bin', 'User')"; \ | ||
| echo "Vulkan SDK $(VULKAN_VERSION) installed. Please restart your shell for changes to take effect."; \ | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paths for LLVM and Vulkan SDK are hardcoded. This can lead to problems if the user has installed them in non-default locations. It would be more robust to use variables for these paths, with sensible defaults. For example:
LLVM_INSTALL_PATH ?= C:\\Program Files\\LLVM
VULKAN_INSTALL_PATH ?= C:\\VulkanSDKThen use $(LLVM_INSTALL_PATH) and $(VULKAN_INSTALL_PATH) in these commands.
| else | ||
| UNAME_S := $(shell uname -s) | ||
| ifeq ($(UNAME_S),Linux) | ||
| DETECTED_OS := Linux | ||
| endif | ||
| ifeq ($(UNAME_S),Darwin) | ||
| DETECTED_OS := macOS | ||
| endif | ||
| endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current OS detection logic doesn't handle cases where uname -s returns something other than Linux or Darwin (e.g., FreeBSD). In such cases, DETECTED_OS will be empty, which could lead to confusing "Unsupported OS: " messages later. It would be better to use an if-else-if structure and set a default value for DETECTED_OS to provide a more informative message for unsupported systems.
else
UNAME_S := $(shell uname -s)
ifeq ($(UNAME_S),Linux)
DETECTED_OS := Linux
else ifeq ($(UNAME_S),Darwin)
DETECTED_OS := macOS
else
DETECTED_OS := $(UNAME_S)
endif
endif
| @echo "Cleaning install directory..." | ||
| rm -f $(INSTALL_DIR)/bin/*.py | ||
| rm -rf $(INSTALL_DIR)/lib/cmake | ||
| rm -rf $(INSTALL_DIR)/lib/pkgconfig | ||
| rm -rf $(INSTALL_DIR)/include | ||
| @echo "Build complete! Binaries are in $(INSTALL_DIR)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code for cleaning the install directory is duplicated in the arm64 build section (lines 105-110). To improve maintainability and reduce redundancy, you could define a multi-line variable using define for these commands and call it from both places. For example:
define WINDOWS_POST_INSTALL_CLEAN
@echo "Cleaning install directory..."
rm -f $(INSTALL_DIR)/bin/*.py
rm -rf $(INSTALL_DIR)/lib/cmake
rm -rf $(INSTALL_DIR)/lib/pkgconfig
rm -rf $(INSTALL_DIR)/include
@echo "Build complete! Binaries are in $(INSTALL_DIR)"
endefThen use $(call WINDOWS_POST_INSTALL_CLEAN) in both build sections.
| @echo "Cleaning install directory..." | ||
| rm -f $(INSTALL_DIR)/bin/*.py | ||
| rm -rf $(INSTALL_DIR)/lib/cmake | ||
| rm -rf $(INSTALL_DIR)/lib/pkgconfig | ||
| rm -rf $(INSTALL_DIR)/include | ||
| @echo "Build complete! Binaries are in $(INSTALL_DIR)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement full Windows build support for AMD64 and ARM64 architectures. The build now uses LLVM/clang toolchain with Vulkan GPU acceleration support on AMD64, and CPU-only builds on ARM64.
Backend (terminal 1)
CLI (terminal 2):