Skip to content

Refactor Windows on ARM build script #193

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

matthew-brett
Copy link
Contributor

Use the standard repo structure for build.

Use the standard repo structure for build.
@matthew-brett matthew-brett marked this pull request as draft April 10, 2025 16:23
@Harishmcw
Copy link
Contributor

Hi Mathew,

Fixes in the Refactored Code:

  1. Fixing Directory Handling for Build Output: (ln No. 26)
pushd "%~dp0\.."
set "ob_out_root=%CD%\local\scipy_openblas"
set "ob_64=%ob_out_root%64"
set "ob_32=%ob_out_root%32"
if exist "%ob_64%" xcopy "%ob_64%" "%ob_32%" /I /Y
set "DEST_DIR=%ob_32%" 
  • Adding pushd "%~dp0.." makes the script more robust—it always sets the working directory to the root of the openblas-libs repo, no matter where the script is invoked from.
  • The original copy command was treating the folder as a file, leading to an error during execution. Switching to xcopy ensures proper copying of the folder and its contents.
  1. Ensuring a Clean Build Environment: (ln. No 46) 
if exist build (rmdir /S /Q build || exit /b 1)
mkdir build || exit /b 1 & cd build || exit /b 1 
  • Prevents stale or conflicting build artifacts from affecting the current build.
  • Helps in achieving a clean and reproducible build every time.
  1. Adding a check before running Python -m build: (ln. No 114)

python -c "import build" 2>NUL || pip install build

  1. Adding the .h extension to the missing header file: (ln. No 100)

if exist lapacke_mangling.h copy /Y lapacke_mangling.h "%DEST_DIR%\include

@matthew-brett
Copy link
Contributor Author

Thanks - could you check I have made those changes as you intended them? By the way, I think you can make pull request to my branch with the changes directly.

@Harishmcw
Copy link
Contributor

Hi Mathew,

I was going through the bat script and noticed the environment variables are currently being set using inline quotes (e.g., set var="value"), which embeds the quotes inside the variable value. So the actual variable value is like:
"C:\some path\local\scipy_openblas64". This can lead to double quotes in unexpected places later in the script.

A safer and more robust approach would be:

set "ob_out_root=%CD%\local\scipy_openblas"
set "ob_64=%ob_out_root%64"
set "ob_32=%ob_out_root%32"
if exist "%ob_64%" xcopy "%ob_64%" "%ob_32%" /I /Y
set "DEST_DIR=%ob_32%" 

This version quotes the values correctly and handles paths with spaces or special characters safely.

Thanks!

@matthew-brett
Copy link
Contributor Author

Current code correct?

@Harishmcw
Copy link
Contributor

Current code correct?

Yep, looks perfect now.👍

@matthew-brett
Copy link
Contributor Author

@Harishmcw - can you think of a way to avoid leaving behind a modified pyproject.toml file?

@Harishmcw
Copy link
Contributor

@Harishmcw - can you think of a way to avoid leaving behind a modified pyproject.toml file?

Hi @matthew-brett ,

To avoid leaving a modified pyproject.toml file, we can create a backup of it.

Add this line after ln No.68:

copy pyproject.toml pyproject.toml.bak

This creates a backup of the original pyproject.toml file as pyproject.toml.bak before any modifications are made. This ensures that we have a copy of the original file.

Add this line after ln No.117:

move /Y pyproject.toml.bak pyproject.toml

This line restores the original pyproject.toml file from the backup (pyproject.toml.bak). The /Y option automatically confirms the overwrite, ensuring that the original file is restored and no modified version is left behind.

Let me know if you need any further clarification or adjustments!

Thanks!

@matthew-brett
Copy link
Contributor Author

How about the current code?

@Harishmcw
Copy link
Contributor

How about the current code?

Yes, the current code works as expected. However, it does leave behind the pyproject_64_32.toml file after the process.

@matthew-brett
Copy link
Contributor Author

How about the current code?

Yes, the current code works as expected. However, it does leave behind the pyproject_64_32.toml file after the process.

I don't see that - I think the move commands take care of that deletion.

@Harishmcw
Copy link
Contributor

How about the current code?

Yes, the current code works as expected. However, it does leave behind the pyproject_64_32.toml file after the process.

I don't see that - I think the move commands take care of that deletion.

Ah, you're right — I missed that move command. Thanks for pointing that out, and sorry for the confusion!

@mattip
Copy link
Collaborator

mattip commented Apr 15, 2025

