fix: add ARM architecture support for conda installation#56
fix: add ARM architecture support for conda installation#56RusilKoirala wants to merge 6 commits intonu-ZOO:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds ARM architecture support to the conda installation script to fix issue #55. The changes enable the setup script to work on ARM-based systems (like Apple Silicon Macs) by detecting the system architecture and using the appropriate conda installer.
Changes:
- Added architecture detection using
uname -mto identify x86_64, arm64, or aarch64 systems - Updated the conda download URL to use the detected architecture instead of hardcoded x86_64
- Enhanced logging to display both OS and architecture information
Comments suppressed due to low confidence (1)
setup.sh:41
- The Miniconda installer is downloaded from
https://repo.anaconda.comintominiconda.shand then executed withbashwithout any integrity verification. If an attacker can tamper with the installer content in transit or via a supply-chain compromise of the hosting endpoint, they could execute arbitrary code in the environment runningsetup.sh. Consider enforcing integrity checks (e.g., verifying a known checksum or signature for the installer) before executing it, or using an installation mechanism that provides built-in integrity verification.
CONDA_URL="https://repo.anaconda.com/miniconda/Miniconda3-py${PYTHON_VERSION//.}_24.9.2-0-${CONDA_OS}-${CONDA_ARCH}.sh"
if which wget; then
wget ${CONDA_URL} -O miniconda.sh
else
curl ${CONDA_URL} -o miniconda.sh
fi
bash miniconda.sh -b -p $HOME/miniconda
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
setup.sh
Outdated
| export CONDA_ARCH=x86_64 | ||
| ;; | ||
| arm64|aarch64) | ||
| export CONDA_ARCH=arm64 |
There was a problem hiding this comment.
The architecture naming convention may be incorrect for Linux ARM systems. Miniconda uses different architecture names depending on the OS:
- macOS: arm64 (correct)
- Linux: aarch64 (not arm64)
Currently, this code maps both arm64 and aarch64 from uname -m to arm64 for the conda URL. While this will work correctly for macOS (which reports arm64), it will likely fail for Linux ARM systems because the miniconda repository uses "aarch64" in the filename for Linux ARM packages (e.g., "Miniconda3-...-Linux-aarch64.sh"), not "arm64".
Consider updating the logic to set CONDA_ARCH based on both the OS and the architecture. For example, use arm64 for macOS and aarch64 for Linux when detecting arm-based systems.
| export CONDA_ARCH=arm64 | |
| if [ "$CONDA_OS" = "MacOSX" ]; then | |
| export CONDA_ARCH=arm64 | |
| else | |
| export CONDA_ARCH=aarch64 | |
| fi |
There was a problem hiding this comment.
Github copilots review here is almost right, but not quite.
When you use uname -m on a linux based arm64 machine, it will return aarch64. So a simpler method for extracting this is like so:
# Setting architecture based on input
CONDA_ARCH=$(uname -m)
case $CONDA_ARCH in
x86_64) : ;;
arm64) : ;;
aarch64) : ;;
*)
echo "Installation only supported on x86_64 and arm architectures"
exit 1
;;
esac
There was a problem hiding this comment.
I believe this small change to simplify the architecture selection would be nice.
It would be good to also alter the GHA runner as done here to test everything under differing architectures :)
setup.sh
Outdated
| case "$(uname -m)" in | ||
| x86_64) | ||
| export CONDA_ARCH=x86_64 | ||
| ;; | ||
| arm64|aarch64) | ||
| if [ "$CONDA_OS" = "MacOSX" ]; then | ||
| export CONDA_ARCH=arm64 | ||
| else | ||
| export CONDA_ARCH=aarch64 | ||
| fi | ||
| ;; | ||
| *) | ||
| echo "Unsupported architecture: $(uname -m)" | ||
| exit 1 | ||
| ;; | ||
| esac |
|
@jwaiton Updated! I've simplified the architecture detection as you suggested. The logic now directly uses the output from |
|
Hi @RusilKoirala. Perfect! That looks to work, and passes the expected tests. The only addition needed now is updating the GHA runner as suggested above, and then I can approve this :) |
|
@jwaiton I've added multi-architecture testing to the GHA workflow. The tests will now run on:
The workflow now uses the |
|
Hi @RusilKoirala, It appears that the MacOS image you wish to run for intel is out of date: The error suggests using macos-15-intel instead. The other tests all failed, due to two things:
Resolving these two issues should fix it :) For the first once I'd recommend a pipe of this style: For the second, I'll leave that to you, but it should just require the addition of |
|
@jwaiton Fixed all three issues:
Tested locally on M4 - everything works. Ready for review! |
|
Hi @RusilKoirala, It appears the tests still fail! I'm unsure as to why the Mac ones fail, but the ubuntu one appears to be relatively simple: The simple solution (I believe) is to use |
|
@jwaiton |
|
Because running the tests is annoying for users outside of the nu-ZOO group (sorry about that!), I've done some debugging myself and found that this is the last required change to make the code run. Doing this will allow the tests to pass :) It also appears that ubuntu arm64 is no longer available as a github runner platform as seen here, so please remove it from the list of runners tested. :) |
Summary
Fixes #55
Added support for ARM-based architectures (arm64/aarch64) in the conda installation process.
Changes
uname -mx86_64Testing
Related
Implementation follows the pattern suggested in the issue discussion.