-
Notifications
You must be signed in to change notification settings - Fork 397
[build] fix native arm64 builds #4168
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: master
Are you sure you want to change the base?
Conversation
40a3191
to
499f26e
Compare
|
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.
Pull Request Overview
Enables reliable ARM64 (Apple Silicon) builds by refining Makefile scripts and requirements generation.
- Prioritizes pyenv shims and standardizes pip invocations (
python -m pip
) across Makefiles. - Improves virtual environment cleanup and ensures Django’s
collectstatic
sees all modules. - Updates
generate_requirements.py
to correctly handle ARM64 architecture and load the right Polars package.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
desktop/core/generate_requirements.py | Map ‘arm64’ to ‘aarch64’ and relocate Polars pins into version-specific lists. |
Makefile.sdk | Switch to $(ENV_PYTHON) -m pip with flags and clean up versioned req dirs. |
Makefile | Prepend pyenv shims to PATH, switch pip installs, set PYTHONPATH for static, and clean venvs. |
if self.arch == 'arm64': | ||
self.arch = 'aarch64' |
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 code maps 'arm64' to 'aarch64' but platform_map
only has an 'arm64' key. After setting self.arch = 'aarch64'
, lookups will fail. Either map 'aarch64' in platform_map
or avoid changing the key.
if self.arch == 'arm64': | |
self.arch = 'aarch64' | |
# No need to map 'arm64' to 'aarch64' as both are handled in arch_requirements_map |
Copilot uses AI. Check for mistakes.
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.
It will not work without this fix.
Makefile.sdk
Outdated
@@ -174,6 +174,7 @@ clean:: ext-clean | |||
@rm -Rf $(BUILD_DIR) dist | |||
@find . -name \*.egg-info -prune -exec rm -Rf {} \; | |||
@find . -name \*.py[co] -exec rm -f {} \; | |||
@rm -rf $(APP_ROOT)/$(PYTHON_VER) |
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.
Removing $(APP_ROOT)/$(PYTHON_VER)
may not target the generated desktop/core/$(PYTHON_VER)
directory. Consider using the correct path where versioned requirements are written.
@rm -rf $(APP_ROOT)/$(PYTHON_VER) | |
@rm -rf $(APP_ROOT)/desktop/core/$(PYTHON_VER) |
Copilot uses AI. Check for mistakes.
Makefile
Outdated
@@ -163,9 +169,9 @@ $(BLD_DIR_ENV)/bin/python: | |||
@$(ENV_PYTHON) -m pip install virtualenv==$(VIRTUAL_ENV_VERSION) virtualenv-make-relocatable==$(VIRTUAL_ENV_RELOCATABLE_VERSION) | |||
@$(eval RELOCATABLE := $(shell which virtualenv-make-relocatable)) | |||
@echo "REQUIREMENT_FILE is $(REQUIREMENT_FILE)" | |||
@$(ENV_PIP) install -r $(REQUIREMENT_FILE) | |||
@$(ENV_PYTHON) -m pip install -r $(REQUIREMENT_FILE) |
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.
[nitpick] For consistency with Makefile.sdk, consider adding flags --no-cache-dir --upgrade --force-reinstall
to pip installs here to ensure reproducible builds across architectures.
@$(ENV_PYTHON) -m pip install -r $(REQUIREMENT_FILE) | |
@$(ENV_PYTHON) -m pip install --no-cache-dir --upgrade --force-reinstall -r $(REQUIREMENT_FILE) |
Copilot uses AI. Check for mistakes.
Python Coverage Report •
Pytest Report
|
What changes were proposed in this pull request?
Overview
This Pull Request introduces a series of targeted modifications across .gitignore, the root Makefile, Makefile.sdk, and desktop/core/generate_requirements.py. These changes are primarily driven by the need to enable a stable and reliable build process for Hue on ARM64 (Apple Silicon / M1/M2) Macs, while ensuring full backward compatibility and improved robustness for all other supported architectures (e.g., x86_64 Linux/macOS).
Problem Statement
Building Hue on ARM64 Macs exposed several critical issues:
Incorrect Python Environment Resolution: The make build system often failed to correctly identify and utilize the native ARM64 Python installed via pyenv. Instead, it would sometimes default to system-installed x86_64 Python frameworks or struggle with pyenv's shim paths, leading to:
Incorrect Python header paths (PYTHON_INCLUDE_DIR).
Errors like FileNotFoundError: .../bin/pip because pip was being sought in the wrong, often non-existent, location.
Malformed pip install Commands: Various Makefile targets (in both the root Makefile and Makefile.sdk) were attempting to execute pip install commands using a syntax that, in certain contexts (especially with the misidentified Python environments), resulted in the Python interpreter trying to "open file 'install'", leading to can't open file 'install': No such file or directory errors.
Incorrect Requirements File Selection: The desktop/core/generate_requirements.py script, responsible for dynamically selecting architecture-specific Python requirements, did not correctly map platform.machine()'s 'arm64' output (from M1/M2 Macs) to its expected 'aarch64' key, causing it to fall back to x86_64-specific dependency lists.
Module Not Found at Runtime (ModuleNotFoundError: No module named 'apps'): After the build, when attempting to run Hue, the Django application couldn't find its core modules because the project's root directory wasn't consistently added to Python's search path (PYTHONPATH).
Changes Introduced
This PR addresses the above problems with the following modifications:
Makefile
Makefile.sdk
The Makefile.sdk was updated to enhance dependency installation reliability and cleanup. Changes included using $(ENV_PYTHON) -m pip with aggressive reinstall flags (--no-cache-dir --upgrade --force-reinstall) for all package installations, and adding a cleanup step to remove generated version-specific requirements directories like desktop/core/3.8/ during make clean.
generate_requirements.py
The generate_requirements.py script was significantly updated to properly support ARM64 (M1/M2 Mac) builds. This involved accurately mapping the arm64 architecture for correct requirements file generation and precisely specifying polars-lts-cpu for ARM64 builds, while ensuring polars[calamine] remains for x86_64 builds.
How was this patch tested?
By executing build according new ARM-native Python 3.8 instructions in "Hue Setup for M1 Macs" docs
Please review Hue Contributing Guide before opening a pull request.