InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.41k stars 663 forks source link

GDCM fails to compile using MSVC 2019, 2022 after bump to 3.0.23 #4552

Closed jhlegarreta closed 6 months ago

jhlegarreta commented 6 months ago

Description

GDCM fails to compile using MSVC 2019, 2022: https://open.cdash.org/index.php?project=Insight&date=2024-03-26 vs https://open.cdash.org/index.php?project=Insight&date=2024-03-27

with the errors:

T:\Dashboard\ITK\Modules\ThirdParty\GDCM\src\gdcm\Source\Common\gdcmSystem.cxx(905,3):
error C3861: 'gettimeofday': identifier not found

and

T:\Dashboard\ITK\Modules\ThirdParty\MINC\src\libminc\volume_io\Prog_utils\time.c(133,20):
error C2079: 'tv' uses undefined struct 'timeval'

Errors appeared after GDCM was bumped from 3.0.22 to 3.0.23 in 919cf42.

Not sure why this was not reported by Azure Pipelines, which uses MSVC 2019: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Testing/ContinuousIntegration/AzurePipelinesWindows.yml#L83

Steps to Reproduce

Have a Windows system with MSVC 2019 or 2022 (may happen on newer versions)

  1. Clone ITK master
  2. Configure and generate
  3. Build project

Expected behavior

No compiler errors are reported.

Actual behavior

Build fails with the errors:

T:\Dashboard\ITK\Modules\ThirdParty\GDCM\src\gdcm\Source\Common\gdcmSystem.cxx(905,3):
error C3861: 'gettimeofday': identifier not found

and

T:\Dashboard\ITK\Modules\ThirdParty\MINC\src\libminc\volume_io\Prog_utils\time.c(133,20):
error C2079: 'tv' uses undefined struct 'timeval'

Reproducibility

100%

Versions

ITK master.

Environment

OS: Windows Compiler: MSVC 2019, 2022

Additional Information

File at issue at GDCM 3.0.23: https://github.com/malaterre/GDCM/blob/v3.0.23/Source/Common/gdcmSystem.cxx

File at issue at GDCM 3.0.22: https://github.com/malaterre/GDCM/blob/v3.0.22/Source/Common/gdcmSystem.cxx

They are exactly the same, so maybe it is this block, where gettimeofday is defined, that is not being compiled: https://github.com/malaterre/GDCM/blob/master/Source/Common/gdcmSystem.cxx#L644-L725

jhlegarreta commented 6 months ago

@dzenanz may be relevant to you.

issakomi commented 6 months ago

Reproducibility 100%

It seems that on many Windows machines the build doesn't fail, e.g. zeratul2.kitware, on the dashboard ryzenator.kitware seems to fail.

MINC build fails (similar, related to 'time' stuff) too. Interesting.

thewtex commented 6 months ago

I did not get build errors on my local build: https://open.cdash.org/viewBuildError.php?buildid=9509272

They are exactly the same, so maybe it is this block, where gettimeofday is defined, that is not being compiled: https://github.com/malaterre/GDCM/blob/master/Source/Common/gdcmSystem.cxx#L644-L725

On the system that builds without error:

/d/ITK-build
$ grep GDCM_HAVE_GETTIME CMakeCache.txt
GDCM_HAVE_GETTIMEOFDAY:INTERNAL=

@dzenanz is GDCM_HAVE_GETTIMEOFDAY defined in the ryzenator's CMakeCache.txt?

dzenanz commented 6 months ago

It is. From T:\Dashboard\ITK-build\CMakeCache.txt:

//Have function gettimeofday
GDCM_HAVE_GETTIMEOFDAY:INTERNAL=1

But the build folder is removed nightly, to ensure a fresh build. So this is not some leftover variable from previous builds.

thewtex commented 6 months ago

@dzenanz can you reproduce the error just building GDCM, the release branch?

dzenanz commented 6 months ago

Yes! 0609129 reproduces it.