It seems github has opened the windows11-arm64 runners for public repos like this. Want to add a workflow?

@matthew-brett
Copy link
Contributor Author

matthew-brett commented May 7, 2025

Recording progress here.

I've started a .github/workflows/windows-arm.yml runner config.

@vksukumarx worked out how to install LLVM directly via the installer (see the runner config).

The current problem is that the cmake installed on the WoA runner is configured for x86_64 - see https://github.com/MacPython/openblas-libs/actions/runs/14864637680/job/41738232174 for error:

"C:/a/openblas-libs/openblas-libs/OpenBLAS/kernel/x86_64/KERNEL.ARMV8"
cannot be read.

This is a known problem that will apparently be fixed in the next version of the runner :

actions/partner-runner-images#84

I have tried using winget to install cmake via the usual CMake installer, but this fails because the installer fails on trying to uninstall the previous version of CMake - I'll get links to errors if anyone is interested.

I have also tried to solve this by installing cmake via msys2 on the image, over at https://github.com/matthew-brett/woa-setup - but this is failing due to permission errors, that I can reproduce on the laptop, but that I do not understand. Here's the command I am using to install msys2 (after downloading the installer):

.\msys2_installer.exe install --root C:\msys2 --default-answer --confirm-command

but, no matter what directory I try, this always give me:

[100] Critical: Error writing Maintenance Tool:  "Cannot write installer configuration to C:/msys2/: Access error"
[100] WriteError : Error writing Maintenance Tool : Cannot write installer configuration to C:/msys2/: Access error

Someone else ran into the same error on the ARM64 image.

There is an open request to install msys2 on the runner but it's not clear whether that will happen.

Any other suggestions for a working native CMake welcome. Msys2 might also be a good way to get clang and flang-new (as they are built for msys2 and WoA).

To make installs easier, I have also asked the team to install winget on the runner image, to make installation easier:

actions/partner-runner-images#95

although, as noted there, I have worked out how to do this in a workflow, with considerable experimentation.

Suggestions?

@rgommers
Copy link
Collaborator

rgommers commented May 7, 2025

pip install cmake? There's a win-arm64 wheel up at https://pypi.org/project/cmake/#files

@matthew-brett
Copy link
Contributor Author

pip install cmake? There's a win-arm64 wheel up at https://pypi.org/project/cmake/#files

Thanks - good thought - with that unblocked, I worked through the rest, the build is now passing.

My remaining worries are:

  • We don't appear to be building static and dynamic library link files (for gfortran, these would be .a and .dll.a. We only have a .LIB file (and the .dll). Am I missing something?
  • I'm afraid I'm confused by the code in tools/build_steps_win_arm64.bat where we munge pyproject.toml and other files to replace openblas64 with openblas32, regardless of the BUILD_BITS. Shouldn't the naming reflect the BUILD_BIT variable? For example, why do we end up with a scipy_openblas32 wheel from a BUILD_BIT=64 build?

Add GHA workflow for building on Windows on ARM.
@Harishmcw
Copy link
Contributor

  • I'm afraid I'm confused by the code in tools/build_steps_win_arm64.bat where we munge pyproject.toml and other files to replace openblas64 with openblas32, regardless of the BUILD_BITS. Shouldn't the naming reflect the BUILD_BIT variable? For example, why do we end up with a scipy_openblas32 wheel from a BUILD_BIT=64 build?

Hi @matthew-brett,

To clarify:

  • The if_bits variable, defined in ⁠tools/build_openblas.sh, determines whether the resulting wheel is named scipy_openblas32 or scipy_openblas64. On the other hand, the build_bits variable is used to set the target architecture through the -DBINARY flag in the CMake configuration when building OpenBLAS (i.e., whether to build a 32-bit or 64-bit library)

  • Since SciPy currently uses the scipy_openblas32 wheel (with 32-bit integer interfaces), the existing code is written to produce that. If there's a need to generate a scipy_openblas64 wheel for NumPy (i.e., with INTERFACE64=1), then we'd need to modify the build logic to support that interface configuration explicitly.

  • I'll be pushing the refactored bat script shortly that also supports setting the INTERFACE64 flag, which will produce the scipy_openblas64 wheel.

@matthew-brett
Copy link
Contributor Author

matthew-brett commented May 8, 2025

For my own understanding - is this correct?

The builds can be 32-bit or 64-bit builds - meaning - binaries for 32-bit for 64-bit architecture. Call this "binary-bits". In e.g. tools/build_openblas.sh, the variable BUILD_BITS specifies whether the binary-bits are 32 or 64.

