-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Fix debug build failure on Windows ARM64+MSVC #5285
base: develop
Are you sure you want to change the base?
Conversation
Wouldn't these changes be needed for all the libraries? |
Not necessarily - the linker error only triggers in certain edge-cases, which the C library happened to hit Is there a set of commands I can use to test the other libraries, if the lines I gave didn't build them? |
release_docs/INSTALL_CMAKE.txt shows the default options in sectio VI. the default options are at the beginning of that section: ---------------- General Build Options ------------------------------------- ---------------- HDF5 Build Options ---------------------------------------- |
@@ -1131,6 +1135,10 @@ if (BUILD_SHARED_LIBS) | |||
PUBLIC "$<$<BOOL:${HDF5_ENABLE_HDFS}>:${HDFS_INCLUDE_DIR}>" | |||
INTERFACE "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/include>;$<BUILD_INTERFACE:${HDF5_SRC_INCLUDE_DIRS};${HDF5_SRC_BINARY_DIR}>" | |||
) | |||
if(MSVC AND CMAKE_SYSTEM_PROCESSOR STREQUAL "ARM64" AND ${HDF_CFG_NAME} MATCHES "Debug") | |||
# Required to work around linker error LNK1322 | |||
target_link_options(${HDF5_LIB_TARGET} PRIVATE "/Gy") |
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.
Should be ${HDF5_LIBSH_TARGET}
, but this kind of flag also seems like something that should be set much higher up in the process rather than here, especially considering the very specific platform and build combination logic.
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.
Is there a place you would suggest to be more appropriate? I'm not massively familiar with the codebase, and would like to make sure I've done things correctly.
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.
Since we don't really have other calls to target_link_options
in the library other than a few niche places in the C++ wrappers, it may take a small bit of work. I suggest having another variable similar to HDF5_CMAKE_C_FLAGS
, say HDF5_CMAKE_C_LINK_FLAGS
, then setting whatever you need in config/cmake/HDFCompilerFlags.cmake
maybe. You can see some examples there like:
list (APPEND HDF5_CMAKE_C_FLAGS "-erroff=%none -DBSD_COMP")
where flags are added to the HDF5_CMAKE_C_FLAGS
list. Once you have your flag added to the new variable there, you can have these calls be:
target_link_options (${HDF5_LIB_TARGET} PRIVATE "${HDF5_CMAKE_C_LINK_FLAGS}")
target_link_options (${HDF5_LIBSH_TARGET} PRIVATE "${HDF5_CMAKE_C_LINK_FLAGS}")
for whatever the variable is called. That way will be a bit more maintainable in case other flags need to be added in the future and keeps the platform-specific logic out of the main CMake files.
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.
Another option is the macro in HDFMacros.cmake
macro (TARGET_C_PROPERTIES wintarget libtype)
target_compile_options(${wintarget} PRIVATE
"$<$<C_COMPILER_ID:MSVC>:${WIN_COMPILE_FLAGS}>"
"$<$<CXX_COMPILER_ID:MSVC>:${WIN_COMPILE_FLAGS}>"
)
if(MSVC)
set_property(TARGET ${wintarget} APPEND PROPERTY LINK_FLAGS "${WIN_LINK_FLAGS}")
endif()
#Disable UNITY_BUILD for now
set_property(TARGET ${wintarget} APPEND PROPERTY UNITY_BUILD OFF)
endmacro ()
@byrnHDF Thanks for that, not sure how I missed it when I was reading the docs! Would the following command line (where I switched a number of options on) suffice for making sure things were compiled? Have I missed anything obvious?
|
@anthony-linaro , thank you for your PR! I could confirm the LNK1322 problem using cross-compiler. |
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.
7 build errors are gone:
Proof: https://my.cdash.org/viewBuildError.php?buildid=2803456
@hyoklee - Yes, those are the same errors I've been looking at enabling wider tooling with the following command line:
Sadly, even with |
As @byrnHDF mentioned, some of the components of HDF5 may also need a |
This is with CMD:
build.ninja:
Note it was set as a CFLAG here - upon a further amount of googling, it appears to be the "more correct" way of doing it |
@anthony-linaro , thank you so much for going extra miles in testing! Interestingly, I don't see any link issue with your PR + enabling wider tooling configuration for cross-compilation: https://my.cdash.org/viewBuildError.php?buildid=2803684 I'm using the latest 🌶️ Windows 2025 GitHub Action: https://github.com/hyoklee/actions/actions/runs/13077623330/workflow I wish I have the same real machine to help you further.
|
@hyoklee If it works for you when doing cross-compilation, that is fine - is this mergeable as-is, or should I go with the others' reviews, and try and re-organise where I set the |
Hi, @anthony-linaro , I'm fine as is as you an see that I've already approved it. |
Is this in anyway related to this: https://linaro.atlassian.net/wiki/spaces/WOAR/pages/28684353912/CMake |
Github will be adding a Windows on arm64 runner sometime soon. I suggest we return to this issue when that runner is available and not make changes until we can properly test. |
Hi @byrnHDF, Was there supposed to be a link to something in particular on our CMake page? This PR isn't associated with any of our CMake work, I'm actually just going through adjacent projects to the VFX Reference Platform, and HDF5 came up. That StackOverflow link is to an issue related to ARM64EC - this PR is for plain ARM64 (not EC, which is a seperate thing, and not really necessary here). If it appears to compile okay with the cross-compilation, then I'm happy to consider this done - if you want to run tests every time too, then I suppose we can wait for the GHA runners to be released. Thanks |
Just did a quick search and thought they might be useful info to keep in mind. |
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.
line 1140 should use ${HDF5_LIBSH_TARGET}
Have we confirmed that this actually works as expected? From the previous comments by @anthony-linaro it seems that that's NOT the case. I also think the other point still stands that flags like this should be put higher up than src/CMakeLists.txt. |
When compiling natively on a Windows ARM64 machine and using MSVC in a "Debug" configuration, there was a linker error, namely LNK1322.
This patch enables the compiler flag to fix those issues for those builds.
Tested with the following commands:
Testing done on a Thinkpad T14s Gen6 (Snapdragon X Elite), with VS2022.