-
-
Notifications
You must be signed in to change notification settings - Fork 319
Fix(CMake): Resolve MSVC C99 complex check failure on re-configure #6052
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: develop
Are you sure you want to change the base?
Fix(CMake): Resolve MSVC C99 complex check failure on re-configure #6052
Conversation
Prevent safety block skipping due to cached size variables. Fixes cache pollution issue.
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.
@xorespesp , thank you for the fix!
close #5940
|
Thanks @xorespesp, probably a bit of a silly question, but have you verified that MSVC builds enable the complex number types after reconfiguration and don't skip/disable them? |
config/ConfigureChecks.cmake
Outdated
| if (MSVC AND | ||
| ((NOT ${HDF_PREFIX}_SIZEOF_FLOAT_COMPLEX) OR ${HDF_PREFIX}_SIZEOF__FCOMPLEX) AND | ||
| ((NOT ${HDF_PREFIX}_SIZEOF_DOUBLE_COMPLEX) OR ${HDF_PREFIX}_SIZEOF__DCOMPLEX) AND | ||
| ((NOT ${HDF_PREFIX}_SIZEOF_LONG_DOUBLE_COMPLEX) OR ${HDF_PREFIX}_SIZEOF__LCOMPLEX)) |
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 it possible to fix by resetting everything back to the original settings? That is, similar to "make distclean" for autotools.
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.
That would be done by deleting the build folder - which is usually the best choice most of the time.
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.
While deleting the build folder might be a clean solution when building a library independently, but consider scenarios where HDF5 is consumed via FetchContent in large projects.
Modern IDEs often trigger CMake re-configuration automatically in the background.
If users are forced to wipe the entire build directory every time a project CMake script is modified, it would lead to excessive rebuild times. This significantly degrades the developer experience.
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.
Sorry, not suggesting a "force delete". I don't recall a FetchContent rebuilding something already built, but then I never went looking. My understanding is those Fetched projects should not rebuild without any change to their source. And my understanding is that CMake intention is that those configure cache settings are the intentional function.
Why do you want to force a rebuild, what changed in the main project that would have a bearing on hdf5 being built? The hdf5 configure is inspect system and compiler capabilities, what would be changing in the main project to alter those checks?
Yes, I tested this by integrating the patched HDF5 sources into my project using The CMake output confirms that it falls back to the correct MSVC types instead of disabling them. Here is the output log I observed during second run(re-configuration): (Ref) |
|
So regardless of the rebuild discussion, the problem is that the complex check has a unique check on windows? |
Yes. this issue is specific to the MSVC unique check logic that i mentioned.
it's not that the cached values themselves are incorrect, but rather how the script "interprets" their presence during re-configuration.
This behavior is particularly problematic when using HDF5 via |
config/ConfigureChecks.cmake
Outdated
| HDF_CHECK_TYPE_SIZE ("_Lcomplex" ${HDF_PREFIX}_SIZEOF__LCOMPLEX) | ||
| cmake_pop_check_state () | ||
| if (${HDF_PREFIX}_SIZEOF__FCOMPLEX AND ${HDF_PREFIX}_SIZEOF__DCOMPLEX AND | ||
| ${HDF_PREFIX}_SIZEOF__FCOMPLEX) |
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.
Must also fix this, should be ${HDF_PREFIX}_SIZEOF__LCOMPLEX
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.
Thx, I pushed the update.
config/ConfigureChecks.cmake
Outdated
| message (STATUS "Checking if complex number support is available") | ||
| CHECK_INCLUDE_FILE (complex.h ${HDF_PREFIX}_HAVE_COMPLEX_H) | ||
| if (${HDF_PREFIX}_HAVE_COMPLEX_H) | ||
| set (H5_HAVE_C99_COMPLEX_NUMBERS 1) |
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 this be removed?
config/ConfigureChecks.cmake
Outdated
| set (${HDF_PREFIX}_SIZEOF_LONG_DOUBLE_COMPLEX ${${HDF_PREFIX}_SIZEOF__LCOMPLEX} | ||
| CACHE INTERNAL "SizeOf for long double _Complex" FORCE) | ||
|
|
||
| unset (H5_HAVE_C99_COMPLEX_NUMBERS) |
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.
and then set cached value based on actual detection
set (H5_HAVE_C99_COMPLEX_NUMBERS 0 CACHE INTERNAL "Using MSVC, no C99 complex" FORCE)
| if (NOT H5_HAVE_STDC_NO_COMPLEX) | ||
| # Compile simple test program with complex numbers | ||
| HDF_FUNCTION_TEST (HAVE_COMPLEX_NUMBERS) | ||
|
|
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.
Then add (C99 sucess path)
set (H5_HAVE_C99_COMPLEX_NUMBERS 1 CACHE INTERNAL "Using C99 complex" FORCE)
In practice:
First configuration:
Standard types fail -> MSVC types detected
Sets H5_HAVE_C99_COMPLEX_NUMBERS = 0 in cache (FORCE)
Re-configuration:
Variable loads from cache as 0
Never gets set to 1 because optimistic line 896 was removed
Remains 0 (correct)
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.
Then add (C99 sucess path)
set (H5_HAVE_C99_COMPLEX_NUMBERS 1 CACHE INTERNAL "Using C99 complex" FORCE)
This part is a bit unclear to me.
If I apply the suggested change, I think the code should be modified like this:
...
if (NOT H5_HAVE_STDC_NO_COMPLEX)
# Compile simple test program with complex numbers
HDF_FUNCTION_TEST (HAVE_COMPLEX_NUMBERS)
if (H5_HAVE_COMPLEX_NUMBERS)
+ if (NOT DEFINED H5_HAVE_C99_COMPLEX_NUMBERS)
+ set (H5_HAVE_C99_COMPLEX_NUMBERS 1 CACHE INTERNAL "Using C99 complex" FORCE)
+ endif()
if (H5_HAVE_C99_COMPLEX_NUMBERS)
message (STATUS "Using C99 complex number types")
else ()
message (STATUS "Using MSVC complex number types")
endif ()
...Is this what you mean?
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.
... Regarding your suggestion, i completely agree. removing the "optimistic initialization" and using cached variables for both the MSVC unique check and the standard success path makes the logic much clearer.
I've updated the code accordingly. could you please take a look?
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.
@brtnfld What's the purpose of turning this into a cache variable with set (H5_HAVE_C99_COMPLEX_NUMBERS 1 CACHE INTERNAL "Using C99 complex" FORCE)? The intent was to not pollute the cmake cache with more than is necessary and in this case the variable is only needed to populate values in the features header file used by the library, so there's really no need for it to be a cache variable here. I also find the logic like
if (H5_HAVE_COMPLEX_NUMBERS)
if (NOT DEFINED H5_HAVE_C99_COMPLEX_NUMBERS)
set (H5_HAVE_C99_COMPLEX_NUMBERS 1 CACHE INTERNAL "Using C99 complex" FORCE)
endif ()
endif ()
much more confusing because it's baking in previous assumptions about what the CMake logic did.
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.
I'm also okay with not making it a cache variable.
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.
@jhendersonHDF The current logic assumes complex type detection is all-or-nothing. Can it be true that on 1 or 2 of the 3 standard types are detected? If so, then the NOT DEFINED hides the complexity
brtnfld
left a comment
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.
LGTM, thanks
|
We can talk about this and get back to you. |
hyoklee
left a comment
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.
All tests pass with VS 18 2026:
https://my.cdash.org/viewTest.php?onlypassed&buildid=3220270
|
@xorespesp Could you test my changes over in #6080? I decided that the complex configuration logic needed a bit of an update for clarity anyway while looking at the issue here. The setting of the size macro variables in the "MSVC" block was part of the problem due to using the same name for the C99 type size variables as the final size macro variables. I believe using separate names and some local variables made things a bit easier to read and avoided needing more unnecessary CMake cache variables. |
This is a fix for #6051.
I’ve updated the condition to check whether the fallback types (
_FCOMPLEX) were detected in a previous run, ensuring that the safety block executes even if the standard size variable is cached.Important
Fixes MSVC C99 complex type check in
ConfigureChecks.cmakeby considering fallback types, resolving re-configuration issue #6051.ConfigureChecks.cmakeby updating condition to check fallback types (_FCOMPLEX,_DCOMPLEX,_LCOMPLEX).This description was created by
for 379aaa7. You can customize this summary. It will automatically update as commits are pushed.