Configure log:

Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.22631.
The CXX compiler identification is MSVC 19.39.33523.0
The C compiler identification is MSVC 19.39.33523.0
Detecting CXX compiler ABI info
Detecting CXX compiler ABI info - done
Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.39.33519/bin/Hostx64/x64/cl.exe - skipped
Detecting CXX compile features
Detecting CXX compile features - done
Detecting C compiler ABI info
Detecting C compiler ABI info - done
Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.39.33519/bin/Hostx64/x64/cl.exe - skipped
Detecting C compile features
Detecting C compile features - done
Looking for pthread.h
Looking for pthread.h - not found
Looking for stddef.h
Looking for stddef.h - found
Looking for stdlib.h
Looking for stdlib.h - found
Looking for string.h
Looking for string.h - found
Looking for string.h
Looking for string.h - found
Looking for memory.h
Looking for memory.h - found
Looking for stdio.h
Looking for stdio.h - found
Looking for math.h
Looking for math.h - found
Looking for float.h
Looking for float.h - found
Looking for time.h
Looking for time.h - found
Looking for stdarg.h
Looking for stdarg.h - found
Looking for ctype.h
Looking for ctype.h - found
Looking for assert.h
Looking for assert.h - found
Looking for stdint.h
Looking for stdint.h - found
Looking for inttypes.h
Looking for inttypes.h - found
Looking for strings.h
Looking for strings.h - not found
Looking for sys/stat.h
Looking for sys/stat.h - found
Looking for sys/types.h
Looking for sys/types.h - found
Looking for unistd.h
Looking for unistd.h - not found
Checking for 64-bit off_t
Checking for 64-bit off_t - not present
Checking for fseeko/ftello
Checking for fseeko/ftello - not found
Large File support - not found
Looking for include file malloc.h
Looking for include file malloc.h - found
Looking for _aligned_malloc
Looking for _aligned_malloc - found
Looking for posix_memalign
Looking for posix_memalign - not found
Looking for memalign
Looking for memalign - not found
Looking for strsignal
Looking for strsignal - not found
Looking for include file sys/time.h
Looking for include file sys/time.h - not found
Looking for include file winsock.h
Looking for include file winsock.h - found
Looking for include files winsock.h, byteswap.h
Looking for include files winsock.h, byteswap.h - not found
Looking for rpc.h
Looking for rpc.h - found
Looking for langinfo.h
Looking for langinfo.h - not found
Looking for nl_langinfo
Looking for nl_langinfo - not found
Looking for strcasecmp
Looking for strcasecmp - not found
Looking for strncasecmp
Looking for strncasecmp - not found
Looking for strptime
Looking for strptime - not found
Looking for _stricmp
Looking for _stricmp - found
Looking for _strnicmp
Looking for _strnicmp - found
Looking for gettimeofday
Looking for gettimeofday - found
Performing Test GDCM_CXX_HAS_FUNCTION
Performing Test GDCM_CXX_HAS_FUNCTION - Success
Performing Test GDCM_HAVE_WCHAR_IFSTREAM
Performing Test GDCM_HAVE_WCHAR_IFSTREAM - Failed
Performing Test GDCM_HAVE_CODECVT
Performing Test GDCM_HAVE_CODECVT - Success
Could NOT find LibXslt (missing: LIBXSLT_LIBRARIES LIBXSLT_INCLUDE_DIR) 
CMake Warning at Utilities/doxygen/CMakeLists.txt:332 (message):
  Cannot build man page from docbook (need an XSL processor)

Configuring done (47.6s)
Generating done (0.4s)

and build log (after a few incremental builds):

Build started at 17:39...
1>------ Build started: Project: gdcmCommon, Configuration: Debug x64 ------
1>gdcmSystem.cxx
1>C:\Misc\GDCM\Source\Common\gdcmSystem.cxx(905,3): error C3861: 'gettimeofday': identifier not found
1>Done building project "gdcmCommon.vcxproj" -- FAILED.
2>------ Skipped Build: Project: PACKAGE, Configuration: Debug x64 ------
2>Project not selected to build for this solution configuration 
3>------ Skipped Build: Project: INSTALL, Configuration: Debug x64 ------
3>Project not selected to build for this solution configuration 
========== Build: 0 succeeded, 1 failed, 16 up-to-date, 2 skipped ==========
========== Build completed at 17:39 and took 00.744 seconds ==========
thewtex commented 6 months ago

@dzenanz what specific commit, if any, does git bisect identify on the GDCM release branch?

dzenanz commented 6 months ago

I now think this might be due to some Windows update, or CMake version. I tried several versions of GDCM, up to 3.0.2 from 2019, and they all run into the same compile error, in both VS2022 and VS2019.

dzenanz commented 6 months ago

I think that gettimeofday symbol comes from some library recently installed with vcpkg. The root cause is that CHECK_FUNCTION_EXISTS only checks symbol availability for linking. PR https://github.com/malaterre/GDCM/pull/169 fixes it in my local build of GDCM.

dzenanz commented 6 months ago

I tried configuring with CMake 3.16.3, and it also finds gettimeofday, which causes compile error.

N-Dekker commented 6 months ago

Maybe gettimeofday should just be replaced by using modern C++ std::chrono instead 🤷 std::chrono is certainly platform independent.

issakomi commented 6 months ago

Maybe gettimeofday should just be replaced by using modern C++ std::chrono instead 🤷 std::chrono is certainly platform independent.

Yes, I also think it would simplify the code (no need to check with cmake). BTW, the issue will not be completely resolved with the PR, MINC will fail.

bool
System::GetCurrentDateTime(char date[22])
{
  const auto tp = std::chrono::system_clock::now();
  const auto s = std::chrono::time_point_cast<std::chrono::seconds>(tp);
  const auto us = std::chrono::time_point_cast<std::chrono::microseconds>(tp) -
                  std::chrono::time_point_cast<std::chrono::microseconds>(s);
  return FormatDateTime(date, std::chrono::system_clock::to_time_t(s), static_cast<long>(us.count()));
}
dzenanz commented 6 months ago

@issakomi please do propose a PR!

issakomi commented 6 months ago

@issakomi please do propose a PR!

Maybe I shall try a GDCM PR with the std::chrono later, but actually it will conflict with your good PR, AFAIK. And unfortunately I can not reproduce the issue locally, I am mostly on Linux and macOS, my only Windows machine is for releases, i3 4th generation with VS 2019 :).

dzenanz commented 6 months ago

@gdevenyi @vfonov What is the preferred solution for MINC? Using chrono, or a GDCM-equivalent change here: https://github.com/InsightSoftwareConsortium/ITK/blob/f44304d6692acdd86439db7256c029055c39d5ed/Modules/ThirdParty/MINC/src/libminc/CMakeLists.txt#L159

gdevenyi commented 6 months ago

Definitely the GDCM fix, libminc is a C library I don't think we can relying on modern c++ Chrono.

dzenanz commented 6 months ago

@vfonov ITK's update script currently gets MINC from develop branch, but there is also the master branch. My PR was merged into master. Which branch should we use? If develop, could you port my patch there too?

vfonov commented 6 months ago

I merged with master not long time ago.

dzenanz commented 6 months ago

I saw that, but ITK currently update its bundled version from develop. Should we switch that to master?

vfonov commented 6 months ago

i am not planning to do much development. So, maybe.