The builds can have 64- or 32-bit API interfaces to BLAS / LAPACK. Call this "API-bits". INTERFACE64=0 in the scripts gives API-bits of 32, INTERFACE64=1 gives API-bits of 64.

It doesn't make sense to have binary-bits = 32 and API-bits = 64.

Windows on ARM (WoA) effectively only exists in 64-bit binary interface form.

The current WoA build script builds binary-bits=64 and API-bits=32.

The pyproject.toml files in the repository are designed for API-bits=64, and so the files have to be rewritten to build API-bits-32, hence the .replace surgery in that script.

Is that right?

@matthew-brett
Copy link
Contributor Author

I renamed BUILD_BIT in tools/build_steps_win_arm64.bat to BUILD_BITS for clarity.

@Harishmcw
Copy link
Contributor

The current WoA build script builds binary-bits=64 and API-bits=32.

Yes, that's correct — the current WoA build script builds with binary-bits = 64 and API-bits = 32 (i.e., INTERFACE64=0).

@khmyznikov
Copy link

Hi, what's the current roadblocks here?

@matthew-brett
Copy link
Contributor Author

@mattip - is there anything you are waiting for here? If you agree this is the right approach, it could go in? But is there another tack you would prefer?

@mattip
Copy link
Collaborator

mattip commented May 14, 2025

Something seems off with the wheel zipped into the build artifact. I see a single wheel scipy_openblas32-0.3.29.0.0-py3-none-any.whl. A wheel is a zip file, so unzipping it I see
three directories: scipy_openblas32, scipy_openblas32-0.3.29.0.0.dist-info, and scipy_openblas64. The last one is not supposed to be there. Some of the metadata in the dist-info directory mentions also scipy_openblas64, I guess if the directory is deleted/renamed before the wheel build it will not be mentioned.

I assume the wheel can be used to build scipy/numpy?

I am fine with merging this as-is, it would be nice to provide an win-arm64 wheel with 64-bit interfaces for NumPy, which would enable using larger arrays.

@rgommers
Copy link
Collaborator

It also shouldn't be a none-any wheel, that's going to give problems probably; the any should be win_arm64.

@Harishmcw
Copy link
Contributor

Hi @mattip

Something seems off with the wheel zipped into the build artifact. I see a single wheel scipy_openblas32-0.3.29.0.0-py3-none-any.whl. A wheel is a zip file, so unzipping it I see three directories: scipy_openblas32, scipy_openblas32-0.3.29.0.0.dist-info, and scipy_openblas64. The last one is not supposed to be there. Some of the metadata in the dist-info directory mentions also scipy_openblas64, I guess if the directory is deleted/renamed before the wheel build it will not be mentioned.

  • Previously, while building the scipy_openblas32 wheel, we were copying the contents of scipy_openblas64 into a new directory named scipy_openblas32. However, since scipy_openblas64 was still present in the local directory during the build process, running python -m build included everything in the local directory into the final wheel. This unintentionally added the scipy_openblas64 directory and associated metadata to the wheel.
  • We have now addressed this issue for interface 32-bit builds, where we temporarily move the scipy_openblas64 folder out of the build context before packaging and restore it afterward. This prevents unrelated directories from being included in the generated wheel, resulting in a clean and correct build output.

I assume the wheel can be used to build scipy/numpy?

  • Yes, the code has been updated to support both 32-bit and 64-bit interface builds, and it ensures that the generated wheel is correctly tagged with win_arm64.

  • I've attached the Fixes.patch file, which contains the updated code. This patch should be applied to the current code in this draft PR. Note: I was unable to attach the .bat file directly here, so I've included the patch file instead.

  • During the 64-bit interface build, when we pass the libnamesuffix flag as 64_, the resulting library is named as scipy_openblas64__64. This happens because, in the CMake-based build of OpenBLAS, the SUFFIX64_UNDERSCORE flag is enabled by default when INTERFACE64 is set, which appends an additional _64 suffix. As a result, the library name ends up with a double suffix.

  • To avoid this, we ignore the libnamesuffix flag during the build. The default suffix _64 is then added by the OpenBLAS build system, giving us the library name scipy_openblas_64. While this doesn’t match our preferred naming convention (scipy_openblas64_), it avoids the double suffix issue.

  • We've raised an issue in the OpenBLAS repository to highlight this behavior, and we will raise a PR to adjust the SUFFIX64_UNDERSCORE flag so that the library is named as expected: scipy_openblas64_.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants