Conversation
…ut much alter (at package step) cf #10453 https://gitlab.kitware.com/cmake/cmake/-/issues/25835#note_1502657
… cmake which doesn't work on arm64 cf #10453 https://gitlab.kitware.com/cmake/cmake/-/issues/25835#note_1502657
…AndFixup can silently fail)
jmarrec
left a comment
There was a problem hiding this comment.
I have launch a BUNCH of builds on my fork to get it to work.
But you can find the last working run on this branch's head (6011543 currently) at https://github.com/jmarrec/EnergyPlus/actions/runs/19665370975
I'm also running a workflow right now that basically backports the changes to v25.2.0 and will build a v25.2.0 windows arm64 installer that matches the official release. Whether we add it to the release page or not should be discussed. cf https://github.com/jmarrec/EnergyPlus/actions/runs/19668078998/job/56329806663
| - name: arm64 | ||
| os: windows-11-arm | ||
| arch: arm64 | ||
| python-arch: arm64 | ||
| vs-generator: ARM64 | ||
| package-arch: arm64 | ||
| enable_hardened_runtime: OFF | ||
| pretty: "Windows arm64" | ||
| python-version: 3.12.10 | ||
| allow_failure: true |
| - name: arm64 | ||
| os: windows-11-arm | ||
| test_config: win-arm64 | ||
| package-arch: arm64 | ||
| pretty: "Windows arm64" | ||
| python-version: 3.12.10 | ||
| python-arch: arm64 | ||
| allow_failure: true |
| # - name: Downgrade cmake to 3.28 | ||
| # run: | | ||
| # cmake --version | ||
| # choco install cmake --version=3.28.4 --allow-downgrade --installargs 'ADD_CMAKE_TO_PATH=System' |
There was a problem hiding this comment.
Ok, so I will completely delete this block.
Anyways, this was previously needed because we were lazy and we needed to bring back python38.dll (at the time) that was gone.
- v24.1.0 installer issues #10453
- cf https://gitlab.kitware.com/cmake/cmake/-/issues/25835#note_1502657
Because I can't install cmake 3.28 from choco on arm64 windows, It's time to fix it
| # Note for future readers as this gets extremely confusing to write (and read). | ||
| # The bottom line is that you should inspect the resulting `build/src/cmake_install.cmake` to see what's the resulting command and ensure that is correctly quoted and evaluated | ||
| # Couple of pointers: | ||
| # - `${VAR}` evaluates at *configure* time | ||
| # - `\${VAR}` writes `${VAR}` verbatim, so that will be evaluated at *install* time | ||
| # - `\${CMAKE_COMMAND}` writes `${CMAKE_COMMAND}` verbatim, and I'm not doing `\"\${CMAKE_COMMAND}\"` because this is a single list argument and CMake will handle quoting appropriately already | ||
| # - We do want `CMAKE_INSTALL_PREFIX` evaluated at install time | ||
| # - I'm using `\"-DXXX=${VAR}\"` so that this single argument is definitely properly quoted in the resulting cmake_install.cmake even if there is a space in the value after -DXXX | ||
| # - eg: if RESOLVED_PYTHON_LIBRARY is `/path/to python/python.lib`, it becomes `"-DRESOLVED_PYTHON_LIB=/path/to python/python.lib"` which will be passed as single argument to the operating system which is what we want | ||
| install( | ||
| CODE "execute_process( | ||
| COMMAND ${CMAKE_COMMAND} | ||
| -DRESOLVED_PYTHON_LIB=${RESOLVED_PYTHON_LIBRARY} | ||
| -DEXECUTABLE_PATH=\${CMAKE_INSTALL_PREFIX}/energyplus${CMAKE_EXECUTABLE_SUFFIX} | ||
| -P ${PROJECT_SOURCE_DIR}/cmake/PythonGetLibAndLinkUp.cmake | ||
| COMMAND \${CMAKE_COMMAND} | ||
| \"-DRESOLVED_PYTHON_LIB=${RESOLVED_PYTHON_LIBRARY}\" | ||
| \"-DEXECUTABLE_PATH=\${CMAKE_INSTALL_PREFIX}/energyplus${CMAKE_EXECUTABLE_SUFFIX}\" | ||
| -P \"${PROJECT_SOURCE_DIR}/cmake/PythonGetLibAndLinkUp.cmake\" |
There was a problem hiding this comment.
This is the actual fix, and because it's such a mess to write/read this stuff, I'm putting comments to explain it (which will serve anyone looking at this in the future, including myself, because cmake variable expansion is such a mess and when wrapping it up in install(CODE you get triple the pain)
| def ensure_python_lib_is_there(self, install_path: Path, verbose: bool = False) -> None: | ||
| print("* Ensuring Python library is present in the installation...", end="") | ||
| if self.os == OS.Windows: | ||
| pattern = "python3*.dll" | ||
| elif self.os == OS.Linux: | ||
| pattern = "libpython3*.so*" | ||
| else: # Mac | ||
| pattern = "libpython3*.dylib" | ||
| found = list(install_path.glob(pattern)) | ||
| # We should have found only one | ||
| if len(found) != 1: | ||
| raise Exception(f"Could not find the Python library with pattern {pattern} in {install_path}") | ||
| # Ensure this is a valid library and not a symlink | ||
| python_lib_path = found[0] | ||
| if not python_lib_path.is_file(): | ||
| raise Exception(f"Found Python library '{python_lib_path}' is not file") | ||
|
|
||
| if python_lib_path.is_symlink(): | ||
| raise Exception(f"Found Python library '{python_lib_path}' is not a real file but a symlink") | ||
|
|
||
| if verbose: | ||
| print(f" [{python_lib_path}]", end="") | ||
| print(" [DONE]!") |
There was a problem hiding this comment.
I also added this test to ensure we have a python lib in the package
|
@jmarrec I assume you've tested the release workflow over on your fork, or do you want to trigger a throwaway pre-release over here? |
You'll see above, in the review comment I have already added links to the workflows. I ran a last one on https://github.com/jmarrec/EnergyPlus/actions/runs/19730951580 to produce the v25.2.0 installer (forgot to download a patched file) and this release has the binaries: https://github.com/jmarrec/EnergyPlus/releases/WoA which I have successfully installed on a windows-11 arm64 clean VM. I can confirm that even EP-Launch and EP-Launch-Python work fine, and I successfully ran a PythonPlugin simulation. |
|
@mitchute I added the binaries to the v25.2.0 release, and added a note 
|
|
Thanks @jmarrec. Everything looks good here. |
Pull request overview
Description of the purpose of this PR
Pull Request Author
Reviewer